From 87071bf7383cf6bf6502613732d9569a874891bc Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 10 Oct 2026 15:53:51 +0200 Subject: [PATCH] feat(server): @ResponseContract checks a handler's responses against its shared schema in tests The decorator records the @trek/shared response schema a route answers with. Under NODE_ENV=test it adds an interceptor that parses every response and fails the request with a logged 500 naming the handler and the issues, so the e2e and integration suites catch a response that drifted from its contract; in production nothing is added to the request path and the body is never rewritten. lint:response-contracts holds the handlers still without one to a per-file baseline that only shrinks, and runs in server-quality. --- .github/workflows/test.yml | 7 + CLAUDE.md | 3 +- server/CLAUDE.md | 4 + server/package.json | 1 + .../scripts/response-contract-baseline.json | 107 +++++++++ server/scripts/response-contract-ratchet.mjs | 204 ++++++++++++++++++ server/src/nest/README.md | 9 +- server/src/nest/common/response-contract.ts | 91 ++++++++ .../nest/common/response-contract.test.ts | 144 +++++++++++++ .../scripts/response-contract-ratchet.test.ts | 134 ++++++++++++ 10 files changed, 700 insertions(+), 4 deletions(-) create mode 100644 server/scripts/response-contract-baseline.json create mode 100644 server/scripts/response-contract-ratchet.mjs create mode 100644 server/src/nest/common/response-contract.ts create mode 100644 server/tests/unit/nest/common/response-contract.test.ts create mode 100644 server/tests/unit/scripts/response-contract-ratchet.test.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cab2dfd29..c1b4a957b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -329,6 +329,13 @@ jobs: # scripts/service-http-baseline.json. A service throws a DomainError. run: cd server && npm run lint:service-http + - name: Route handlers without a response contract may only shrink + # Counts the handlers under src/nest/ without @ResponseContract (or a + # response-contract-exempt comment) per file against + # scripts/response-contract-baseline.json. A covered handler's + # responses are parsed against its @trek/shared schema in every test. + run: cd server && npm run lint:response-contracts + - name: Open request schemas may only shrink # Needs the shared build above. Counts open shapes (records of # unknown, passthrough, any) in shared's request schemas. diff --git a/CLAUDE.md b/CLAUDE.md index 8f9bd8df4..9ed7832a4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,6 +55,7 @@ cd server && npm run lint:test-sql # raw SQL fixtures in tests/ cd server && npm run lint:query-api # MikroORM QueryBuilder calls in the repositories may only shrink (CI gate) cd server && npm run lint:mcp-zod # inline zod in the MCP tool files may only shrink; fields derive from @trek/shared (CI gate) cd server && npm run lint:service-http # { error, status } results and their caller-side branches may only shrink; services throw DomainError (CI gate) +cd server && npm run lint:response-contracts # route handlers without @ResponseContract may only shrink (CI gate) cd client && npm run theme:lint # flag styling that bypasses appearance tokens (local only) npm run typecheck --workspace=server # tsc --noEmit (also client/shared; server also has typecheck:tests) ``` @@ -129,7 +130,7 @@ These principles come out of a full-repo audit and shape all new code: ## Quality gates in CI -`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client and server through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:strict`, `lint:boundaries`, `lint:tx`, `lint:dialect`, `lint:test-sql`, `lint:query-api`, `lint:mcp-zod` and `lint:service-http`, the client's and server's `lint:format`, `lint:layers`, `lint:offline`, `lint:dup`, `lint:pairs`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `lint:format` on `shared/`, both check-only. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. +`.github/workflows/test.yml` runs, per package: typecheck (server also `typecheck:tests`), lint (client and server through `lint:warnings`, which also holds the warning count per rule), `lint:pages`, `lint:rtl`, `lint:size` (client and server), the server's `lint:strict`, `lint:boundaries`, `lint:tx`, `lint:dialect`, `lint:test-sql`, `lint:query-api`, `lint:mcp-zod`, `lint:service-http` and `lint:response-contracts`, the client's and server's `lint:format`, `lint:layers`, `lint:offline`, `lint:dup`, `lint:pairs`, `lint:skips` and `lint:i18n-keys`, shared's `contracts:open`, `check:plugin-facts`, `i18n:parity:strict` (with the untranslated-strings ratchet), the S3 contract suite, the Postgres probe (`postgres-probe`: every dialect helper on SQLite and Postgres, and the statements every repository method reaches with sample arguments against a Postgres service; the SQL errors per method and the list of methods it cannot measure are held to a baseline that only shrinks and fails while it is still unmeasured; see `server/CLAUDE.md`), the browser end-to-end flows (Chromium and WebKit), a client build with `check:gl-split`, coverage for all four packages, then a SonarCloud scan. It also builds the Docker image and boots it until `/api/health/ready` answers (`docker-image`), and lints and renders the Helm chart against the Kubernetes schemas and runs `docker compose config` on every compose file (`deploy-manifests`). `lint-prettier.yml` additionally runs `lint` + `lint:format` on `shared/`, both check-only. `plugin-sdk` has no lint script and is outside the eslint gates. **Not in CI**: `theme:lint` and the screenshot and help-media Playwright projects; run them locally. There are no git hooks; nothing runs before a commit except you. Other workflows build Docker images, scan them, and publish `trek-plugin-sdk` on `plugin-sdk-v*` tags. - **`ci-ok` is the required check.** The branch ruleset requires `ci-ok` (plus `lint` and `check-target` from their own workflows), and `ci-ok` needs every job above except the scan. It fails if any of them failed or was cancelled and passes if they passed or were skipped. A new gate job counts only once it is listed in `ci-ok`'s `needs:`. - **Guides name paths that exist**: the `changes` job runs `scripts/ci/doc-paths.mjs`, which resolves every backtick path in the CLAUDE.md files, `README.md`, `server/src/nest/README.md` and the wiki against the tracked files; the missing ones are listed in `scripts/ci/doc-paths-baseline.json`, which only shrinks (`--update` after a fix). diff --git a/server/CLAUDE.md b/server/CLAUDE.md index c75756709..7dda55b31 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -31,6 +31,7 @@ npm run lint:test-sql # raw better-sqlite3 statements in tests/ may only shr npm run lint:query-api # MikroORM QueryBuilder calls in the repositories may only shrink (CI gate) npm run lint:mcp-zod # inline z. calls per *.mcp.ts may only shrink; tool fields derive from @trek/shared (CI gate) npm run lint:service-http # error-envelope idioms (status: 4xx in services, if (result.error) in callers) may only shrink (CI gate) +npm run lint:response-contracts # route handlers without @ResponseContract may only shrink (CI gate) npm run contracts:dto-open # open shapes in the createZodDto schemas may only shrink (runs in the unit tests) ``` @@ -50,6 +51,8 @@ npm run contracts:dto-open # open shapes in the createZodDto schemas may only sh **`lint:mcp-zod`** (`scripts/mcp-inline-zod.mjs`) and **`lint:service-http`** (`scripts/service-http-ratchet.mjs`) share the per-file count rules of `scripts/lib/count-ratchet.mjs`: a file may hold at most its entry in its baseline (`scripts/mcp-inline-zod-baseline.json`, `scripts/service-http-baseline.json`), a file without one none, and an entry above what its file holds now fails until `-- --update` lowers it (never raises or adds). `lint:mcp-zod` counts every `z.` in a `*.mcp.ts`: a tool field derives from the request schema in `@trek/shared` or a shared primitive (`idSchema`); a bound REST deliberately lacks stays inline and counted. `lint:service-http` counts, in a `*.service.ts`, the `status: 4xx`/`5xx` literals of a returned `{ error, status }` and every `new HttpException(`, and in a `*.controller.ts` or `*.mcp.ts` every branch on a returned error (`if (result.error)`, `if ('error' in r)`). A service refuses by throwing a `DomainError` (`src/nest/common/domain-error.ts`) instead; `src/nest/README.md` ("Refusals") has the shape. +**`lint:response-contracts`** (`scripts/response-contract-ratchet.mjs`) reads every class under `src/nest/` with the TypeScript parser and counts, per file, the route handlers (`@Get`/`@Post`/`@Put`/`@Patch`/`@Delete`/`@All`/`@Head`/`@Options`) that carry no `@ResponseContract(...)` against `scripts/response-contract-baseline.json`: a file may hold at most its entry, a file without one none, and an entry above what its file holds now fails until `npm run lint:response-contracts -- --update` lowers it (it never raises or adds one). A handler that writes through `@Res()` (a stream, a redirect) or passes a third party's answer through unchanged has no body of TREK's to check and is marked with a `response-contract-exempt: ` comment above it instead. The run prints the covered share. + Single test: `npx vitest run tests/unit/nest/weather.controller.test.ts`, or `npx vitest run -t "returns 401 without cookie"`. **Build emits even with type errors** (`scripts/build.mjs` catches `tsc` failures and warns). `npm run build` succeeding does **not** mean the code typechecks — run `npm run typecheck`. @@ -117,6 +120,7 @@ SQLite is the engine today and Postgres is the planned second one. Code under `s ## Cross-cutting pieces - **`src/nest/common/`**: the global exception filter (error envelope), the Zod validation pipe, the idempotency interceptor (`X-Idempotency-Key` replay), and `validate-route-guards.ts` (boot-time ratchet refusing any public route not on its allow-list). +- **Response contracts** (`src/nest/common/response-contract.ts`): `@ResponseContract(schema)` names the `@trek/shared` response schema a handler answers with. Under `NODE_ENV=test` it adds an interceptor that parses every response of that handler against the schema and turns a body the schema does not describe into a logged 500 naming the handler and the first issues, so every e2e and integration test that reaches the route is a drift check; in production it only records the schema and adds nothing to the request path. The parse never replaces the body. When a check fails, fix the schema to describe what the route really sends, or the test stub to return what the service really returns, never the response. Rows keep their storage spelling in these schemas (0/1 flags, TEXT or epoch times) wherever the route has always answered that way. - **Auth is default-deny**: the global auth guard and the MFA policy guard run as `APP_GUARD`s, followed by the managed-install guard (`managed.guard.ts`), which refuses the routes a centrally administered install withholds from its own admin — it runs last so strangers still get a 401, never a route-revealing 403. Routes opt out with `@Public()`/`@OptionalAuth()`. The global guard stands down for routes declaring their own guard chain, so `@UseGuards(JwtAuthGuard, TripAccessGuard)` + `@RequirePermission` stacks work. Session is a cookie (`trek_session`); `@CurrentUser()` reads the user. Every session token carries a `jti` naming its `user_sessions` row, and `verifyJwtAndLoadUser` refuses one whose row is revoked, expired or gone (logout, `/api/auth/sessions`, a password change or reset, an MFA disable all end rows; `session-purge.job.ts` deletes the expired ones nightly; a revoked row waits for its own expiry, because for a session renewed from a pre-`jti` token, whose id is derived from that token, the revoked row is all that keeps the token from inserting it again). A token without a `jti` predates the table and passes until it expires, so an upgrade signs nobody out. The `token` field in the login-type JSON bodies and in the SSO exchange (`GET /api/auth/oidc/exchange`, which also answers `success: true`) is deprecated: kept for API clients, read by nothing of ours. **Never gate a multipart upload with a guard** — guards run before the parser and the client sees ECONNRESET instead of your 403; check in the handler. - **Logging**: `src/nest/audit/audit-log.logger.ts` (`logInfo`/`logWarn`/`logError`/`logDebug`) prints each line to the console and queues it for `data/logs/trek.log`, which `log-file.ts` appends in batches off the event loop and rotates at 10 MB over five files; `index.ts` flushes what is queued on exit. Never write the log file synchronously from a request path. - **Request correlation** (`src/nest/common/request-correlation.ts`): every HTTP request runs under an id (a well-formed incoming `X-Request-Id`, else a fresh UUID) that the response carries in `X-Request-Id`, and every log line written inside it is tagged `[http ]`. The exception filter leaves a 5xx on `res.locals` so the access log writes the request line and the stack as one entry. The other entry points (MCP tool/resource/prompt calls via the registry's `around` seam, WebSocket messages in `TrekWsAdapter`, plugin RPC dispatches in the supervisor, cron ticks in `CronRegistrarService`) each run under their own id through `traceEntry()` (`nest/audit/entry-trace.logger.ts`), which logs one line per call: debug on success, debug as `skipped` when the call declines its work on purpose (a cron tick whose lease another process holds), warn or error with the reason on failure. A new entry point goes through `traceEntry()` too. diff --git a/server/package.json b/server/package.json index dd5c7cd01..cf607c743 100644 --- a/server/package.json +++ b/server/package.json @@ -26,6 +26,7 @@ "lint:query-api": "node scripts/query-api-ratchet.mjs", "lint:mcp-zod": "node scripts/mcp-inline-zod.mjs", "lint:service-http": "node scripts/service-http-ratchet.mjs", + "lint:response-contracts": "node scripts/response-contract-ratchet.mjs", "contracts:dto-open": "node scripts/dto-open-shapes.mjs", "gen:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts", "check:plugin-facts": "node --import tsx scripts/gen-plugin-facts.ts --check", diff --git a/server/scripts/response-contract-baseline.json b/server/scripts/response-contract-baseline.json new file mode 100644 index 000000000..963276f85 --- /dev/null +++ b/server/scripts/response-contract-baseline.json @@ -0,0 +1,107 @@ +{ + "counts": { + "src/nest/accommodations/accommodations.controller.ts": 4, + "src/nest/addons/addons.controller.ts": 1, + "src/nest/admin/admin.controller.ts": 44, + "src/nest/airports/airports.controller.ts": 2, + "src/nest/assignments/assignments.controller.ts": 13, + "src/nest/atlas/atlas.controller.ts": 14, + "src/nest/atlas/public-stats.controller.ts": 1, + "src/nest/atlas/travel-stats.controller.ts": 1, + "src/nest/auth/auth-public.controller.ts": 9, + "src/nest/auth/auth.controller.ts": 24, + "src/nest/auth/passkey.controller.ts": 7, + "src/nest/auth/sessions.controller.ts": 3, + "src/nest/backup/backup.controller.ts": 8, + "src/nest/budget/budget.controller.ts": 16, + "src/nest/categories/categories.controller.ts": 4, + "src/nest/collab/collab.controller.ts": 20, + "src/nest/collections/collections.controller.ts": 40, + "src/nest/config/config.controller.ts": 1, + "src/nest/day-notes/day-notes.controller.ts": 4, + "src/nest/days/days.controller.ts": 6, + "src/nest/doc-sync/doc-sync-webhook.controller.ts": 1, + "src/nest/doc-sync/doc-sync.controller.ts": 15, + "src/nest/feeds/feeds.controller.ts": 10, + "src/nest/files/files-download.controller.ts": 1, + "src/nest/files/files.controller.ts": 11, + "src/nest/google-quota/google-quota.controller.ts": 2, + "src/nest/health/features.controller.ts": 3, + "src/nest/help/help.controller.ts": 4, + "src/nest/integrations/airtrail.controller.ts": 6, + "src/nest/integrations/dawarich.controller.ts": 15, + "src/nest/journey/journey-public.controller.ts": 3, + "src/nest/journey/journey.controller.ts": 40, + "src/nest/llm-parse/llm-capabilities.controller.ts": 1, + "src/nest/llm-parse/llm-local.controller.ts": 2, + "src/nest/maps/maps.controller.ts": 11, + "src/nest/mcp-transport/mcp-transport.controller.ts": 3, + "src/nest/memories/immich.controller.ts": 12, + "src/nest/memories/synology.controller.ts": 10, + "src/nest/memories/unified.controller.ts": 7, + "src/nest/notifications/notifications.controller.ts": 18, + "src/nest/notifications/push/push.controller.ts": 3, + "src/nest/oauth/oauth-api.controller.ts": 8, + "src/nest/oauth/oauth-public.controller.ts": 3, + "src/nest/oidc/oidc.controller.ts": 5, + "src/nest/packing/admin-packing-templates.controller.ts": 11, + "src/nest/packing/packing.controller.ts": 20, + "src/nest/photos/photos.controller.ts": 3, + "src/nest/place-enrichment/place-enrichment.controller.ts": 2, + "src/nest/place-shadow/place-shadow.controller.ts": 4, + "src/nest/places/places.controller.ts": 17, + "src/nest/platform/discovery.controller.ts": 9, + "src/nest/plugins/contributions/atlas-layers.controller.ts": 1, + "src/nest/plugins/contributions/day-schedule.controller.ts": 1, + "src/nest/plugins/contributions/day-tints.controller.ts": 1, + "src/nest/plugins/contributions/journal-entry-rows.controller.ts": 1, + "src/nest/plugins/contributions/map-layers.controller.ts": 1, + "src/nest/plugins/contributions/map-markers.controller.ts": 1, + "src/nest/plugins/contributions/pdf-sections.controller.ts": 1, + "src/nest/plugins/contributions/place-details.controller.ts": 1, + "src/nest/plugins/contributions/plugin-calendar.controller.ts": 1, + "src/nest/plugins/contributions/plugin-photos.controller.ts": 3, + "src/nest/plugins/contributions/plugin-pois.controller.ts": 1, + "src/nest/plugins/contributions/plugin-routes.controller.ts": 1, + "src/nest/plugins/contributions/plugin-search.controller.ts": 2, + "src/nest/plugins/contributions/trip-card-contributions.controller.ts": 1, + "src/nest/plugins/contributions/trip-warnings.controller.ts": 1, + "src/nest/plugins/contributions/view-contributions.controller.ts": 1, + "src/nest/plugins/oauth/plugin-oauth.controller.ts": 4, + "src/nest/plugins/plugin-activity.controller.ts": 1, + "src/nest/plugins/plugin-frame.controller.ts": 1, + "src/nest/plugins/plugin-user-settings.controller.ts": 3, + "src/nest/plugins/plugins-feed.controller.ts": 1, + "src/nest/plugins/plugins-proxy.controller.ts": 1, + "src/nest/plugins/plugins.controller.ts": 24, + "src/nest/public-api/public-api.controller.ts": 3, + "src/nest/receipt-scan/receipt-scan.controller.ts": 1, + "src/nest/reservation-import/reservation-import.controller.ts": 5, + "src/nest/reservations/reservations.controller.ts": 6, + "src/nest/reservations/upcoming-reservations.controller.ts": 1, + "src/nest/roadtrip/charging-lookup.controller.ts": 1, + "src/nest/roadtrip/charging.controller.ts": 1, + "src/nest/roadtrip/day-boundaries.controller.ts": 3, + "src/nest/roadtrip/google-route.controller.ts": 2, + "src/nest/roadtrip/roadtrip-hazards.controller.ts": 1, + "src/nest/roadtrip/roadtrip-preferences.controller.ts": 2, + "src/nest/roadtrip/roadtrip-search.controller.ts": 1, + "src/nest/roadtrip/roadtrip.controller.ts": 7, + "src/nest/route-usage/route-usage.controller.ts": 3, + "src/nest/school-holidays/school-holidays.controller.ts": 8, + "src/nest/settings/settings.controller.ts": 5, + "src/nest/share/share.controller.ts": 5, + "src/nest/storage/storage-admin.controller.ts": 8, + "src/nest/system-notices/system-notices.controller.ts": 2, + "src/nest/tags/tags.controller.ts": 4, + "src/nest/todo/todo.controller.ts": 7, + "src/nest/tours/tours-import.controller.ts": 1, + "src/nest/tours/tours.controller.ts": 4, + "src/nest/transit/transit.controller.ts": 2, + "src/nest/trip-invite/trip-invite.controller.ts": 5, + "src/nest/trip-members/trip-members.controller.ts": 7, + "src/nest/trips/trips.controller.ts": 12, + "src/nest/vacay/vacay.controller.ts": 33, + "src/nest/weather/weather.controller.ts": 2 + } +} diff --git a/server/scripts/response-contract-ratchet.mjs b/server/scripts/response-contract-ratchet.mjs new file mode 100644 index 000000000..93f49cc45 --- /dev/null +++ b/server/scripts/response-contract-ratchet.mjs @@ -0,0 +1,204 @@ +#!/usr/bin/env node +/* + * lint:response-contracts: route handlers without a @ResponseContract may only shrink. + * + * `@ResponseContract(schema)` (src/nest/common/response-contract.ts) names the + * @trek/shared schema a handler answers with, and under NODE_ENV=test every + * response is parsed against it, so the e2e suites catch a response that + * drifted from its contract. This holds the handlers still without one to + * what they are today. + * + * Every method under src/nest/ carrying a route decorator (@Get, @Post, @Put, + * @Patch, @Delete, @All, @Head, @Options) counts as a handler. One counts as + * covered when it also carries @ResponseContract(...), or when a comment + * above it reads `response-contract-exempt: ` (a handler that writes + * through @Res(), a redirect or a stream, has no return value to check). The + * uncovered handlers are counted per file against + * scripts/response-contract-baseline.json: a file may hold at most its entry, + * a file without one none. An entry above what its file holds now, or for a + * file that is gone, fails too until --update lowers it. + * + * npm run lint:response-contracts check against the baseline (CI) + * npm run lint:response-contracts -- --update lower the counts to what the files hold now; + * it never raises an entry and never adds one + * + * --dir= points the check at another server root (the unit tests use it). + */ +import { existsSync, readdirSync, readFileSync, realpathSync, statSync, writeFileSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import { join, relative, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const require = createRequire(import.meta.url); +const ts = require('typescript'); + +/** The tree the check walks, relative to the server root. */ +export const ROOT = 'src/nest'; + +/** The baseline, relative to the server root. */ +export const BASELINE = 'scripts/response-contract-baseline.json'; + +/** The decorators that make a method a route handler. */ +export const ROUTE_DECORATORS = new Set(['Get', 'Post', 'Put', 'Patch', 'Delete', 'All', 'Head', 'Options']); + +/** The comment that excuses a handler with no return value to check. */ +export const EXEMPT_MARKER = 'response-contract-exempt:'; + +const SOURCE = /\.ts$/; + +function decoratorName(decorator) { + const expr = decorator.expression; + const callee = ts.isCallExpression(expr) ? expr.expression : expr; + if (ts.isIdentifier(callee)) return callee.text; + if (ts.isPropertyAccessExpression(callee)) return callee.name.text; + return null; +} + +/** Whether a comment directly above the method (between its decorators or before them) exempts it. */ +function isExempt(text, method) { + const head = text.slice(method.getFullStart(), method.name.getStart()); + return head.split('\n').some((line) => /^\s*(\/\/|\/?\*)/.test(line) && line.includes(EXEMPT_MARKER)); +} + +/** The route handlers of one file's text: `{ name, covered }` in source order. */ +export function handlersOf(text, fileName = 'file.ts') { + const source = ts.createSourceFile(fileName, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + const handlers = []; + const visit = (node) => { + if (ts.isMethodDeclaration(node)) { + const names = (ts.getDecorators(node) ?? []).map(decoratorName); + if (names.some((n) => n && ROUTE_DECORATORS.has(n))) { + const name = node.name && ts.isIdentifier(node.name) ? node.name.text : (node.name?.getText(source) ?? '?'); + handlers.push({ name, covered: names.includes('ResponseContract') || isExempt(text, node) }); + } + } + ts.forEachChild(node, visit); + }; + visit(source); + return handlers; +} + +function walk(dir, files = []) { + for (const name of readdirSync(dir)) { + const path = join(dir, name); + if (statSync(path).isDirectory()) walk(path, files); + else if (SOURCE.test(name)) files.push(path); + } + return files; +} + +/** + * Handler totals and the uncovered counts keyed by the server-relative POSIX path + * (files with none left out of `uncovered`). + */ +export function scan(serverDir) { + const dir = join(serverDir, ROOT); + if (!existsSync(dir) || !statSync(dir).isDirectory()) { + throw new Error(`${ROOT}/ does not exist under ${serverDir}: the check would pass without looking at anything`); + } + const uncovered = {}; + let total = 0; + for (const path of walk(dir)) { + const file = relative(serverDir, path).split('\\').join('/'); + const handlers = handlersOf(readFileSync(path, 'utf8'), file); + total += handlers.length; + const n = handlers.filter((h) => !h.covered).length; + if (n > 0) uncovered[file] = n; + } + if (total === 0) + throw new Error(`no route handler found under ${ROOT}/: the check would pass without looking at anything`); + return { total, uncovered }; +} + +/** The baseline as committed; missing or malformed stops the run instead of reading as empty. */ +export function readBaseline(path) { + let parsed; + try { + parsed = JSON.parse(readFileSync(path, 'utf8')); + } catch (err) { + throw new Error(`${BASELINE} cannot be read: ${err.message}`); + } + const isObject = (v) => v !== null && typeof v === 'object' && !Array.isArray(v); + if (!isObject(parsed) || !isObject(parsed.counts)) { + throw new Error(`${BASELINE} must be an object with a "counts" object`); + } + for (const [file, n] of Object.entries(parsed.counts)) { + if (!Number.isInteger(n) || n <= 0) { + throw new Error(`${BASELINE}: ${file} holds ${JSON.stringify(n)}, expected a whole number above 0`); + } + } + return parsed; +} + +/** The counts lowered to what the files hold now. Never raises an entry, never adds one. */ +export function lowerCounts(baseline, counts) { + const lowered = {}; + for (const [file, allowed] of Object.entries(baseline.counts)) { + const now = counts[file] ?? 0; + if (now > 0) lowered[file] = Math.min(allowed, now); + } + return Object.fromEntries(Object.entries(lowered).sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0))); +} + +/** Files over their allowance, and entries above what their file holds now. */ +export function compare(baseline, counts) { + const grown = Object.entries(counts).filter(([file, n]) => n > (baseline.counts[file] ?? 0)); + const lowered = lowerCounts(baseline, counts); + const stale = Object.entries(baseline.counts).filter(([file, n]) => lowered[file] !== n); + return { grown, stale }; +} + +function main(argv) { + const dirArg = argv.find((a) => a.startsWith('--dir=')); + const serverDir = dirArg ? resolve(dirArg.slice('--dir='.length)) : fileURLToPath(new URL('..', import.meta.url)); + const baselinePath = join(serverDir, BASELINE); + + const { total, uncovered } = scan(serverDir); + const baseline = readBaseline(baselinePath); + if (argv.includes('--update')) { + baseline.counts = lowerCounts(baseline, uncovered); + writeFileSync(baselinePath, JSON.stringify(baseline, null, 2) + '\n'); + } + + const { grown, stale } = compare(baseline, uncovered); + for (const [file, n] of grown) { + const entry = baseline.counts[file]; + console.error( + `FAIL ${file}: ${n} route handler(s) without @ResponseContract, ` + + (entry ? `its baseline is ${entry}. ` : 'a file without a baseline entry may hold none. ') + + 'Declare the @trek/shared response schema the handler answers with (src/nest/common/response-contract.ts), ' + + `or mark a handler that writes through @Res() with a "${EXEMPT_MARKER} " comment.`, + ); + } + for (const [file, entry] of stale) { + console.error( + `FAIL ${file} is held at ${entry} in ${BASELINE}, ` + + (file in uncovered ? `but it has ${uncovered[file]} now.` : 'but it has none left (or the file is gone).'), + ); + } + if (stale.length) { + console.error( + 'Run npm run lint:response-contracts -- --update to lower the baseline with the change that made it smaller.', + ); + } + const open = Object.values(uncovered).reduce((sum, n) => sum + n, 0); + const share = ((100 * (total - open)) / total).toFixed(1); + console.log( + `response-contracts: ${total - open} of ${total} route handler(s) covered (${share} %), the rest held at their baseline`, + ); + return grown.length || stale.length ? 1 : 0; +} + +// Compared by real path, so the check still runs when the script is started through a symlink. +const isCli = + Boolean(process.argv[1]) && + existsSync(process.argv[1]) && + realpathSync(process.argv[1]) === fileURLToPath(import.meta.url); +if (isCli) { + try { + process.exitCode = main(process.argv.slice(2)); + } catch (err) { + console.error(`FAIL ${err.message}`); + process.exitCode = 1; + } +} diff --git a/server/src/nest/README.md b/server/src/nest/README.md index 217c4161b..b78735767 100644 --- a/server/src/nest/README.md +++ b/server/src/nest/README.md @@ -65,7 +65,9 @@ over an injected `AddonsService`. ## Cross-cutting pieces - `common/`: the exception filter, the Zod validation pipe, the idempotency - interceptor (`X-Idempotency-Key` replay), `validate-route-guards.ts`, and + interceptor (`X-Idempotency-Key` replay), `@ResponseContract` (the shared + response schema a handler answers with, checked in every test that reaches + it), `validate-route-guards.ts`, and stateless helpers (`avatarUrl`, `conflictResult`, `demo`, `passwordPolicy`, `timezoneService`, `cookie`, `rowShape`, `geo`, `crypto/`). The helpers are free functions on purpose: numbered migrations and `src/demo/demo-seed.ts` @@ -222,7 +224,8 @@ is measured, so moving code into this tree starts measuring it: run ## Definition of done (per module) -Contract in `@trek/shared` → service over injected repositories → controller -with the routes, statuses and bodies of the contract → the parallel MCP tool on +Contract in `@trek/shared` (request and response schemas) → service over +injected repositories → controller with the routes, statuses and bodies of the +contract, each handler declaring its `@ResponseContract` → the parallel MCP tool on the same service and permission → unit and e2e tests → module registered in `app.module.ts` → the client calls it through the typed contract. diff --git a/server/src/nest/common/response-contract.ts b/server/src/nest/common/response-contract.ts new file mode 100644 index 000000000..89c12ec31 --- /dev/null +++ b/server/src/nest/common/response-contract.ts @@ -0,0 +1,91 @@ +import { readEnv } from '../../app-config'; +import { + applyDecorators, + SetMetadata, + UseInterceptors, + type CallHandler, + type ExecutionContext, + type NestInterceptor, +} from '@nestjs/common'; + +import { map, type Observable } from 'rxjs'; +import type { ZodType } from 'zod'; + +/** The metadata key `@ResponseContract()` writes: the handler's response schema from @trek/shared. */ +export const RESPONSE_CONTRACT_KEY = 'trekResponseContract'; + +/** How many Zod issues a violation names; the first few are enough to find the drift. */ +const ISSUES_SHOWN = 5; + +/** + * A handler answered with a body its shared response schema does not describe. A plain + * error on purpose: the exception filter answers it with the generic 500 and logs it + * with its message (to the access log, or to the console in a hand-built test app), so + * a failing test prints which handler drifted and where. + */ +export class ResponseContractViolation extends Error { + override name = 'ResponseContractViolation'; +} + +/** + * Throws a ResponseContractViolation when `body` does not parse under `schema`. The body + * itself is never replaced by the parse result: the contract describes the wire, and a + * parse that strips or coerces must not change what the client receives. + */ +export function assertResponseContract(schema: ZodType, body: unknown, where: string): void { + const result = schema.safeParse(body); + if (result.success) return; + const issues = result.error.issues + .slice(0, ISSUES_SHOWN) + .map((issue) => `${issue.path.length ? issue.path.join('.') : '(root)'}: ${issue.message}`) + .join('; '); + throw new ResponseContractViolation(`${where} answered outside its response contract: ${issues}`); +} + +/** Parses every response of one handler against its schema (see ResponseContract). */ +export class ResponseContractInterceptor implements NestInterceptor { + constructor(private readonly schema: ZodType) {} + + intercept(context: ExecutionContext, next: CallHandler): Observable { + const where = `${context.getClass().name}.${context.getHandler().name}`; + return next.handle().pipe( + map((body: unknown) => { + assertResponseContract(this.schema, body, where); + return body; + }), + ); + } +} + +/** + * Whether the response contracts are checked: under NODE_ENV=test only. Decided once, + * when a controller class is defined, so a production build carries no interceptor at + * all on these routes rather than one that asks the environment on every request. The + * test setup sets NODE_ENV before any controller is imported. + */ +export function responseContractsEnforced(): boolean { + return readEnv().app.isTest; +} + +/** + * Declares the shape a handler answers with: a response schema from @trek/shared, the + * same one the client can type its call with. + * + * Under NODE_ENV=test every response of the handler is parsed against it, and a body + * the schema does not describe fails the request with a 500, logged with the handler and + * the first issues, so every e2e or integration test that reaches the route becomes a drift + * check. In production it only records the schema (the metadata the ratchet and the + * docs read) and adds nothing to the request path. + * + * Only for handlers whose return value is the JSON body. A handler that writes through + * `@Res()` (a stream, a redirect, a file) returns nothing to check and stays without + * one, marked `response-contract-exempt: ` in a comment above it. + * + * When a check fails, the schema is what gets fixed, to describe the response the route + * really sends: parity is law, the response does not change to suit the contract. + */ +export function ResponseContract(schema: ZodType): MethodDecorator { + const decorators: MethodDecorator[] = [SetMetadata(RESPONSE_CONTRACT_KEY, schema)]; + if (responseContractsEnforced()) decorators.push(UseInterceptors(new ResponseContractInterceptor(schema))); + return applyDecorators(...decorators); +} diff --git a/server/tests/unit/nest/common/response-contract.test.ts b/server/tests/unit/nest/common/response-contract.test.ts new file mode 100644 index 000000000..c9ac4ac4a --- /dev/null +++ b/server/tests/unit/nest/common/response-contract.test.ts @@ -0,0 +1,144 @@ +/** + * @ResponseContract (src/nest/common/response-contract.ts): under NODE_ENV=test every + * response of a decorated handler is parsed against its shared schema, and one the + * schema does not describe fails the request; the body itself is never rewritten. + */ +import { + RESPONSE_CONTRACT_KEY, + ResponseContract, + ResponseContractInterceptor, + ResponseContractViolation, + assertResponseContract, + responseContractsEnforced, +} from '../../../../src/nest/common/response-contract'; +import { TrekExceptionFilter } from '../../../../src/nest/common/trek-exception.filter'; +import { Controller, Get, Module, type CallHandler, type ExecutionContext } from '@nestjs/common'; +import { Test } from '@nestjs/testing'; + +import { lastValueFrom, of } from 'rxjs'; +import request from 'supertest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { z } from 'zod'; + +const itemSchema = z.object({ id: z.number(), name: z.string() }); + +function context(): ExecutionContext { + class ItemsController {} + function show() {} + return { getClass: () => ItemsController, getHandler: () => show } as unknown as ExecutionContext; +} + +const handled = (body: unknown): CallHandler => ({ handle: () => of(body) }); + +describe('assertResponseContract', () => { + it('passes a body the schema describes', () => { + expect(() => assertResponseContract(itemSchema, { id: 1, name: 'a' }, 'X.y')).not.toThrow(); + }); + + it('names the handler and the first issues of a body it does not describe', () => { + expect(() => assertResponseContract(itemSchema, { id: '1' }, 'ItemsController.show')).toThrow( + /^ItemsController\.show answered outside its response contract: id: .*; name: /, + ); + }); + + it('marks the root when the body itself is wrong', () => { + expect(() => assertResponseContract(itemSchema, undefined, 'X.y')).toThrow(/: \(root\): /); + }); + + it('names at most five issues', () => { + const wide = z.object(Object.fromEntries(Array.from({ length: 8 }, (_, i) => [`f${i}`, z.string()]))); + try { + assertResponseContract(wide, {}, 'X.y'); + expect.unreachable(); + } catch (e) { + expect(e).toBeInstanceOf(ResponseContractViolation); + expect((e as Error).message.split('; ')).toHaveLength(5); + } + }); +}); + +describe('ResponseContractInterceptor', () => { + it('hands the body on untouched, extra keys included', async () => { + const body = { id: 1, name: 'a', extra: true }; + const out = await lastValueFrom(new ResponseContractInterceptor(itemSchema).intercept(context(), handled(body))); + expect(out).toBe(body); + }); + + it('fails a body outside the schema with the handler named', async () => { + const run = lastValueFrom(new ResponseContractInterceptor(itemSchema).intercept(context(), handled({ id: 1 }))); + await expect(run).rejects.toThrow(/ItemsController\.show answered outside its response contract: name: /); + }); +}); + +describe('ResponseContract', () => { + afterEach(() => vi.unstubAllEnvs()); + + it('is enforced under NODE_ENV=test only', () => { + expect(responseContractsEnforced()).toBe(true); + vi.stubEnv('NODE_ENV', 'production'); + expect(responseContractsEnforced()).toBe(false); + }); + + it('records the schema on the handler', () => { + class C { + @ResponseContract(itemSchema) + show() { + return { id: 1, name: 'a' }; + } + } + expect(Reflect.getMetadata(RESPONSE_CONTRACT_KEY, C.prototype.show)).toBe(itemSchema); + expect(Reflect.getMetadata('__interceptors__', C.prototype.show)).toHaveLength(1); + }); + + it('adds no interceptor outside of tests, so production requests carry no check', () => { + vi.stubEnv('NODE_ENV', 'production'); + class C { + @ResponseContract(itemSchema) + show() { + return { id: 1 }; + } + } + expect(Reflect.getMetadata(RESPONSE_CONTRACT_KEY, C.prototype.show)).toBe(itemSchema); + expect(Reflect.getMetadata('__interceptors__', C.prototype.show)).toBeUndefined(); + }); + + it('answers a drifted response with the generic 500 and logs which handler drifted', async () => { + @Controller('items') + class ItemsController { + @Get('good') + @ResponseContract(itemSchema) + good() { + return { id: 1, name: 'a' }; + } + + @Get('bad') + @ResponseContract(itemSchema) + bad() { + return { id: 1, title: 'a' }; + } + } + @Module({ controllers: [ItemsController] }) + class ItemsModule {} + + const moduleRef = await Test.createTestingModule({ imports: [ItemsModule] }).compile(); + const app = moduleRef.createNestApplication(); + app.useGlobalFilters(new TrekExceptionFilter()); + await app.init(); + const logged = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + const good = await request(app.getHttpServer()).get('/items/good'); + expect(good.status).toBe(200); + expect(good.body).toEqual({ id: 1, name: 'a' }); + + const bad = await request(app.getHttpServer()).get('/items/bad'); + expect(bad.status).toBe(500); + expect(bad.body).toEqual({ error: 'Internal server error' }); + expect(String(logged.mock.calls[0]?.[1])).toMatch( + /ItemsController\.bad answered outside its response contract: name: /, + ); + } finally { + logged.mockRestore(); + await app.close(); + } + }); +}); diff --git a/server/tests/unit/scripts/response-contract-ratchet.test.ts b/server/tests/unit/scripts/response-contract-ratchet.test.ts new file mode 100644 index 000000000..fb74b90d3 --- /dev/null +++ b/server/tests/unit/scripts/response-contract-ratchet.test.ts @@ -0,0 +1,134 @@ +/** + * scripts/response-contract-ratchet.mjs holds the route handlers without a + * @ResponseContract to their baseline. It runs the real script against + * throwaway server roots (--dir) and once on this repository, so the committed + * baseline is checked by the unit suite too. + */ +import { execFileSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; + +const SCRIPT = path.join(__dirname, '../../../scripts/response-contract-ratchet.mjs'); +const SERVER_ROOT = path.join(__dirname, '../../..'); +const NEST = 'src/nest'; + +const controller = (...handlers: string[]) => + `@Controller('x')\nexport class XController {\n${handlers.join('\n')}\n}\n`; +const bare = (name: string) => ` @Get('${name}')\n ${name}() {\n return {};\n }\n`; +const contracted = (name: string) => + ` @Get('${name}')\n @ResponseContract(schema)\n async ${name}() {\n return {};\n }\n`; +const exempt = (name: string) => + ` // response-contract-exempt: streams through @Res()\n @Get('${name}')\n ${name}(@Res() res: Response) {\n res.end();\n }\n`; + +const roots: string[] = []; + +function serverRoot(files: Record, baseline: unknown = { counts: {} }): string { + const dir = mkdtempSync(path.join(tmpdir(), 'trek-response-contracts-')); + roots.push(dir); + mkdirSync(path.join(dir, NEST), { recursive: true }); + mkdirSync(path.join(dir, 'scripts'), { recursive: true }); + for (const [file, text] of Object.entries(files)) { + mkdirSync(path.dirname(path.join(dir, file)), { recursive: true }); + writeFileSync(path.join(dir, file), text); + } + if (baseline !== null) { + writeFileSync( + path.join(dir, 'scripts/response-contract-baseline.json'), + typeof baseline === 'string' ? baseline : JSON.stringify(baseline), + ); + } + return dir; +} + +function run(dir: string, ...args: string[]): { status: number | null; out: string } { + try { + const stdout = execFileSync(process.execPath, [SCRIPT, `--dir=${dir}`, ...args], { + encoding: 'utf8', + stdio: ['ignore', 'pipe', 'pipe'], + }); + return { status: 0, out: stdout }; + } catch (err) { + const { status, stdout, stderr } = err as { status: number | null; stdout: string; stderr: string }; + return { status, out: `${stdout}${stderr}` }; + } +} + +afterEach(() => { + for (const dir of roots.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe('response-contract-ratchet.mjs', () => { + it('RC-001: passes when every handler declares its contract or is exempt', () => { + const { status, out } = run( + serverRoot({ [`${NEST}/x/x.controller.ts`]: controller(contracted('a'), exempt('b')) }), + ); + expect(status).toBe(0); + expect(out).toContain('response-contracts: 2 of 2 route handler(s) covered (100.0 %)'); + }); + + it('RC-002: fails a new file holding a handler without one, and ignores plain methods', () => { + const helper = ' private helper() {\n return 1;\n }\n'; + const { status, out } = run(serverRoot({ [`${NEST}/x/x.controller.ts`]: controller(bare('a'), helper) })); + expect(status).toBe(1); + expect(out).toContain( + `FAIL ${NEST}/x/x.controller.ts: 1 route handler(s) without @ResponseContract, a file without a baseline entry may hold none.`, + ); + }); + + it('RC-003: holds a baselined file to its entry', () => { + const file = `${NEST}/x/x.controller.ts`; + const baseline = { counts: { [file]: 2 } }; + const grown = run(serverRoot({ [file]: controller(bare('a'), bare('b'), bare('c')) }, baseline)); + expect(grown.status).toBe(1); + expect(grown.out).toContain('its baseline is 2.'); + expect(run(serverRoot({ [file]: controller(bare('a'), bare('b'), contracted('c')) }, baseline)).status).toBe(0); + }); + + it('RC-004: counts every route decorator, whatever the verb', () => { + const verbs = ['Post', 'Put', 'Patch', 'Delete', 'All'].map( + (verb, i) => ` @${verb}('v${i}')\n v${i}() {\n return {};\n }\n`, + ); + const { out } = run(serverRoot({ [`${NEST}/x/x.controller.ts`]: controller(...verbs) })); + expect(out).toContain('0 of 5 route handler(s) covered'); + }); + + it('RC-005: an entry above its file fails until --update lowers it, and --update never raises or adds one', () => { + const a = `${NEST}/a/a.controller.ts`; + const b = `${NEST}/b/b.controller.ts`; + const dir = serverRoot( + { [a]: controller(bare('x')), [b]: controller(bare('x'), bare('y'), bare('z')) }, + { counts: { [a]: 3, [`${NEST}/gone/gone.controller.ts`]: 1, [b]: 2 } }, + ); + const stale = run(dir); + expect(stale.status).toBe(1); + expect(stale.out).toContain(`FAIL ${a} is held at 3`); + expect(stale.out).toContain(`FAIL ${NEST}/gone/gone.controller.ts is held at 1`); + + run(dir, '--update'); + const lowered = JSON.parse(readFileSync(path.join(dir, 'scripts/response-contract-baseline.json'), 'utf8')) as { + counts: Record; + }; + expect(lowered.counts).toEqual({ [a]: 1, [b]: 2 }); + }); + + it('RC-006: a missing or malformed baseline, a missing tree or a tree without handlers stops the run', () => { + const file = { [`${NEST}/x/x.controller.ts`]: controller(contracted('a')) }; + expect(run(serverRoot(file, null)).out).toContain('cannot be read'); + expect(run(serverRoot(file, '{"counts":{"x":0}}')).out).toContain('expected a whole number above 0'); + expect(run(serverRoot(file, '[]')).out).toContain('must be an object with a "counts" object'); + expect(run(serverRoot({ [`${NEST}/x/x.service.ts`]: 'export class X {}\n' })).out).toContain( + 'no route handler found', + ); + const empty = mkdtempSync(path.join(tmpdir(), 'trek-response-contracts-')); + roots.push(empty); + expect(run(empty).out).toContain('does not exist'); + }); + + it('RC-007: this repository holds its committed baseline', () => { + const { status, out } = run(SERVER_ROOT); + expect(out).toContain('response-contracts:'); + expect(status).toBe(0); + }); +});