Phase 2 stage 1 — Foundations: provider, contracts, constants, and the error handler fix - #47
Merged
Conversation
Records the port catalog, the adapter map, the service decomposition of books.ts, the thin route shape, and the contract test strategy, so the two phase 2 PRs can be read against the same source the work followed. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
… them Pins the exact 400 body the routes have been sending by hand in about thirty places, so replacing those blocks cannot quietly change the contract the client reads. Also specifies the error handler behaviour that registering it correctly will deliver, namely a 404 for a missing file with no filesystem path echoed back, pass-through of deliberate client errors, and a generic body for anything unexpected. Deliberately red, neither module exists yet. The pre-commit hook is bypassed for that reason alone and the next commits turn these green. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
The handler was set after every route plugin had been awaited, and Fastify only propagates an error handler to encapsulation contexts created after it is set, so no route ever used it. Phase 0 found this and froze the resulting behaviour in characterization tests rather than fixing it out of scope. Moving the registration ahead of the route plugins delivers what the handler always intended. A missing book now returns 404 with a clean body instead of a 500 whose message contained an absolute filesystem path. Errors carrying only a status code, including parameter validation failures, keep that status but render in the app error shape that the client already reads. Extracts the mapping into a pure function so it is testable without booting a server, and adds parseBody, which stage two uses to replace about thirty hand-written validation blocks. Seven Phase 0 assertions were updated for exactly these behaviours, each marked in place with the reason. The eight MCP authoring routes still return 500 on bad input, since they parse inline rather than through parseBody, but their body is now generic instead of echoing the raw parser message. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
This pins the behaviour shared/provider.ts must have before it exists, including the enum order, the PROVIDERS tuple, the model regex, the default provider, and the isProviderId guard. The test file alone is red, since it imports a module that does not exist until the next commit adds it. Running it in isolation, for example by checking out this commit on its own, fails with a missing module error until that next commit lands. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Anthropic, OpenAI, and Google were declared independently in shared/contracts.ts, server/services/model-client.ts, and server/services/key-store.ts, and the default provider string was repeated twelve times across the route handlers. This adds shared/provider.ts as the single source of truth for ProviderSchema, the PROVIDERS tuple, the MODEL_REGEX pattern, DEFAULT_PROVIDER, and the isProviderId guard. shared/contracts.ts now imports ProviderSchema from the new module and re-exports it so existing consumers keep working unchanged, and its model regex now points at the shared MODEL_REGEX. model-client.ts and key-store.ts drop their local copies of the provider list and use isProviderId and PROVIDERS instead, with identical error messages and an identical keyStatus shape. Every fallback of the form provider ?? 'anthropic' in the route handlers now reads provider ?? DEFAULT_PROVIDER. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
This adds shared/responses.ts and shared/events.ts as the dependency root's types for what the server routes actually return today. Every type is read off the real route handler or the SSE emitter it forwards, cross checked against the books route characterization test where one exists, rather than designed from scratch. shared/responses.ts covers LibraryBook, GenerationStage, GenerationStatus, BookDetail, SearchResults, SkillProgress, TaskType, TaskStatus, TaskProgress, ClientTask, EpubPreview, VoiceInfo, and AudiobookStatus. EpubPreview is inferred from the existing ImportEpubPreviewResponseSchema in shared/contracts.ts instead of being restated. shared/events.ts covers the five SSE streams named in the plan as discriminated unions on the literal type field, create book, revise toc, start book, generate or regenerate a chapter including the reconnect stream, and the background tasks stream. The four generation streams share an identical error event shape in the real code, so that variant is factored into one exported type and reused rather than repeated four times. Both files are types only, no Zod schemas, no runtime values, so they compile away entirely. shared/README.md gets two new table rows for the added modules and nothing else changes. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
Five files each defined their own five minute AI timeout, two route files hand-rolled an identical book id param schema, and a third restated the provider list as a separate regex literal. All of them now come from one place, so changing a value is a one line edit rather than a grep that risks missing a copy. Adds server/constants.ts for the tunable numbers, http/status.ts for the status codes this codebase uses, http/route-params.ts for the Fastify param schemas, and http/send-media-range.ts for the range request helper that was sitting inside the books route file. The provider param pattern is now derived from the shared provider list and was verified to build the exact same string as the literal it replaced. The book id and chapter patterns are byte identical too, since the Phase 0 characterization tests assert the responses they produce. Claude-Session: https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 1 of Phase 2. Small and early on purpose, so the Phase 3 client work can rebase on the shared types it needs without waiting for the full server restructure. Part of #41.
What this is
Three foundations: one source of truth for the AI providers, the shared response and SSE contract types the client will consume, and the server's named constants plus its HTTP helper modules. Plus one sanctioned behavior fix that had to land with the error-handling work.
The sanctioned fix: the error handler never ran
Phase 0's characterization tests recorded a real defect.
buildServer()calledsetErrorHandlerAFTER every route plugin had already been awaited throughregister(). Fastify only propagates an error handler to encapsulation contexts created after it is set, so all nine plugins booted against Fastify's default handler and the app's own handler was dead code. Phase 0 froze that behavior deliberately rather than fixing it out of scope.Registering it ahead of the route plugins is the fix. The mapping is now a pure function, so it is unit tested without booting a server.
Seven Phase 0 assertions were updated, each marked in place with the reason.
books.characterization.test.tsGET /api/books/:idunknown id{error:'Not found'}, no pathbooks.characterization.test.tsGET /api/books/:idbad id pattern{statusCode,code,error,message}{error}books.characterization.test.tsDELETEthenGET{error:'Not found'}books.characterization.test.ts{error,message,statusCode}{error}, same message textbooks.characterization.test.tsFST_ERR_VALIDATIONshape{error}books.characterization.test.ts0FST_ERR_VALIDATIONshape{error}audiobook.characterization.test.tsvoiceIdpatternFST_ERR_VALIDATIONshape{error}No status code changed except the two ENOENT cases going 500 to 404. The rest keep their status and only move to the
{error}shape, which is whatclient/lib/api.tsalready reads viaerr.error. The file-header FROZEN QUIRK note was rewritten to explain the resolution rather than deleted, since it is why those assertions carry a change marker.The eight MCP authoring routes did NOT flip to 400 in this PR. They still return 500 on bad input, because they parse inline rather than through
parseBody. Their body did change from Fastify's shape, which echoed the raw parser message, to a generic{error:'Internal server error'}, which is a leak fix. The 500 to 400 flip happens in stage 2 whenparseBodyreaches them.Foundations
One provider list.
'anthropic', 'openai', 'google'was declared in three places and the default provider was repeated 12 times. All of it now comes fromshared/provider.ts, along withMODEL_REGEX, which had two copies. A fifth duplicate, the Fastify param pattern inmodels.ts, is now derived from the shared list and verified to build the exact same string.Shared contract types.
shared/responses.tsandshared/events.tsare types only, so they compile away. Every type is read off the handler that produces it, with JSDoc naming the route. The SSE unions are discriminated on the realtypediscriminants found in thereply.raw.writecalls.Named values. Five files each defined their own five minute AI timeout.
server/constants.tsnow holds the tunable numbers, andserver/http/holdsstatus.ts,route-params.ts(two files hand-rolled an identical book id schema),send-media-range.ts(lifted out of the books route file), plusparse.tsanderror-handler.ts.Gate
pnpm test23 passed (23)files,315 passed (315)tests, up from 286pnpm typecheckpnpm lint/api/healthreturns ok,/api/booksserves real data/api/books/does-not-existreturns{"error":"Not found"}at HTTP 404, no pathparams/provider must match pattern "^(anthropic|openai|google)$"at HTTP 400, byte identical to the literal it replacedrg "5 \* 60 \* 1000" server/rg "AI_TIMEOUT_MS =" server/server/http/route-params.tsTDD
Both new behaviors were specified before they existed.
shared/provider.test.ts,server/http/parse.test.ts, andserver/http/error-handler.test.tswere each committed red, with the pre-commit hook bypassed for that reason alone and stated in the commit body, then turned green by the following commit.Findings handed to Phase 3
The contract work surfaced concrete drift the client team should know about.
interface Book, duplicated byte-identically inclient/App.tsxandclient/components/BookListRow.tsx, marks five fields optional that the server always sends:hasCover,showTitleOnCover,coverUpdatedAt,hasAudiobook,prompt.status?: string. The server always sends one of six literals.shared/book-status.tshas the right type.updatedAt,profileOverrides,audioGeneratedChapters.client/lib/parse-sse-stream.tshas ONE flattened event union covering four different streams, so itsdonevariant conflates create-book's shape with generate-chapter's unrelated one, and itsstageis a barestring.shared/events.tsreplaces this with five separate discriminated unions.client/lib/api.ts's existingSearchResponseandEpubPreviewalready match the derived types exactly, so adopting them there is a pure rename.client/lib/providers.tsindependently declares the provider list. It should adopt@shared/provider. Left alone because Phase 3 ownsclient/.Deviations
shared/domain/provider.tsand ashared/contracts/directory. Phase 1 deliveredshared/as flat modules, so directories of those names would collide with the existingshared/domain.tsandshared/contracts.ts. These land asshared/provider.ts,shared/responses.ts, andshared/events.ts, mirroringshared/book-status.ts. Chosen for zero import churn while Phase 3 is running concurrently.http/sse.tsis deferred to stage 2. Extracting it now would create a module with no callers, since the streaming routes do not move until the route split.stageSSE event is declared but never emitted. No code path constructs one today. It is kept in the union because it is part of the server's own exported event type and the client already handles it in three places.https://claude.ai/code/session_01FSkESkJTRpt3Ye2cM7c5n5