From ef68c62d803f13d0f7714bd9af7080d266284cd8 Mon Sep 17 00:00:00 2001 From: Maurice Date: Sat, 10 Oct 2026 19:54:20 +0200 Subject: [PATCH] build(server): the response-contract exempt marker only excuses a handler that writes through @Res() A response-contract-exempt comment counted any handler as covered, whatever it returned, so one comment line let a JSON handler past the ratchet. The marker now counts only on a handler taking @Res() or @Next(). The five vacay holiday proxies return the provider's JSON, so they lose their markers and are counted again until they declare a schema. --- server/CLAUDE.md | 2 +- .../scripts/response-contract-baseline.json | 1 + server/scripts/response-contract-ratchet.mjs | 21 +++++++++++++++---- server/src/nest/vacay/vacay.controller.ts | 10 ++++----- .../scripts/response-contract-ratchet.test.ts | 17 +++++++++++++++ 5 files changed, 41 insertions(+), 10 deletions(-) diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 7a597df51..7750eeb2e 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -55,7 +55,7 @@ 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 every file under `src/nest` but the HTTP layer (controllers, guards, pipes, filters, interceptors, DTOs and `domain-error.ts` itself), the `status: 4xx`/`5xx` literals of a returned `{ error, status }` and every `new HttpException(`, and in a `*.controller.ts`, `*.mcp.ts` or `*.rpc.ts` every branch on a returned error (`if (result.error)`, `if (!r.error)`, `'error' in r`, `r.error === 'not_found'`, `r?.error && ...`, `r.error ? a : b`). 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. +**`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) has no return value to check and is marked with a `response-contract-exempt: ` comment above it instead; the marker excuses only such a handler, so one that returns a body, a third party's answer passed through included, stays counted until it declares a schema. 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"`. diff --git a/server/scripts/response-contract-baseline.json b/server/scripts/response-contract-baseline.json index dc871e33c..a90d780f5 100644 --- a/server/scripts/response-contract-baseline.json +++ b/server/scripts/response-contract-baseline.json @@ -96,6 +96,7 @@ "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": 5, "src/nest/weather/weather.controller.ts": 2 } } diff --git a/server/scripts/response-contract-ratchet.mjs b/server/scripts/response-contract-ratchet.mjs index 93f49cc45..dd2c57075 100644 --- a/server/scripts/response-contract-ratchet.mjs +++ b/server/scripts/response-contract-ratchet.mjs @@ -10,9 +10,11 @@ * * 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 + * covered when it also carries @ResponseContract(...), or when it writes + * through @Res() (or hands off with @Next()) and a comment above it reads + * `response-contract-exempt: `: a redirect or a stream has no return + * value to check. The marker on a handler that returns its body excuses + * nothing, so a comment cannot stand in for a schema. 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 @@ -60,6 +62,16 @@ function isExempt(text, method) { return head.split('\n').some((line) => /^\s*(\/\/|\/?\*)/.test(line) && line.includes(EXEMPT_MARKER)); } +/** Whether the handler takes the response (or next) object, so it answers through it rather than by returning. */ +function writesThroughResponse(method) { + return method.parameters.some((param) => + (ts.getDecorators(param) ?? []).some((d) => { + const name = decoratorName(d); + return name === 'Res' || name === 'Next' || name === 'Response'; + }), + ); +} + /** 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); @@ -69,7 +81,8 @@ export function handlersOf(text, fileName = 'file.ts') { 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) }); + const exempt = isExempt(text, node) && writesThroughResponse(node); + handlers.push({ name, covered: names.includes('ResponseContract') || exempt }); } } ts.forEachChild(node, visit); diff --git a/server/src/nest/vacay/vacay.controller.ts b/server/src/nest/vacay/vacay.controller.ts index c018d8473..0e1730a53 100644 --- a/server/src/nest/vacay/vacay.controller.ts +++ b/server/src/nest/vacay/vacay.controller.ts @@ -360,28 +360,28 @@ export class VacayController { return { success: true }; } - // response-contract-exempt: passes the holiday provider's answer through as it came; its shape is the provider's. + // No contract yet: passes the holiday provider's answer through as it came, so its shape is the provider's. @Get('holidays/countries') async holidayCountries() { const result = await this.vacay.getCountries(); return result.data; } - // response-contract-exempt: passes the holiday provider's answer through as it came; its shape is the provider's. + // No contract yet: passes the holiday provider's answer through as it came, so its shape is the provider's. @Get('holidays/:year/:country') async holidays(@Param('year') year: string, @Param('country') country: string) { const result = await this.vacay.getHolidays(year, country); return result.data; } - // response-contract-exempt: passes the holiday provider's answer through as it came; its shape is the provider's. + // No contract yet: passes the holiday provider's answer through as it came, so its shape is the provider's. @Get('school-holidays/regions/:country') async schoolHolidayRegions(@Param('country') country: string) { const result = await this.vacay.getSchoolHolidayRegions(country, country.toUpperCase() === 'DE' ? 'DE' : 'EN'); return result.data; } - // response-contract-exempt: passes the holiday provider's answer through as it came; its shape is the provider's. + // No contract yet: passes the holiday provider's answer through as it came, so its shape is the provider's. @Get('school-holidays/:year/:country') async schoolHolidaysForCountry( @Param('year') year: string, @@ -391,7 +391,7 @@ export class VacayController { return this.schoolHolidays(year, country, undefined, group); } - // response-contract-exempt: passes the holiday provider's answer through as it came; its shape is the provider's. + // No contract yet: passes the holiday provider's answer through as it came, so its shape is the provider's. @Get('school-holidays/:year/:country/:subdivision') async schoolHolidaysForSubdivision( @Param('year') year: string, diff --git a/server/tests/unit/scripts/response-contract-ratchet.test.ts b/server/tests/unit/scripts/response-contract-ratchet.test.ts index fb74b90d3..d392527da 100644 --- a/server/tests/unit/scripts/response-contract-ratchet.test.ts +++ b/server/tests/unit/scripts/response-contract-ratchet.test.ts @@ -68,6 +68,23 @@ describe('response-contract-ratchet.mjs', () => { expect(out).toContain('response-contracts: 2 of 2 route handler(s) covered (100.0 %)'); }); + it('RC-008: the exempt marker excuses only a handler that writes through @Res() or @Next()', () => { + const marked = (name: string, param: string) => + ` // response-contract-exempt: says so\n @Get('${name}')\n ${name}(${param}) {\n return {};\n }\n`; + const { status, out } = run( + serverRoot({ + [`${NEST}/x/x.controller.ts`]: controller( + marked('returnsBody', "@Param('id') id: string"), + marked('streams', '@Res() res: Response'), + marked('handsOff', '@Next() next: () => void'), + ), + }), + ); + expect(status).toBe(1); + expect(out).toContain(`FAIL ${NEST}/x/x.controller.ts: 1 route handler(s) without @ResponseContract`); + expect(out).toContain('2 of 3 route handler(s) covered'); + }); + 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) }));