diff --git a/docs/plans/refactor/phase-2.md b/docs/plans/refactor/phase-2.md new file mode 100644 index 0000000..b50d030 --- /dev/null +++ b/docs/plans/refactor/phase-2.md @@ -0,0 +1,221 @@ +# Phase 2 — Server Hexagonal Restructure + +Scope: `server/` only. Assumes P1 delivered `server/ + shared/ + client/` with `@server/* @shared/* @client/*` aliases and ESLint boundaries. Assumes P0 landed `fastify.inject` characterization tests. Verified against the real code (books.ts 2,244 lines read in full, plus every other route/service). + +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). + +## 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/`). + +```ts +// ports/text-generation.ts — covers 9 generateObject + 6 streamText call sites +export interface ModelRef { provider: ProviderId; model: string } // shared/domain/provider.ts +export interface ChatMessage { role: 'user' | 'assistant'; content: string } + +export interface TextGeneration { + /** books.ts:327,1079,1217; generation-manager.ts:294; chat.ts:63 */ + streamText(req: { model: ModelRef; system?: string; prompt?: string; + messages?: ChatMessage[]; signal?: AbortSignal }): AsyncIterable + /** books.ts:131,270,870,976,1444,1507; covers.ts:145; profile.ts:92; gen-mgr:131 */ + generateObject(req: { model: ModelRef; schema: z.ZodType; prompt: string; system?: string + schemaName?: string; schemaDescription?: string + signal?: AbortSignal }): Promise + /** profile.ts:147 only — tool loop + fullStream text-delta filtering */ + runToolConversation(req: { model: ModelRef; system: string; messages: ChatMessage[] + tools: Record; maxSteps: number + signal?: AbortSignal }): AsyncIterable<{ type: 'text'; text: string }> +} +``` +The adapter owns the 5-minute timeout (`AbortSignal.any([external, AbortSignal.timeout(AI_GENERATION_TIMEOUT_MS)])`, Node 24 — verified). That deletes `createTimeout()` ×5 and the manual signal-combining at generation-manager.ts:287-292. Callers pass only *cancellation* signals (task abort). + +```ts +// ports/book-repository.ts — structured domain data (YAML/MD today) +export interface BookRepository { + listBooks(): Promise; getBook(id: BookId): Promise + saveBook(meta: BookMeta): Promise; deleteBook(id): Promise; resetBook(id): Promise + getToc(id): Promise; saveToc(id, toc: Toc): Promise + getChapter(id, num: ChapterNumber): Promise; saveChapter(id, num, md: string): Promise + chapterExists(id, num): Promise + getQuiz(id, num): Promise; saveQuiz(id, num, quiz): Promise; quizExists(id, num): Promise + getFinalQuiz(id): Promise; saveFinalQuiz(id, quiz): Promise; finalQuizExists(id): boolean + getFeedback(id, num): Promise; saveFeedback(id, num, fb): Promise + getAllFeedback(id): Promise + getProgress(id): Promise; saveChapterProgress(id, num, p: ChapterProgress): Promise + getChaptersRead(id): Promise; getSkillProgress(): Promise + getProfile(): Promise; saveProfile(p): Promise; getProfileUpdatedAt(): Promise + saveBrief/getBrief/saveSummary/getSummary/getAllSummaries/saveReference/getReference/listReferences +} + +// ports/artifact-store.ts — binary artifacts. DOCUMENTED filesystem-bound: +// ffmpeg and HTTP Range streaming need real paths; pretending otherwise is false abstraction. +export interface ArtifactStore { + coverPath(id): Promise; hasCover(id): Promise; coverMtime(id): Promise + saveCover(id, data: Buffer, mediaType): Promise; deleteCover(id): Promise + epubPath(id): string; epubExists(id): boolean; writeEpub(id, bytes: Buffer): Promise + audiobookPath(id): string; audiobookExists(id): boolean; audioDir(id): string + chapterAudioPath(id, num): string; chapterWavPath(id, num): string; chapterAudioExists(id, num): Promise + getAudiobookManifest(id): Promise; saveAudiobookManifest(id, m): Promise + deleteAudiobookArtifacts(id): Promise; recoverFromCrash(): Promise +} + +// ports/speech-synthesis.ts (kokoro) // ports/audio-assembly.ts (ffmpeg) +listVoices(): VoiceInfo[] probeDurationSec(path, signal): Promise +isInstalled(): boolean concatToM4b(req: { inputs: string[]; chapters: +missingComponents(): MissingComponents AudiobookChapterEntry[]; out: string; bitrate: string +install(onProgress, signal): Promise signal: AbortSignal }): Promise +synthesizePreview(voiceId): Promise +synthesizeChapter(req: { text; voiceId; speed; outPath; signal?; onSentence? }): Promise +startWorkerPool(n): Promise; stopWorkerPool(): Promise + +// ports/image-generation.ts +generate(req: { provider: ProviderId; preferredModel: string; prompt: string + signal: AbortSignal }): Promise // image-generation.ts:185 as-is + +// ports/key-vault.ts +get(p: ProviderId): string|null; set(p, key): void; remove(p): void; has(p): boolean +status(): Record + +// ports/diagram-renderer.ts +render(charts: string[]): Promise // markup per chart; '' = failed + +// ports/epub-import.ts — MUST NOT persist (today epub-importer.ts:10 imports book-store) +preview(bytes: Buffer): Promise +read(bytes: Buffer): Promise // pure data: meta fields, chapters[], coverBytes? + +// ports/epub-export.ts +build(req: { title; author; css?; coverPath? + chapters: Array<{ title: string; html: string }> }): Promise + +// ports/background-tasks.ts — expose signal, never the AbortController +start(spec: { type: TaskType; bookId: string; bookTitle: string; total: number }): TaskHandle +report(taskId, current: number, label: string): void; succeed(taskId, result?): void +fail(taskId, error: string): void; cancel(taskId): boolean +get(taskId): Task|undefined; list(): Task[]; findActive(bookId, type?): TaskHandle|undefined +subscribe(cb: (e: TaskEvent) => void): Unsubscribe +export interface TaskHandle { id: string; signal: AbortSignal } +``` +Two cheap extras, approved by the architect: **`ports/clock.ts`** (`nowIso()`, `newId()` — kills 15 `new Date().toISOString()` and makes service unit tests assert exact timestamps) and the `AudioAssembly` split above (ffmpeg is a distinct dependency from kokoro; 2 methods). + +## 2. Adapter map + +| Today | Becomes | Notes | +|---|---|---| +| `services/book-store.ts` (810) | `adapters/fs-book-repository.ts` + `adapters/fs-artifact-store.ts` | factory takes `dataDir`; existing `vi.mock('data-dir')` tests become plain temp-dir tests | +| `services/model-client.ts` + inline `ai` calls | `adapters/ai-sdk-text-generation.ts` | owns timeout, provider validation, `experimental_repairText` logging hook | +| `services/key-store.ts` | `adapters/file-key-vault.ts` | factory args replace import-time side effects | +| `services/image-generation.ts` | `adapters/http-image-generation.ts` | drop its `getKey` import, inject KeyVault | +| `services/kokoro-service.ts` | `adapters/kokoro-speech-synthesis.ts` | | +| ffmpeg internals of `services/audiobook-generator.ts` (`runFfmpeg`, `getAudioDurationSec`, m4b stitch) | `adapters/ffmpeg-audio-assembly.ts` | orchestration half → service (§3) | +| `services/epub-importer.ts` | `adapters/epub2-import.ts` | strip `saveBook/saveToc/saveChapter/saveCover` imports; return data | +| epub-gen-memory block, books.ts:1630-1634,1768 | `adapters/epub-gen-export.ts` | keeps CJS double-default handling (Electron constraint) | +| `index.ts:78-108` kroki decoration + `electron/main.ts:385` | `adapters/kroki-diagram-renderer.ts`, `adapters/electron-diagram-renderer.ts` (registered by main.ts) | **`services/mermaid-renderer.ts` is dead code** — only its own test imports it. Delete it and the `fastify.mermaidRenderer` cast at books.ts:1668 | +| `services/task-manager.ts` | `adapters/in-memory-background-tasks.ts` | | +| books.ts:2007-2019 `spawn(open/explorer/xdg-open)` | `adapters/os-file-manager.ts` | | +| `services/generation-manager.ts` stream hub | `services/chapter-generation-stream.ts` | not a port — in-memory domain service | + +## 3. Service decomposition (`books.ts` and friends) + +Every row = a `services/.ts` exporting `createX(deps)` returning one function. Pure helpers land in `server/domain/`. + +| Current lines | New module | +|---|---| +| books 39-87 ∥ gen-mgr 45-92 (verbatim dup) | `domain/profile-context.ts` `describeLearningProfile(profile)` + labels | +| books 89-121 | `domain/skill-progress-report.ts` (pure) | +| books 33-37, gen-mgr 10-14, chat 60-61, profile 88-89/133-134, covers 141-142 | deleted — adapter owns timeout | +| books 123-152 ∥ gen-mgr 123-154 | `services/generate-quiz.ts` (+ `domain/quiz-scoring.ts`: shuffle w/ injected rng, `QUIZ_QUALITY_RULES` → `prompts/quiz.ts`) | +| books 169-176, 178 | `domain/chapter-range.ts`, `domain/sanitize.ts` | +| books 184-243 | `http/send-media-range.ts` | +| books 246-375 + 1301-1361 | `services/start-book.ts` (+ `services/classify-book-skills.ts` from 268-317) | +| books 377-409 | `services/list-library.ts` | +| books 413-491 | `services/search-library.ts` | +| books 499-535 | `services/read-chapter.ts`, `services/get-chapter-quiz.ts` | +| books 537-579 | `services/submit-feedback.ts` (scoring → domain) | +| books 581-688 | `services/generate-next-chapter.ts` (regenerate = same service, `targetChapterNum` set) | +| books 692-724, gen-mgr 156-382 | `services/chapter-generation-stream.ts` + `http/sse.ts` | +| books 730-754 | `services/update-book-details.ts` (+ `domain/tags.ts` normalize) | +| books 756-772, 774-800 | `services/delete-book.ts`, `services/reset-book.ts`, `services/rate-book.ts` | +| books 802-906 | `services/generate-final-quiz.ts` + `domain/final-quiz-plan.ts` (char tiers 830, count 849, focus text 851-866 — all pure) | +| books 908-1031 | `services/suggest-profile-updates.ts` | +| books 1033-1165 | `services/create-book.ts` (TOC stream, parse, persist, failure marking) | +| books 1167-1299 | `services/revise-toc.ts` | +| books 1363-1489 | `services/suggest-next-book.ts` + `domain/learning-evidence.ts` (summary assembly, pure) | +| books 1491-1533 | `services/suggest-book-details.ts` | +| books 1537-1599 | `services/generate-all-chapters.ts` | +| books 1603-1785 | `services/export-epub.ts` + `domain/epub-embedding.ts` (hidden-div round-trip markup, pairs with importer) | +| books 1811-1922, audiobook-gen 126-379 | `services/generate-audiobook.ts` (gates + voice resolution + narration loop) | +| books 2029-2049 | `services/record-progress.ts` | +| books 2059-2243 | `services/authoring/*.ts` (skeleton, chapter-content, meta, brief, summaries, toc, references, quiz) | +| import.ts 28-47 | `services/import-book.ts` (adapter returns data, service persists) | +| chat.ts, profile.ts 74-187, covers.ts | `services/explain-passage.ts`, `services/interview-profile.ts`, `services/suggest-skills.ts`, `services/generate-cover.ts`, `services/suggest-cover-prompt.ts` | + +**One Zod mechanism — chosen: `http/parse.ts`.** `parseBody(schema, request.body)` throws `RequestValidationError { statusCode: 400, issues }`; one new branch in the existing `setErrorHandler` (index.ts:123) renders the *exact current* body `{ error: 'Invalid request', details: issues }`. Rejected Fastify/ajv `schema.body` validation: it changes the 400 payload shape (breaking the P0 behavior lock and client error handling) and drops Zod defaults/transforms the code relies on (`.default([])`). Rejected per-route preValidation hooks: same 20× repetition, moved. Result: ~20 try/catch blocks → 0. + +**Constants** — `server/constants.ts` (+ `http/status.ts`): `AI_GENERATION_TIMEOUT_MS` (5 sites), `GENERATION_STREAM_CLEANUP_MS`, `TASK_CLEANUP_DELAY_MS` (60s), `MODEL_LIST_TIMEOUT_MS` (10s), `DIAGRAM_RENDER_TIMEOUT_MS` (30s), `DEFAULT_CHAPTER_COUNT` (12, ×4), `MAX_CHAPTERS` (500), `DEFAULT_QUIZ_LENGTH` (3), `FINAL_QUIZ_QUESTION_COUNT`, `FINAL_QUIZ_CHAR_BUDGET_TIERS`, `PROFILE_EXCERPT_CHARS` (300), `CHAT_CONTEXT_CHARS` (4000), `PREV_CHAPTER_TAIL_CHARS` (500), `SEARCH_SNIPPET_RADIUS` (60), `TOC_ERROR_SNIPPET_CHARS` (300), `COVER_CACHE_MAX_AGE_S` (3600), `VOICE_PREVIEW_CACHE_MAX_AGE_S`, `IMPORT_BODY_LIMIT_BYTES`, `M4B_BITRATE`, `RATE_LIMITS` (6 inline configs), `DEFAULT_MODEL`. `DEFAULT_PROVIDER` (12× `?? 'anthropic'`) and `PROVIDERS`/`MODEL_REGEX` (dup'd across model-client:6-7, key-store:17, schemas:188-190) go to `shared/domain/provider.ts`. Route param JSON schemas (books:154-167 ∥ covers:11-15) → `http/route-params.ts`. + +## 4. Thin routes + +Target ~12 files, each <200 lines: `library, generation, assessment, suggestions, authoring, epub, covers, audiobook, chat, profile, models, settings, tasks`. `books.ts` ceases to exist. Shape: + +```ts +export function libraryRoutes({ listLibrary, submitFeedback }: LibraryDeps): FastifyPluginAsync { + return async fastify => { + fastify.get('/api/books', () => listLibrary()) + fastify.post<{ Params: BookChapterParams }>('/api/books/:id/chapters/:num/feedback', + { schema: { params: bookChapterParams } }, + req => submitFeedback({ bookId: req.params.id, chapter: toChapterNumber(req.params.num), + ...parseBody(FeedbackBodySchema, req.body) })) + } +} +``` +Rules (lint-enforced): routes import from `@shared`, `../http/*`, and their deps type only — never `ai`, `node:fs`, `zod` parsing blocks, `await import()`, or adapters. Wiring: `server/composition-root.ts` builds ports+services; `startServer(port, host, overrides?: Partial)` passes services as plugin options, so `fastify.inject` tests boot with fakes. + +## 5. Contract tests + fakes + +Each port ships `ports/.fake.ts` (in-memory, recording) and `ports/.contract.ts` exporting `describeXContract(makeSubject)`. Run against: **fake + real** for BookRepository/ArtifactStore (`mkdtemp` temp dir — replaces the `vi.mock('lib/data-dir')` hack in book-store.test.ts), KeyVault (temp dir), BackgroundTasks (real is in-memory; `vi.useFakeTimers` for cleanup delays). **Fake only** for TextGeneration, ImageGeneration, SpeechSynthesis, AudioAssembly, DiagramRenderer, EpubImport/Export — never hit live providers. Every service gets unit tests against fakes (assert prompts contain profile context, gates return 409, feedback scoring, final-quiz tiering, etc.). + +## 6. Domain split (P3 hand-off) + +`shared/` — P1 delivered the base schemas + book-status predicates + body-schema contracts. Phase 2 S2 adds `shared/contracts/` response types the client needs — `LibraryBook`/`BookSummary` (BookMeta + `hasCover/showTitleOnCover/coverUpdatedAt/chaptersRead/hasAudiobook`), `GenerationStatus`, `ClientTask`/`TaskEvent`, `SearchResults`, `SkillProgress`, `EpubPreview`, `VoiceInfo`, `AudiobookStatus`, and the **SSE event unions** for create-book, revise-toc, start, generate-next, tasks-stream. Verify whether the client renders the six slider label arrays; if yes they move to `shared/domain/preference-labels.ts`. + +`server/domain/` — server-only: `profile-context, final-quiz-plan, quiz-scoring, learning-evidence, skill-progress-report, epub-embedding, chapter-range, tags, sanitize`, plus `server/prompts/*.ts` typed builders. + +## 7. Implementer tasks + +| # | Task | Acceptance check | +|---|---|---| +| S1 | `shared/domain/provider.ts` consolidation (PROVIDERS/MODEL_REGEX/DEFAULT_PROVIDER dedup) — P1 already moved the schemas | tsc clean, tests green, boundary rule passes | +| S2 | `shared/contracts/` response + SSE types derived from real route returns | tsc clean; **notify orchestrator — P3 unblocked** | +| S3 | `constants.ts`, `http/{parse,status,route-params,sse,send-media-range}.ts` + error-handler branch | `rg "5 \* 60 \* 1000" server` = 0; `rg "instanceof ZodError" server/routes` = 0; P0 tests green unedited | +| S4 | All port interfaces + fakes + contract harness (tests first — contract test defines the port) | contract tests green against fakes | +| S5 | All adapters (11) incl. book-store/key-store factory conversion | contract tests green against real adapters; existing service tests green | +| S6a | `composition-root.ts`, `startServer(overrides)`, plugin-option threading | `pnpm dev:server` boots, `/api/health` 200, `electron:preview` boots | +| S6b | **Mechanical split of books.ts** into the 12 target route files, zero logic change | P0 tests green; `books.ts` deleted; slices now own disjoint files | +| S7 | Slice A — library + authoring services/routes (service unit tests written red first) | routes <200 lines, service unit tests, P0 green | +| S8 | Slice B — generation (create, revise-toc, start, next, all, stream hub) | ditto + manual SSE book creation | +| S9 | Slice C — assessment + suggestions | ditto | +| S10 | Slice D — epub import/export, covers, audiobook | ditto + 206 range response verified | +| S11 | Slice E — chat, profile, models, settings, tasks | ditto | +| S12 | Delete dead code: `mermaid-renderer.ts`, `generation-manager.ts` dup profile builder, `fastify.mermaidRenderer` cast, `any` at books.ts:173 (if it survived P0) | `rg "from 'ai'" server/{routes,services}` = 0; lint 0 warnings | +| S13 | Verify sweep: three Electron modes, coverage report, route-doc script sanity | all gates below | + +**Parallel:** S1∥S3; S7-S11 fully parallel (5 Sonnet implementers, separate worktrees, rebased on S6b). **Sequential:** S1→S2→S4→S5→S6a→S6b→{S7..S11}→S12→S13. S6b is the critical de-conflicting step — without it four slices collide in `books.ts`; S6a registers all final route modules so no slice edits `index.ts`. + +## 8. Risks + +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. +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. +- 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. diff --git a/server/constants.ts b/server/constants.ts new file mode 100644 index 0000000..29f2d96 --- /dev/null +++ b/server/constants.ts @@ -0,0 +1,76 @@ +/** + * Tunable values for the server. + * + * These were previously scattered as literals across services and routes, + * several of them redefined independently in more than one file. Each name + * here records what the number means, so a future change is a one-line edit + * instead of a grep-and-replace that risks missing a copy. + * + * Some constants share a value today (e.g. AI_GENERATION_TIMEOUT_MS and + * GENERATION_STREAM_CLEANUP_MS are both five minutes) but are kept separate + * because they mean different things and would be tuned independently. + */ + +// --- AI generation --- + +/** Abort an AI SDK call (chapter, quiz, TOC, cover prompt, etc.) after this long. */ +export const AI_GENERATION_TIMEOUT_MS = 300_000 // 5 minutes + +/** How long a finished or failed SSE generation stream stays in memory, after its last subscriber leaves, before its state is evicted. */ +export const GENERATION_STREAM_CLEANUP_MS = 300_000 // 5 minutes + +/** How long a finished or failed background task (cover, epub, audiobook, etc.) stays in memory before eviction. */ +export const TASK_CLEANUP_DELAY_MS = 60_000 // 1 minute + +/** Timeout for fetching the available-models list from a provider's API. */ +export const MODEL_LIST_TIMEOUT_MS = 10_000 + +/** Timeout for rendering a single mermaid diagram via the kroki.io fallback renderer. */ +export const DIAGRAM_RENDER_TIMEOUT_MS = 30_000 + +// --- Book generation defaults --- + +/** Chapter count used when a create-book request doesn't specify one. */ +export const DEFAULT_CHAPTER_COUNT = 12 + +/** Hard ceiling on chapter count, used when truncating an AI-generated or AI-revised table of contents. */ +export const MAX_CHAPTERS = 500 + +/** Quiz question count used when a request doesn't specify one. */ +export const DEFAULT_QUIZ_LENGTH = 3 + +/** Model ID used as a fallback for on-demand quiz generation when the request doesn't specify one (e.g. reconnecting to a chapter whose quiz file went missing). */ +export const DEFAULT_MODEL = 'claude-sonnet-4-6' + +// --- Context window sizing --- + +/** Characters kept from each chapter when summarizing a book for profile-suggestion prompts. */ +export const PROFILE_EXCERPT_CHARS = 300 + +/** Characters of chapter content sent as context to the inline chat endpoint. */ +export const CHAT_CONTEXT_CHARS = 4000 + +/** Characters of the previous chapter's tail included for continuity when generating the next chapter. */ +export const PREV_CHAPTER_TAIL_CHARS = 500 + +/** Characters of surrounding text included on each side of a full-text search match, when building a result snippet. */ +export const SEARCH_SNIPPET_RADIUS = 60 + +/** Characters of raw model output echoed in the error message when table-of-contents parsing fails. */ +export const TOC_ERROR_SNIPPET_CHARS = 300 + +// --- HTTP caching --- + +/** Cache-Control max-age, in seconds, for served book cover images. */ +export const COVER_CACHE_MAX_AGE_S = 3600 // 1 hour + +/** Cache-Control max-age, in seconds, for synthesized audiobook voice preview clips. */ +export const VOICE_PREVIEW_CACHE_MAX_AGE_S = 2_592_000 // 30 days + +// --- Misc --- + +/** Max request body size, in bytes, for EPUB import endpoints (the request carries the EPUB as base64). */ +export const IMPORT_BODY_LIMIT_BYTES = 20 * 1024 * 1024 // 20MB, to accommodate ~10MB EPUB as base64 + +/** Audio bitrate ffmpeg encodes with when muxing the final M4B audiobook. */ +export const M4B_BITRATE = '64k' diff --git a/server/http/error-handler.test.ts b/server/http/error-handler.test.ts new file mode 100644 index 0000000..117014f --- /dev/null +++ b/server/http/error-handler.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect } from 'vitest' +import { toErrorResponse } from './error-handler.js' +import { RequestValidationError } from './parse.js' + +/** + * The error handler is expressed as a pure function so its behaviour can be + * pinned without booting a server. + * + * Context: until this phase the handler was registered AFTER every route + * plugin had been awaited, so it never applied to any route and every error + * fell through to Fastify's default handler. Phase 0's characterization tests + * recorded that broken behaviour deliberately. These tests describe the + * intended behaviour that registering it correctly now delivers. + */ + +describe('toErrorResponse', () => { + it('renders a validation failure as the exact body the routes used to send', () => { + const issues = [{ code: 'custom' as const, path: ['topic'], message: 'Required' }] + const res = toErrorResponse(new RequestValidationError(issues)) + expect(res.status).toBe(400) + expect(res.body).toEqual({ error: 'Invalid request', details: issues }) + expect(res.logAsServerError).toBe(false) + }) + + it('turns a missing file into a 404 without echoing the path', () => { + const err = Object.assign( + new Error("ENOENT: no such file or directory, open '/Users/someone/Library/tutor/books/abc/meta.yml'"), + { code: 'ENOENT' }, + ) + const res = toErrorResponse(err) + expect(res.status).toBe(404) + expect(res.body).toEqual({ error: 'Not found' }) + // The whole point: no filesystem path may reach the client. + expect(JSON.stringify(res.body)).not.toContain('/Users/') + expect(JSON.stringify(res.body)).not.toContain('meta.yml') + }) + + it('passes through a deliberate client error using its own message', () => { + const err = Object.assign(new Error('Chapter 99 out of range (1-12)'), { statusCode: 400 }) + const res = toErrorResponse(err) + expect(res.status).toBe(400) + expect(res.body).toEqual({ error: 'Chapter 99 out of range (1-12)' }) + expect(res.logAsServerError).toBe(false) + }) + + it('preserves other 4xx codes such as a conflict', () => { + const err = Object.assign(new Error('Book is generating'), { statusCode: 409 }) + expect(toErrorResponse(err)).toMatchObject({ status: 409, body: { error: 'Book is generating' } }) + }) + + it('hides the detail of an unexpected server error and flags it for logging', () => { + const res = toErrorResponse(new Error('connection string user:hunter2@db failed')) + expect(res.status).toBe(500) + expect(res.body).toEqual({ error: 'Internal server error' }) + expect(JSON.stringify(res.body)).not.toContain('hunter2') + expect(res.logAsServerError).toBe(true) + }) + + it('treats an explicit 5xx statusCode as a server error too', () => { + const err = Object.assign(new Error('upstream exploded'), { statusCode: 502 }) + const res = toErrorResponse(err) + expect(res.status).toBe(500) + expect(res.body).toEqual({ error: 'Internal server error' }) + expect(res.logAsServerError).toBe(true) + }) + + it('falls back to a generic message when a client error carries none', () => { + const err = Object.assign(new Error(''), { statusCode: 400 }) + expect(toErrorResponse(err).body).toEqual({ error: 'Internal server error' }) + }) + + it('checks ENOENT before statusCode, so a tagged ENOENT still 404s', () => { + const err = Object.assign(new Error('nope'), { code: 'ENOENT', statusCode: 500 }) + expect(toErrorResponse(err).status).toBe(404) + }) +}) diff --git a/server/http/error-handler.ts b/server/http/error-handler.ts new file mode 100644 index 0000000..3fe044d --- /dev/null +++ b/server/http/error-handler.ts @@ -0,0 +1,79 @@ +import type { FastifyInstance } from 'fastify' +import { RequestValidationError } from './parse.js' + +/** + * The single place an error becomes a response. + * + * This handler existed before but never ran. It was registered on the root + * instance AFTER every route plugin had already been awaited through + * `register()`, and Fastify only propagates an error handler to child + * encapsulation contexts created after it is set. So every route booted + * against Fastify's default handler instead, which meant a missing book + * returned 500 with an absolute filesystem path in the message rather than a + * clean 404. Registering it ahead of the route plugins is the fix. + */ + +/** What the client receives, plus whether the server should log it as a fault. */ +export interface ErrorResponse { + status: number + body: { error: string; details?: unknown } + logAsServerError: boolean +} + +type AppError = Error & { code?: string; statusCode?: number } + +/** + * Pure mapping from a thrown error to a response, kept separate from Fastify + * so it can be unit tested without booting a server. + * + * Order matters. ENOENT is checked before `statusCode` so a missing file 404s + * even when something upstream tagged it 500. + */ +export function toErrorResponse(error: AppError): ErrorResponse { + if (error instanceof RequestValidationError) { + return { + status: 400, + body: { error: 'Invalid request', details: error.issues }, + logAsServerError: false, + } + } + + // Never echo the message here. It contains the absolute path of the file + // that was missing, which is exactly what leaked before this was fixed. + if (error.code === 'ENOENT') { + return { status: 404, body: { error: 'Not found' }, logAsServerError: false } + } + + const statusCode = error.statusCode ?? 500 + + // An unexpected failure may carry anything in its message, including + // credentials from a connection string, so the client gets a generic body + // and the detail goes to the log instead. + if (statusCode >= 500) { + return { status: 500, body: { error: 'Internal server error' }, logAsServerError: true } + } + + return { + status: statusCode, + body: { error: error.message || 'Internal server error' }, + logAsServerError: false, + } +} + +/** + * Registers the handler. MUST be called before any route plugin is registered, + * otherwise the plugins inherit Fastify's default handler and this one never + * runs. See the note at the top of this file. + */ +export function registerErrorHandler(fastify: FastifyInstance): void { + fastify.setErrorHandler((error: AppError, request, reply) => { + const { status, body, logAsServerError } = toErrorResponse(error) + if (logAsServerError) { + fastify.log.error( + { err: error, req: { method: request.method, url: request.url } }, + 'Unhandled server error', + ) + } + return reply.status(status).send(body) + }) +} diff --git a/server/http/parse.test.ts b/server/http/parse.test.ts new file mode 100644 index 0000000..36ea58a --- /dev/null +++ b/server/http/parse.test.ts @@ -0,0 +1,70 @@ +import { describe, it, expect } from 'vitest' +import { z } from 'zod' +import { parseBody, RequestValidationError } from './parse.js' + +/** + * parseBody replaces roughly thirty hand-written try/catch blocks that each + * did the same thing. The exact 400 body those blocks produced is a published + * contract the client reads, so these tests pin it rather than describe it. + */ + +const Schema = z.object({ + topic: z.string().min(1), + count: z.number().int().optional(), + tags: z.array(z.string()).default([]), +}) + +describe('parseBody', () => { + it('returns the parsed value when the body is valid', () => { + expect(parseBody(Schema, { topic: 'effect', count: 3 })).toEqual({ + topic: 'effect', + count: 3, + tags: [], + }) + }) + + it('applies schema defaults, which Fastify ajv validation would not', () => { + // This is why the refactor keeps Zod rather than moving to ajv `schema.body`. + expect(parseBody(Schema, { topic: 'effect' }).tags).toEqual([]) + }) + + it('throws RequestValidationError rather than ZodError', () => { + expect(() => parseBody(Schema, {})).toThrow(RequestValidationError) + }) + + it('carries statusCode 400 so the error handler needs no special casing', () => { + try { + parseBody(Schema, {}) + expect.unreachable('should have thrown') + } catch (err) { + expect((err as RequestValidationError).statusCode).toBe(400) + } + }) + + it('exposes the Zod issues verbatim, which the 400 body echoes as `details`', () => { + try { + parseBody(Schema, { topic: '' }) + expect.unreachable('should have thrown') + } catch (err) { + const issues = (err as RequestValidationError).issues + expect(Array.isArray(issues)).toBe(true) + expect(issues.length).toBeGreaterThan(0) + expect(issues[0]).toHaveProperty('path') + expect(issues[0]).toHaveProperty('message') + } + }) + + it('uses the message the routes previously sent, so the body is unchanged', () => { + try { + parseBody(Schema, {}) + expect.unreachable('should have thrown') + } catch (err) { + expect((err as Error).message).toBe('Invalid request') + } + }) + + it('rejects a null or undefined body the same way as a malformed one', () => { + expect(() => parseBody(Schema, null)).toThrow(RequestValidationError) + expect(() => parseBody(Schema, undefined)).toThrow(RequestValidationError) + }) +}) diff --git a/server/http/parse.ts b/server/http/parse.ts new file mode 100644 index 0000000..b18dc74 --- /dev/null +++ b/server/http/parse.ts @@ -0,0 +1,43 @@ +import type { z } from 'zod' + +/** + * One way to validate a request body. + * + * Every route used to wrap `Schema.parse(request.body)` in its own try/catch, + * check `err instanceof ZodError`, and hand-write the same 400 response. That + * was repeated about thirty times, and the eight MCP authoring routes forgot + * the guard entirely, so bad input there surfaced as a 500. + * + * Fastify's own ajv `schema.body` validation was considered and rejected. It + * produces a different 400 payload, which the client already depends on, and + * it drops the Zod defaults and transforms the handlers rely on, such as + * `.default([])`. + */ + +/** + * Thrown when a request body does not match its schema. Carries `statusCode` + * so the error handler needs no special casing, and `issues` so the response + * can echo them as `details`, exactly as the hand-written blocks did. + */ +export class RequestValidationError extends Error { + readonly statusCode = 400 + readonly issues: z.core.$ZodIssue[] + + constructor(issues: z.core.$ZodIssue[]) { + super('Invalid request') + this.name = 'RequestValidationError' + this.issues = issues + } +} + +/** + * Parses a request body, throwing {@link RequestValidationError} on failure. + * Returns the parsed value, so schema defaults and transforms still apply. + */ +export function parseBody(schema: z.ZodType, body: unknown): T { + const result = schema.safeParse(body) + if (!result.success) { + throw new RequestValidationError(result.error.issues) + } + return result.data +} diff --git a/server/http/route-params.ts b/server/http/route-params.ts new file mode 100644 index 0000000..66ed9cc --- /dev/null +++ b/server/http/route-params.ts @@ -0,0 +1,41 @@ +import { PROVIDERS } from '@shared/provider.js' + +/** + * Fastify JSON-schema fragments for route `:params`, shared by every route + * file that needs them. Before this module, `books.ts` and `covers.ts` each + * hand-rolled an identical book-id schema, and `models.ts` restated the + * provider list as a separate regex literal. + * + * The book id and chapter patterns must stay byte-identical to what shipped + * before — the Phase 0 characterization tests assert the exact 400 responses + * they produce for malformed params. + */ + +/** Matches a route `:id` that must be a book id, e.g. `/api/books/:id`. */ +export const bookIdSchema = { + type: 'object' as const, + properties: { id: { type: 'string' as const, pattern: '^[a-z0-9-]{1,50}$' } }, + required: ['id'] as const, +} + +/** Matches a route with both a book `:id` and a chapter `:num`, e.g. `/api/books/:id/chapters/:num`. */ +export const bookChapterSchema = { + type: 'object' as const, + properties: { + id: { type: 'string' as const, pattern: '^[a-z0-9-]{1,50}$' }, + num: { type: 'string' as const, pattern: '^[1-9][0-9]{0,2}$' }, + }, + required: ['id', 'num'] as const, +} + +/** + * Matches a route `:provider` that must be one of the known AI providers. + * Built from the shared provider list rather than a restated regex literal, + * so a provider added to `shared/provider.ts` doesn't also need updating + * here. + */ +export const providerParamSchema = { + type: 'object' as const, + properties: { provider: { type: 'string' as const, pattern: `^(${PROVIDERS.join('|')})$` } }, + required: ['provider'] as const, +} diff --git a/server/http/send-media-range.ts b/server/http/send-media-range.ts new file mode 100644 index 0000000..3813a05 --- /dev/null +++ b/server/http/send-media-range.ts @@ -0,0 +1,72 @@ +import type { FastifyReply } from 'fastify' +import { STATUS_NOT_FOUND, STATUS_RANGE_NOT_SATISFIABLE } from './status.js' + +/** + * Serve a media file with HTTP Range support. Writes via reply.raw so + * Fastify doesn't re-derive Content-Length from a stream (it ends up + * at 0 for streams, breaking