From f38b5a9efa21df865c81ab1ced99ef31571d0ff3 Mon Sep 17 00:00:00 2001 From: Ethan Date: Wed, 22 Apr 2026 16:43:52 -0700 Subject: [PATCH] feat: runtime response-schema validation in dev/test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged that `check:openapi` only catches schema↔spec drift — a formatter change that forgets to update schemas still slips past. This closes that gap with a dev/test-mode middleware that wraps `res.json()` and parses the outgoing body against the route's declared response schema. Prod is a pass-through no-op. - src/middleware/validate-response.ts — the wrapper. Throws loudly in dev/test when a 2xx body doesn't match its schema; no-op when NODE_ENV=production. - src/routes/response-schema-map.ts — the route→schema map. Keyed by Express's router-relative `${method} ${route.path}` so the lookup works for parameterized paths like `/:id`. - Wired into both createMemoryRouter and createAgentRouter via `router.use(validateResponse(...))` before any handler. The validator caught two real formatter↔schema drift issues on its first run, both fixed here: - `Date` leaking into search memory `created_at`. Express serializes Date → ISO string, so the wire shape is string, but `res.json()` sees the raw Date before serialization. Added an `IsoDateString` preprocess that accepts both and validates as string. - `NaN` leaking into search similarity/score/importance. `JSON.stringify(NaN)` writes `null` on the wire, so the wire shape is `number | null`, but `res.json()` sees NaN. Added a `NumberOrNaN` preprocess that converts NaN → null and validates as nullable number. Schemas now report the accurate wire types (string, nullable number) and the OpenAPI spec regenerates with zero further drift. All 1037 core tests pass with the middleware active. Co-Authored-By: Claude Opus 4.7 (1M context) --- openapi.json | 30 +++++++++--- openapi.yaml | 24 +++++++--- src/middleware/validate-response.ts | 63 +++++++++++++++++++++++++ src/routes/agents.ts | 5 ++ src/routes/memories.ts | 5 ++ src/routes/response-schema-map.ts | 73 +++++++++++++++++++++++++++++ src/schemas/responses.ts | 64 ++++++++++++++++++------- 7 files changed, 234 insertions(+), 30 deletions(-) create mode 100644 src/middleware/validate-response.ts create mode 100644 src/routes/response-schema-map.ts diff --git a/openapi.json b/openapi.json index 5ffd398..973621a 100644 --- a/openapi.json +++ b/openapi.json @@ -3592,13 +3592,22 @@ "type": "string" }, "importance": { - "type": "number" + "type": [ + "number", + "null" + ] }, "score": { - "type": "number" + "type": [ + "number", + "null" + ] }, "similarity": { - "type": "number" + "type": [ + "number", + "null" + ] }, "source_site": { "type": "string" @@ -4077,13 +4086,22 @@ "type": "string" }, "importance": { - "type": "number" + "type": [ + "number", + "null" + ] }, "score": { - "type": "number" + "type": [ + "number", + "null" + ] }, "similarity": { - "type": "number" + "type": [ + "number", + "null" + ] }, "source_site": { "type": "string" diff --git a/openapi.yaml b/openapi.yaml index 64e693f..2ceaa04 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -2425,11 +2425,17 @@ paths: id: type: string importance: - type: number + type: + - number + - "null" score: - type: number + type: + - number + - "null" similarity: - type: number + type: + - number + - "null" source_site: type: string required: @@ -2758,11 +2764,17 @@ paths: id: type: string importance: - type: number + type: + - number + - "null" score: - type: number + type: + - number + - "null" similarity: - type: number + type: + - number + - "null" source_site: type: string required: diff --git a/src/middleware/validate-response.ts b/src/middleware/validate-response.ts new file mode 100644 index 0000000..30fc236 --- /dev/null +++ b/src/middleware/validate-response.ts @@ -0,0 +1,63 @@ +/** + * @file Dev/test-mode response validator. + * + * Wraps `res.json()` to parse the outgoing body against a route→schema + * map declared alongside the OpenAPI response schemas. Catches the + * formatter↔schema drift that `check:openapi` alone can't see: if a + * formatter in `routes/memory-response-formatters.ts` changes a field + * name or shape without the corresponding update in + * `schemas/responses.ts`, tests fail loudly at the point of emission. + * + * No-op in production. Enabled by default everywhere else (tests, + * `NODE_ENV=development`) so the check is on in the only environments + * that actually run tests or local dev. + * + * Route keys follow Express's router-relative `${method} ${route.path}` + * format (e.g. `post /ingest`, `get /:id`). `req.route.path` is set by + * the time `res.json()` is called inside the handler, so the lookup + * resolves correctly even for parameterized paths. + * + * Error-path responses (4xx/5xx) are not validated — each route emits + * its own error envelope that's already schema-checked by the + * `validateBody` middleware on the way in, and runtime error shapes + * are intentionally less strict than success bodies. + */ + +import type { RequestHandler } from 'express'; +import type { z } from 'zod'; + +export type ResponseSchemaMap = Record; + +/** + * Build a middleware that validates 2xx JSON response bodies against + * the supplied schema map. Returns a pass-through no-op when + * NODE_ENV is `production`, so prod has zero per-request cost. + */ +export function validateResponse(schemaMap: ResponseSchemaMap): RequestHandler { + if (process.env.NODE_ENV === 'production') { + return (_req, _res, next) => next(); + } + return (req, res, next) => { + const originalJson = res.json.bind(res); + res.json = function patchedJson(body: unknown) { + if (res.statusCode >= 200 && res.statusCode < 300 && req.route?.path) { + const key = `${req.method.toLowerCase()} ${req.route.path}`; + const schema = schemaMap[key]; + if (schema) { + const result = schema.safeParse(body); + if (!result.success) { + throw new Error( + `Response body for ${key} violates declared schema.\n` + + `Formatter output does not match ${key}'s OpenAPI response schema. ` + + `Either update the formatter, or if the change is intentional, update ` + + `src/schemas/responses.ts and regenerate the OpenAPI spec.\n` + + `Zod issues:\n${JSON.stringify(result.error.issues, null, 2)}`, + ); + } + } + } + return originalJson(body); + }; + next(); + }; +} diff --git a/src/routes/agents.ts b/src/routes/agents.ts index 97d28f9..bba8cc1 100644 --- a/src/routes/agents.ts +++ b/src/routes/agents.ts @@ -13,6 +13,8 @@ import { Router, type Request, type Response } from 'express'; import { AgentTrustRepository } from '../db/agent-trust-repository.js'; import { handleRouteError } from './route-errors.js'; import { validateBody, validateQuery, validateParams } from '../middleware/validate.js'; +import { validateResponse } from '../middleware/validate-response.js'; +import { AGENT_RESPONSE_SCHEMAS } from './response-schema-map.js'; import { SetTrustBodySchema, GetTrustQuerySchema, @@ -24,6 +26,9 @@ import { export function createAgentRouter(trustRepo: AgentTrustRepository): Router { const router = Router(); + // Dev/test-mode response validator: no-op in production, throws loudly + // if any 2xx body violates the schema declared in responses.ts. + router.use(validateResponse(AGENT_RESPONSE_SCHEMAS)); registerSetTrustRoute(router, trustRepo); registerGetTrustRoute(router, trustRepo); registerListConflictsRoute(router, trustRepo); diff --git a/src/routes/memories.ts b/src/routes/memories.ts index 2772f3f..480dfca 100644 --- a/src/routes/memories.ts +++ b/src/routes/memories.ts @@ -33,6 +33,8 @@ import { import type { AgentScope, WorkspaceContext } from '../db/repository-types.js'; import { handleRouteError } from './route-errors.js'; import { validateBody, validateQuery, validateParams } from '../middleware/validate.js'; +import { validateResponse } from '../middleware/validate-response.js'; +import { MEMORY_RESPONSE_SCHEMAS } from './response-schema-map.js'; import { IngestBodySchema, type IngestBody, @@ -106,6 +108,9 @@ export function createMemoryRouter( ): Router { const router = Router(); registerCors(router); + // Dev/test-mode response validator: no-op in production, throws loudly + // if any 2xx body violates the schema declared in responses.ts. + router.use(validateResponse(MEMORY_RESPONSE_SCHEMAS)); registerIngestRoute(router, service); registerQuickIngestRoute(router, service); registerSearchRoute(router, service, configRouteAdapter); diff --git a/src/routes/response-schema-map.ts b/src/routes/response-schema-map.ts new file mode 100644 index 0000000..c51fa4e --- /dev/null +++ b/src/routes/response-schema-map.ts @@ -0,0 +1,73 @@ +/** + * @file Route→schema maps consumed by `validate-response` middleware. + * + * Keyed by Express's router-relative `${method} ${route.path}` format + * (method lowercase, path matches what Express stores in `req.route.path`). + * When a new route is added, add a corresponding entry here; the + * validator is a no-op for routes not in the map (so new routes + * ship without a hard dependency on a schema existing yet). + */ + +import { + IngestResponseSchema, + SearchResponseSchema, + ExpandResponseSchema, + ListResponseSchema, + GetMemoryResponseSchema, + StatsResponseSchema, + HealthResponseSchema, + ConfigUpdateResponseSchema, + ConsolidateResponseSchema, + DecayResponseSchema, + CapResponseSchema, + LessonsListResponseSchema, + LessonStatsResponseSchema, + LessonReportResponseSchema, + ReconciliationResponseSchema, + ReconcileStatusResponseSchema, + ResetSourceResponseSchema, + SuccessResponseSchema, + MutationSummaryResponseSchema, + AuditRecentResponseSchema, + AuditTrailResponseSchema, + TrustResponseSchema, + ConflictsListResponseSchema, + ResolveConflictResponseSchema, + AutoResolveConflictsResponseSchema, +} from '../schemas/responses.js'; +import type { ResponseSchemaMap } from '../middleware/validate-response.js'; + +export const MEMORY_RESPONSE_SCHEMAS: ResponseSchemaMap = { + 'post /ingest': IngestResponseSchema, + 'post /ingest/quick': IngestResponseSchema, + 'post /search': SearchResponseSchema, + 'post /search/fast': SearchResponseSchema, + 'post /expand': ExpandResponseSchema, + 'get /list': ListResponseSchema, + 'get /stats': StatsResponseSchema, + 'get /health': HealthResponseSchema, + 'put /config': ConfigUpdateResponseSchema, + 'post /consolidate': ConsolidateResponseSchema, + 'post /decay': DecayResponseSchema, + 'get /cap': CapResponseSchema, + 'get /lessons': LessonsListResponseSchema, + 'get /lessons/stats': LessonStatsResponseSchema, + 'post /lessons/report': LessonReportResponseSchema, + 'delete /lessons/:id': SuccessResponseSchema, + 'post /reconcile': ReconciliationResponseSchema, + 'get /reconcile/status': ReconcileStatusResponseSchema, + 'post /reset-source': ResetSourceResponseSchema, + 'get /:id': GetMemoryResponseSchema, + 'delete /:id': SuccessResponseSchema, + 'get /audit/summary': MutationSummaryResponseSchema, + 'get /audit/recent': AuditRecentResponseSchema, + 'get /:id/audit': AuditTrailResponseSchema, +}; + +export const AGENT_RESPONSE_SCHEMAS: ResponseSchemaMap = { + 'put /trust': TrustResponseSchema, + 'get /trust': TrustResponseSchema, + 'get /conflicts': ConflictsListResponseSchema, + 'put /conflicts/:id/resolve': ResolveConflictResponseSchema, + 'post /conflicts/auto-resolve': AutoResolveConflictsResponseSchema, +}; diff --git a/src/schemas/responses.ts b/src/schemas/responses.ts index 0566dea..1900389 100644 --- a/src/schemas/responses.ts +++ b/src/schemas/responses.ts @@ -14,6 +14,34 @@ import { z } from './zod-setup'; +/** + * ISO date-time string on the wire. Accepts `Date` at validation time + * so `validateResponse` middleware can run before Express serializes + * the body — Express's JSON.stringify converts Date → ISO string, + * matching the outer `z.string()` the OpenAPI spec documents. + */ +const IsoDateString = z.preprocess( + (v) => (v instanceof Date ? v.toISOString() : v), + z.string(), +); + +/** Same pattern but nullable (schema exports nullable string). */ +const IsoDateStringOrNull = z.preprocess( + (v) => (v instanceof Date ? v.toISOString() : v), + z.string().nullable(), +); + +/** + * Float that may be NaN on the JS side. `JSON.stringify(NaN)` emits + * `null`, so the wire shape is `number | null` — the schema reflects + * that, and the preprocess converts NaN so the validator (which runs + * before serialization) matches. + */ +const NumberOrNaN = z.preprocess( + (v) => (typeof v === 'number' && Number.isNaN(v) ? null : v), + z.number().nullable(), +); + // --------------------------------------------------------------------------- // Shared sub-schemas // --------------------------------------------------------------------------- @@ -46,12 +74,12 @@ const MemoryRowSchema = z.object({ summary: z.string(), overview: z.string(), trust_score: z.number(), - observed_at: z.string(), - created_at: z.string(), - last_accessed_at: z.string(), + observed_at: IsoDateString, + created_at: IsoDateString, + last_accessed_at: IsoDateString, access_count: z.number(), - expired_at: z.string().nullable().optional(), - deleted_at: z.string().nullable().optional(), + expired_at: IsoDateStringOrNull.optional(), + deleted_at: IsoDateStringOrNull.optional(), network: z.unknown(), opinion_confidence: z.number().nullable().optional(), observation_subject: z.string().nullable().optional(), @@ -63,11 +91,11 @@ const MemoryRowSchema = z.object({ const SearchMemoryItemSchema = z.object({ id: z.string(), content: z.string(), - similarity: z.number().optional(), - score: z.number().optional(), - importance: z.number().optional(), + similarity: NumberOrNaN.optional(), + score: NumberOrNaN.optional(), + importance: NumberOrNaN.optional(), source_site: z.string().optional(), - created_at: z.string().optional(), + created_at: IsoDateString.optional(), }).openapi({ description: 'Projected memory record in a search result.' }); const TierAssignmentSchema = z.object({ @@ -157,7 +185,7 @@ const LessonRowSchema = z.object({ severity: z.enum(['low', 'medium', 'high', 'critical']), active: z.boolean(), metadata: z.record(z.string(), z.unknown()), - created_at: z.string(), + created_at: IsoDateString, }).passthrough().openapi({ description: 'Lesson row from the repository.' }); const ClaimVersionRowSchema = z.object({ @@ -171,15 +199,15 @@ const ClaimVersionRowSchema = z.object({ source_site: z.string(), source_url: z.string(), episode_id: z.string().nullable().optional(), - valid_from: z.string(), - valid_to: z.string().nullable().optional(), + valid_from: IsoDateString, + valid_to: IsoDateStringOrNull.optional(), superseded_by_version_id: z.string().nullable().optional(), mutation_type: z.enum(['add', 'update', 'supersede', 'delete', 'clarify']).nullable().optional(), mutation_reason: z.string().nullable().optional(), previous_version_id: z.string().nullable().optional(), actor_model: z.string().nullable().optional(), contradiction_confidence: z.number().nullable().optional(), - created_at: z.string(), + created_at: IsoDateString, }).passthrough().openapi({ description: 'Claim-version row (one snapshot in a memory\'s history).' }); const MemoryConflictRowSchema = z.object({ @@ -195,9 +223,9 @@ const MemoryConflictRowSchema = z.object({ clarification_note: z.string().nullable(), status: z.enum(['open', 'resolved_new', 'resolved_existing', 'resolved_both', 'auto_resolved']), resolution_policy: z.string().nullable(), - resolved_at: z.string().nullable(), - created_at: z.string(), - auto_resolve_after: z.string().nullable(), + resolved_at: IsoDateStringOrNull, + created_at: IsoDateString, + auto_resolve_after: IsoDateStringOrNull, }).passthrough().openapi({ description: 'Memory conflict row from the repository.' }); const HealthConfigResponseSchema = z.object({ @@ -374,8 +402,8 @@ const AuditTrailEntryResponseSchema = z.object({ contradiction_confidence: z.number().nullable(), previous_version_id: z.string().nullable(), superseded_by_version_id: z.string().nullable(), - valid_from: z.string(), - valid_to: z.string().nullable(), + valid_from: IsoDateString, + valid_to: IsoDateStringOrNull, memory_id: z.string().nullable(), }).openapi({ description: 'Single entry in a memory\'s audit trail.' });