From e29f568385d1935081d57ddd4a2de76090ddbca7 Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 00:41:23 +0800 Subject: [PATCH] refactor(types,rest,services,plugin-sharing): one shared writer for the response envelope, and error.code is enforced at compile time (#3973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `BaseResponseSchema` declares one envelope for every REST body the platform emits. It declared it once; the code that *wrote* it was copied per route module. After #3843 and #3983 converted the last drifting one, seven modules each carried their own two-line `sendOk` / `sendError` pair — the envelope's shape in fourteen places rather than one. `check:route-envelope` proved those copies agreed, which is why this is a cleanup rather than a bug fix. But a guard proves agreement; it does not create it. An eighth module starts by copying the pair again — not hypothetically: `share-link-routes.ts` was found already drifting by the repo-wide scan, and its drift had broken `client.shareLinks.create()` / `.list()` (#3983). Placement was #3973's open question, not design. `packages/spec` is schemas-only (Prime Directive #2), and the callers span `rest`, four `services/*` and one `plugins/*`. `@objectstack/types` depends on nothing but `@objectstack/spec`, so every caller can reach it, and it is already where the repo puts a helper the HTTP boundaries share — `looksLikeInternalErrorLeak` (#3867) sits one file over and made the same argument first. The builders take a structural `{ status(n), json(body) }`, so the package imports no HTTP contract at all. `error.code` is now checked by the compiler. All seven copies typed it `string`, so an invented code was caught only at runtime, by a conformance suite parsing a driven body — i.e. only on routes some test happened to drive. The shared `sendError` types it as ADR-0112's closed `ErrorCode` union, so an unregistered code fails to compile at every call site at once. This cost no call-site churn: every code the seven modules emit was already registered. Nothing changes on the wire. The seven pairs were identical modulo the optional `status` and `extra` parameters this one unions, and each module's driven conformance suite still parses its real bodies against the real spec schemas. One internal call site was rewritten: `package-routes` passed `details` positionally and now passes `{ details }`, producing the same `error.details`. The guard got stronger. A module routing everything through the shared pair builds none itself, so the seven now declare `0 / 0 / 0` where they declared `2 / 1 / 1`, and the pair is pinned separately at `2 / 1 / 1` so the invariant is total for the surface rather than per-module. What the count asserts is no longer "your two builders are the enveloped ones" but "you have no builders". Two out-of-scope findings filed rather than folded in (Prime Directive #10): #4224 (settings passes four keys `ApiErrorSchema` does not declare — the reason `extra` stays `Record`) and #4225 (three admin routes answer 503 naming the wrong service — carried out of #3973 so closing it does not bury the note). Co-Authored-By: Claude Opus 5 --- .../shared-response-envelope-builders.md | 87 +++++++++++ packages/plugins/plugin-sharing/package.json | 3 +- .../plugin-sharing/src/share-link-routes.ts | 68 +++----- .../rest/src/external-datasource-routes.ts | 55 +++---- packages/rest/src/package-routes.ts | 40 ++--- .../service-datasource/src/admin-routes.ts | 87 +++++------ packages/services/service-i18n/package.json | 3 +- .../service-i18n/src/i18n-service-plugin.ts | 44 +----- .../service-settings/src/settings-routes.ts | 34 +--- .../services/service-storage/package.json | 3 +- .../service-storage/src/storage-routes.ts | 49 +----- packages/types/src/index.ts | 1 + packages/types/src/response-envelope.test.ts | 123 +++++++++++++++ packages/types/src/response-envelope.ts | 134 ++++++++++++++++ pnpm-lock.yaml | 9 ++ scripts/check-route-envelope.mjs | 145 +++++++++++++++--- 16 files changed, 576 insertions(+), 309 deletions(-) create mode 100644 .changeset/shared-response-envelope-builders.md create mode 100644 packages/types/src/response-envelope.test.ts create mode 100644 packages/types/src/response-envelope.ts diff --git a/.changeset/shared-response-envelope-builders.md b/.changeset/shared-response-envelope-builders.md new file mode 100644 index 0000000000..0820a82b2a --- /dev/null +++ b/.changeset/shared-response-envelope-builders.md @@ -0,0 +1,87 @@ +--- +"@objectstack/types": minor +"@objectstack/rest": patch +"@objectstack/service-storage": patch +"@objectstack/service-settings": patch +"@objectstack/service-datasource": patch +"@objectstack/service-i18n": patch +"@objectstack/plugin-sharing": patch +--- + +refactor(types,rest,services,plugin-sharing): one shared writer for the response envelope, and `error.code` is enforced at compile time (#3973) + +`BaseResponseSchema` declares one envelope for every REST body the platform +emits. It declared it once; the code that *wrote* it was copied per route +module. After #3843 and #3983 converted the last drifting one, seven modules +each carried their own two-line `sendOk` / `sendError` pair — so the envelope's +shape lived in fourteen places rather than one. + +`pnpm check:route-envelope` proved those seven copies agreed, which is why this +is a cleanup rather than a bug fix. But a guard proves agreement; it does not +create it. An eighth module starts by copying the pair again — not +hypothetically: `share-link-routes.ts` was found already drifting by the +repo-wide scan, and its drift had broken `client.shareLinks.create()` and +`.list()` through `unwrapResponse` (#3983). + +## What moved + +`sendOk` / `sendError` now live once, in `@objectstack/types` +(`response-envelope.ts`), and all seven modules import them: + +| Module | +|---| +| `service-storage/storage-routes.ts` | +| `service-settings/settings-routes.ts` | +| `service-datasource/admin-routes.ts` | +| `rest/external-datasource-routes.ts` | +| `rest/package-routes.ts` | +| `service-i18n/i18n-service-plugin.ts` | +| `plugin-sharing/share-link-routes.ts` | + +Placement was the open question in #3973, not design. `packages/spec` is +schemas-only (Prime Directive #2), and the callers span `rest`, four +`services/*` and one `plugins/*`, which rules out anything depending on them. +`@objectstack/types` depends on nothing but `@objectstack/spec`, so every caller +can reach it, and it is already where the repo puts a helper the HTTP boundaries +share — `looksLikeInternalErrorLeak` (#3867) sits one file over and made the +same argument first. + +The builders take a structural `{ status(n), json(body) }`, so the package +imports no HTTP contract at all: `IHttpResponse` satisfies it, and so does the +`any`-typed `res` the older modules carry. + +## `error.code` is now checked by the compiler + +All seven copies typed the parameter `code: string`. ADR-0112 (#3841) closed the +vocabulary — `ErrorCode` is `StandardErrorCode ∪ ERROR_CODE_LEDGER` — but an +invented code was still caught only at runtime, by a conformance suite parsing a +driven body, i.e. only on routes some test happened to drive. + +The shared `sendError` types `code` as `ErrorCode`, so an unregistered code now +fails to compile, at every call site at once: + +```ts +sendError(res, 400, 'NOT_A_REGISTERED_CODE', 'invented'); +// Argument of type '"NOT_A_REGISTERED_CODE"' is not assignable to parameter of type 'ErrorCode'. +``` + +This cost no call-site churn: every code the seven modules emit was already +registered. + +## Nothing changes on the wire + +The seven pairs were identical modulo the optional `status` and `extra` +parameters this one unions, and each module's driven conformance suite still +parses its real bodies against the real spec schemas. One internal call site was +rewritten: `package-routes` passed `details` positionally and now passes +`{ details }`, producing the same `error.details` it always did. + +## The guard got stronger + +`scripts/check-route-envelope.mjs` counts response write sites per module. A +module that routes everything through the shared pair builds **none** itself, so +the seven now declare `0 / 0 / 0` where they used to declare `2 / 1 / 1`, and the +shared pair is pinned separately at `2 / 1 / 1` so the invariant stays total for +the surface rather than per-module. What the count asserts is no longer "your two +builders are the enveloped ones" but "you have no builders" — and a new route +that hand-rolls a body still moves it off zero and fails. diff --git a/packages/plugins/plugin-sharing/package.json b/packages/plugins/plugin-sharing/package.json index 01b89e7715..46f1677ff1 100644 --- a/packages/plugins/plugin-sharing/package.json +++ b/packages/plugins/plugin-sharing/package.json @@ -21,7 +21,8 @@ "@objectstack/formula": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/platform-objects": "workspace:*", - "@objectstack/spec": "workspace:*" + "@objectstack/spec": "workspace:*", + "@objectstack/types": "workspace:*" }, "devDependencies": { "@types/node": "^26.1.1", diff --git a/packages/plugins/plugin-sharing/src/share-link-routes.ts b/packages/plugins/plugin-sharing/src/share-link-routes.ts index 3f08ffdd48..946107720f 100644 --- a/packages/plugins/plugin-sharing/src/share-link-routes.ts +++ b/packages/plugins/plugin-sharing/src/share-link-routes.ts @@ -11,10 +11,11 @@ * GET /api/v1/share-links/:token/messages → conversation rows → data: ai_messages[] * * Every body is the declared `{ success: true, data }` / `{ success: false, - * error: { code, message } }` envelope, built in exactly two places — {@link sendOk} - * and {@link sendError}. Read `sendOk`'s note first: the shapes above are the - * dispatcher twin's, which this module converged onto in #3983, and the `success` - * flag they used to omit is why two `client.shareLinks.*` methods were broken here. + * error: { code, message } }` envelope, written by the shared `sendOk` / + * `sendError` (`@objectstack/types`). Read the note below on why `data` carries + * the payload bare: the shapes above are the dispatcher twin's, which this module + * converged onto in #3983, and the `success` flag they used to omit is why two + * `client.shareLinks.*` methods were broken here. * * The resolve route is intentionally public — it's the only endpoint * holders of a token need. It does: @@ -29,7 +30,9 @@ * and renders the response read-only. */ -import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import type { IHttpServer, IHttpRequest, RouteHandler } from '@objectstack/spec/contracts'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; import type { ShareLinkExecutionContext } from '@objectstack/spec/contracts'; import type { ShareLinkService } from './share-link-service.js'; import type { SharingEngine } from './sharing-service.js'; @@ -57,24 +60,9 @@ export interface ShareLinkRoutesOptions { const defaultContext = (_req: IHttpRequest): ShareLinkExecutionContext => ({}); /** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }`. + * ## Why `data` carries the payload bare on this module's five routes * - * The nested `{ code, message }` was already right (#3675's changeset cited this - * module as the good example of it); the `success` flag is what was missing. - * See {@link sendOk} for why that flag is load-bearing rather than decorative. - */ -function sendError(res: IHttpResponse, status: number, code: string, message: string) { - res.status(status).json({ success: false, error: { code, message } }); -} - -/** - * Emit a success body in the DECLARED envelope — `BaseResponseSchema` - * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`, - * with `data` carrying each route's payload DIRECTLY. - * - * ## This is a convergence, not a new dialect + * `sendOk(res, links)`, not `sendOk(res, { links })`. * * These five routes have a twin: `runtime/src/domains/share-links.ts` serves * the same paths off the dispatcher, and for cloud's per-environment kernels @@ -86,34 +74,22 @@ function sendError(res: IHttpResponse, status: number, code: string, message: st * routes, two shapes, decided by which surface happened to mount them — the * asymmetry #3636 fixed for `/i18n`, one domain over. * - * ## What that asymmetry actually broke - * - * Three of these routes are `disposition: 'sdk'` in `runtime/src/route-ledger.ts` - * (`shareLinks.create` / `.list` / `.revoke`), and `ObjectStackClient.unwrapResponse` - * keys on a boolean `success`. With no flag it returns the body verbatim, so - * against THIS surface: - * - * • `shareLinks.create()` — documented "Returns the link row (incl. `token`)" - * — handed back `{ link: … }`, making `.token` `undefined`. - * • `shareLinks.list()` — typed `Promise` — handed back `{ links: [] }`, - * so any `.map()` on it threw. - * + * That was not cosmetics. Three of these routes are `disposition: 'sdk'` in + * `runtime/src/route-ledger.ts` (`shareLinks.create` / `.list` / `.revoke`), and + * `ObjectStackClient.unwrapResponse` keys on a boolean `success`. With no flag + * it returns the body verbatim, so against THIS surface `shareLinks.create()` + * handed back `{ link: … }` (making the documented `.token` `undefined`) and + * `shareLinks.list()` handed back `{ links: [] }`, so any `.map()` on it threw. * `packages/client/src/admin-surfaces.test.ts` mocks all three as * `{ success: true, data: }`: the SDK was written and tested against - * the dispatcher's shape. So this is not envelope cosmetics — it is why two SDK - * methods did not work on the plugin surface at all. + * the dispatcher's shape (#3983). * - * ## Why `data` carries the payload bare - * - * `sendOk(res, links)`, not `sendOk(res, { links })`. That is what makes - * `unwrapResponse` return the same value on both surfaces, and it is what the - * consumers already read (`body.links ?? body.data`, `created.link ?? created.data`, - * `body?.data ?? []`) — they carry that tolerance precisely BECAUSE both shapes - * exist in the fleet. Prime Directive #12: the shim goes once the producer agrees. + * So passing the payload bare is what makes `unwrapResponse` return the same + * value on both surfaces, and it is what the consumers already read + * (`body.links ?? body.data`, `created.link ?? created.data`, `body?.data ?? []`) + * — they carry that tolerance precisely BECAUSE both shapes existed in the + * fleet. Prime Directive #12: the shim goes once the producer agrees. */ -function sendOk(res: IHttpResponse, data: unknown, status = 200): void { - res.status(status).json({ success: true, data }); -} /** Strip `redactFields` from a record (also removes from nested arrays of objects). */ function applyRedaction(record: any, redactFields: string[]): any { diff --git a/packages/rest/src/external-datasource-routes.ts b/packages/rest/src/external-datasource-routes.ts index f22b40a903..fe102a3768 100644 --- a/packages/rest/src/external-datasource-routes.ts +++ b/packages/rest/src/external-datasource-routes.ts @@ -2,6 +2,8 @@ import type { PluginContext } from '@objectstack/core'; import type { IHttpServer } from '@objectstack/spec/contracts'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; /** * External Datasource Federation REST routes (ADR-0015 §6.2). @@ -22,8 +24,25 @@ import type { IHttpServer } from '@objectstack/spec/contracts'; * into the private `@objectstack/datasource-admin` package, which registers * them via its own `registerDatasourceAdminRoutes`. * - * Every body is built by `sendOk` / `sendError` below, in the envelope - * `BaseResponseSchema` declares (#3843). + * Every body is built by the shared `sendOk` / `sendError`, in the envelope + * `BaseResponseSchema` declares (#3843, consolidated in #3973). Before #3843 + * this module emitted the pre-#3675 `{ error: '' }`, with the message a + * SIBLING of `error` on the import path — so a caller reading + * `body.error.message` got `undefined`. + * + * Codes follow ADR-0112 (#3841, settled while #3843 was in review): + * `external_service_unavailable` → the standard `SERVICE_UNAVAILABLE`, since the + * ledger asks generic conditions to reuse the catalog rather than register a + * per-service synonym; `external_import_error` → `EXTERNAL_IMPORT_ERROR`, + * registered under this package because a refused federated import is specific + * to it. + * + * Note `POST /validate` keeps its `ok` — unlike the `{ ok: true, key }` #3689 + * retired from storage, that one was a private second word for `success`, while + * this `ok` is a COMPUTED verdict over the federated objects + * (`results.every(r => r.ok)`). A domain verdict that happens to share the name + * is not a second envelope flag, so it belongs inside `data` rather than being + * dropped. */ export function registerExternalDatasourceRoutes( server: IHttpServer, @@ -40,38 +59,6 @@ export function registerExternalDatasourceRoutes( } }; - /** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }`. - * - * Before #3843 this module emitted the pre-#3675 `{ error: '' }`, - * with the message a SIBLING of `error` on the import path — so a caller - * reading `body.error.message` got `undefined`. - * - * Codes follow ADR-0112 (#3841, settled while this was in review): - * `external_service_unavailable` → the standard `SERVICE_UNAVAILABLE`, since the - * ledger asks generic conditions to reuse the catalog rather than register a - * per-service synonym; `external_import_error` → `EXTERNAL_IMPORT_ERROR`, - * registered under this package because a refused federated import is specific - * to it. - */ - const sendError = (res: any, status: number, code: string, message: string) => - res.status(status).json({ success: false, error: { code, message } }); - - /** - * Emit a success body in the DECLARED envelope — `{ success: true, data }`. - * - * Payload keys are unchanged, one level deeper. Note `POST /validate` keeps - * its `ok` — unlike the `{ ok: true, key }` #3689 retired from storage, that - * one was a private second word for `success`, while this `ok` is a COMPUTED - * verdict over the federated objects (`results.every(r => r.ok)`). A domain - * verdict that happens to share the name is not a second envelope flag, so it - * belongs inside `data` rather than being dropped. - */ - const sendOk = (res: any, data: unknown, status = 200) => - res.status(status).json({ success: true, data }); - const unavailable = (res: any) => sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The external-datasource service is not available.'); diff --git a/packages/rest/src/package-routes.ts b/packages/rest/src/package-routes.ts index 02f90a5a17..9e85dcb5a5 100644 --- a/packages/rest/src/package-routes.ts +++ b/packages/rest/src/package-routes.ts @@ -2,6 +2,8 @@ import { IHttpServer } from '@objectstack/core'; import type { PackageService } from '@objectstack/service-package'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; /** * Options for package route registration. @@ -44,13 +46,10 @@ export interface PackageRoutesOptions { * swallowed every `client.packages.install` call with a 400. The * dispatcher's own `POST /packages/:id/publish` (ADR-0033 draft publish) * is two segments — different shape, no clash. - */ -/** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }`. * - * This module was the *partially* converted one when #3843 was filed, which is + * ## Where this module's error codes came from + * + * This was the *partially* converted module when #3843 was filed, which is * arguably worse than untouched: 3 of its 16 bodies carried `success: true`, so * the same registrar answered two shapes depending on which route you hit. Its * error bodies were the pre-#3675 `{ error: '' }` throughout — and the @@ -63,8 +62,10 @@ export interface PackageRoutesOptions { * * Because there were no codes here to preserve, these had to be MINTED. They * follow ADR-0112 (#3841, settled while this was in review): SCREAMING_SNAKE, and - * registered in `ERROR_CODE_LEDGER` under `@objectstack/rest` — an unregistered - * code fails `ApiErrorSchema` parse, which fails the conformance suite. + * registered in `ERROR_CODE_LEDGER` under `@objectstack/rest`. That union is now + * the `code` parameter's TYPE — `sendError` takes `ErrorCode`, not `string` + * (#3973) — so an unregistered code fails to compile rather than waiting for a + * conformance suite to parse a driven body. * * Generic conditions reuse the STANDARD catalog rather than becoming registered * synonyms of it: a missing request field is `MISSING_REQUIRED_FIELD`, an absent @@ -72,27 +73,6 @@ export interface PackageRoutesOptions { * the package-specific outcomes are registered — `PACKAGE_MANIFEST_INVALID`, * `PACKAGE_PUBLISH_FAILED`, `PACKAGE_DELETE_PARTIAL`, `PACKAGE_DELETE_FAILED`. */ -function sendError(res: any, status: number, code: string, message: string, details?: unknown) { - res.status(status).json({ - success: false, - error: { code, message, ...(details === undefined ? {} : { details }) }, - }); -} - -/** - * Emit a success body in the DECLARED envelope — `{ success: true, data }`. - * - * The three bodies that already carried `success: true` kept their payload as - * SIBLINGS of the flag (`{ success: true, message, package }`); those move under - * `data` so the envelope has one payload slot rather than a spread. `packages` - * SDK methods read these through `unwrapResponse`, which returns `body.data` - * when the flag is present, so `packages.list()` still resolves to - * `{ packages, total }`. - */ -function sendOk(res: any, data: unknown, status = 200) { - res.status(status).json({ success: true, data }); -} - export function registerPackageRoutes( server: IHttpServer, packageService: PackageService, @@ -257,7 +237,7 @@ export function registerPackageRoutes( 400, 'PACKAGE_DELETE_PARTIAL', `Deleting ${packageId} left ${result.failedCount} item(s) behind.`, - { failed: result.failed, cleanups: result.cleanups }, + { details: { failed: result.failed, cleanups: result.cleanups } }, ); return; } diff --git a/packages/services/service-datasource/src/admin-routes.ts b/packages/services/service-datasource/src/admin-routes.ts index 958de9b32c..24975909bd 100644 --- a/packages/services/service-datasource/src/admin-routes.ts +++ b/packages/services/service-datasource/src/admin-routes.ts @@ -2,6 +2,8 @@ import type { PluginContext } from '@objectstack/core'; import type { IHttpServer } from '@objectstack/spec/contracts'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; import { DRIVER_CATALOG } from './driver-catalog.js'; /** @@ -22,9 +24,40 @@ import { DRIVER_CATALOG } from './driver-catalog.js'; * `secret` field; the route splits `secret` out so it never reaches the draft * the service persists. * - * Every body — both halves — is built by `sendOk` / `sendError` below, in the - * envelope `BaseResponseSchema` declares. See those two for what this module - * emitted before #3843 and why it was the worse of the two drifting dialects. + * Every body — both halves — is built by the shared `sendOk` / `sendError`, in + * the envelope `BaseResponseSchema` declares (#3843, consolidated in #3973). + * + * ## What this module emitted before #3843 + * + * `{ error: '' }` — the shape #3675 had already declared wrong for + * `service-storage`, with `message` a SIBLING of `error` rather than a field + * of it: + * + * res.status(400).json({ error: 'datasource_admin_error', message }); + * + * so a caller reading `body.error.message` got `undefined` here and the real + * message from the dispatcher — the identical asymmetry #3675 opened on, one + * layer over. `ObjectStackClient` sniffs several shapes to paper over the + * difference; that shim is the consumer-side symptom Prime Directive #12 says + * to cure at the producer. + * + * The codes follow ADR-0112, which #3841 settled while #3843 was in review: + * `error.code` is SCREAMING_SNAKE and `ApiErrorSchema.code` is the closed + * `ErrorCode` union — which is now also `sendError`'s parameter type, so an + * unregistered code fails to COMPILE rather than failing a schema parse at + * runtime (#3973). The old lowercase trio was re-spelled accordingly, and the + * generic conditions went to the STANDARD catalog rather than becoming + * registered synonyms of it: + * + * datasource_admin_unavailable → SERVICE_UNAVAILABLE (standard) + * not_found → RESOURCE_NOT_FOUND (standard) + * datasource_admin_error → DATASOURCE_ADMIN_ERROR (registered — a + * lifecycle/validation refusal specific to + * this service, so not a standard synonym) + * + * Which service is unavailable is carried by `message`; the ledger explicitly + * asks generic conditions to reuse the catalog instead of registering a + * per-service 503. */ export function registerDatasourceAdminRoutes( server: IHttpServer, @@ -49,54 +82,6 @@ export function registerDatasourceAdminRoutes( } }; - /** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }`. - * - * Before #3843 this module emitted `{ error: '' }` — the shape #3675 - * had already declared wrong for `service-storage`, with `message` a SIBLING - * of `error` rather than a field of it: - * - * res.status(400).json({ error: 'datasource_admin_error', message }); - * - * so a caller reading `body.error.message` got `undefined` here and the real - * message from the dispatcher — the identical asymmetry #3675 opened on, one - * layer over. `ObjectStackClient` sniffs several shapes to paper over the - * difference; that shim is the consumer-side symptom Prime Directive #12 says - * to cure at the producer. - * - * The codes follow ADR-0112, which #3841 settled while this was in review: - * `error.code` is SCREAMING_SNAKE and `ApiErrorSchema.code` is now the closed - * `ErrorCode` union, so an unregistered code fails schema parse. The old - * lowercase trio was re-spelled accordingly, and the generic conditions went to - * the STANDARD catalog rather than becoming registered synonyms of it: - * - * datasource_admin_unavailable → SERVICE_UNAVAILABLE (standard) - * not_found → RESOURCE_NOT_FOUND (standard) - * datasource_admin_error → DATASOURCE_ADMIN_ERROR (registered — a - * lifecycle/validation refusal specific to - * this service, so not a standard synonym) - * - * Which service is unavailable is carried by `message`; the ledger explicitly - * asks generic conditions to reuse the catalog instead of registering a - * per-service 503. - */ - const sendError = (res: any, status: number, code: string, message: string) => - res.status(status).json({ success: false, error: { code, message } }); - - /** - * Emit a success body in the DECLARED envelope — `{ success: true, data }`. - * - * The payload keys are unchanged, just one level deeper: `{ datasources }` - * becomes `{ success: true, data: { datasources } }`. `ObjectStackClient` - * reads these through `unwrapResponse`, which returns `body.data` when the - * flag is present, so the SDK's published return types describe the same - * object they always did. - */ - const sendOk = (res: any, data: unknown, status = 200) => - res.status(status).json({ success: true, data }); - const unavailable = (res: any) => sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.'); diff --git a/packages/services/service-i18n/package.json b/packages/services/service-i18n/package.json index 4cf7da2f8c..3796e7931f 100644 --- a/packages/services/service-i18n/package.json +++ b/packages/services/service-i18n/package.json @@ -19,7 +19,8 @@ }, "dependencies": { "@objectstack/core": "workspace:*", - "@objectstack/spec": "workspace:*" + "@objectstack/spec": "workspace:*", + "@objectstack/types": "workspace:*" }, "devDependencies": { "@types/node": "^26.1.1", diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index 9b95e34fd5..9a7976e293 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -6,51 +6,11 @@ import type { IHttpServer, IHttpRequest, IHttpResponse } from '@objectstack/spec import type { II18nService } from '@objectstack/spec/contracts'; import type { TranslationData } from '@objectstack/spec/system'; import { resolveObjectFieldLabels, toLocaleDescriptors } from '@objectstack/spec/system'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; import { FileI18nAdapter } from './file-i18n-adapter.js'; import type { FileI18nAdapterOptions } from './file-i18n-adapter.js'; -/** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }` with `code` a semantic - * STRING. - * - * These routes used to emit a bare `{ error: '' }`, so the same SDK - * method handed callers a string against this provider and an object against - * the dispatcher's `/i18n` domain — the error-path twin of the success-path - * asymmetry #3636 fixed (#3675). `ObjectStackClient` tolerates both shapes by - * sniffing, which is the consumer-side shim Prime Directive #12 says to - * delete by fixing the producer. - * - * Shape borrowed from the sibling services that already nest the error object - * (`settings-routes.ts`, `share-link-routes.ts`); the `success` flag is what - * those two still omit and the contract requires. - */ -function sendError(res: IHttpResponse, status: number, code: string, message: string): void { - res.status(status).json({ success: false, error: { code, message } }); -} - -/** - * Emit a success body in the DECLARED envelope — `BaseResponseSchema` - * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`. - * - * #3636 put the right shape on all three read routes, but built it inline in each - * one — four call sites for one envelope half, while the error half had already - * been consolidated behind `sendError` (#3675). Those bodies were never wrong; - * this is about the GUARD, not the wire. - * - * `scripts/check-route-envelope.mjs` counts response write sites per module, so a - * consolidated module sits at a fixed two no matter how many routes it grows. This - * one sat at five with a declared ratchet (#3973) precisely because a fifth read - * route could have hand-rolled a fourth-dialect body and only a driven test would - * have caught it — and a driven test only covers the routes that existed when it - * was written. Collapsing the four inline builders into this one retires that - * ratchet: the module now declares the same `2 / 1 / 1` as the other five. - */ -function sendOk(res: IHttpResponse, data: unknown): void { - res.json({ success: true, data }); -} - /** * Configuration options for the I18nServicePlugin. */ diff --git a/packages/services/service-settings/src/settings-routes.ts b/packages/services/service-settings/src/settings-routes.ts index 2209f54b29..365466f41d 100644 --- a/packages/services/service-settings/src/settings-routes.ts +++ b/packages/services/service-settings/src/settings-routes.ts @@ -13,7 +13,9 @@ * `SettingsService`. */ -import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import type { IHttpServer, IHttpRequest, RouteHandler } from '@objectstack/spec/contracts'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; import { SettingsService } from './settings-service.js'; import { SettingsForbiddenError, @@ -46,36 +48,6 @@ export interface SettingsRoutesOptions { // wire a verified `contextFromRequest` (the plugin does). const defaultContext = (_req: IHttpRequest): SettingsContext => ({ enforced: true }); -/** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }`. - * - * This module was already half-right before #3843: `error` was correctly a - * nested `{ code, message }` object, but no body carried the `success` flag the - * envelope declares — so `BaseResponseSchema.safeParse` failed on every - * response, success and error alike, and a caller keying on `success` (as - * `ObjectStackClient.unwrapResponse` does) could not tell these routes' bodies - * apart from an already-unwrapped payload. - */ -function sendError(res: IHttpResponse, status: number, code: string, message: string, extra?: Record) { - res.status(status).json({ success: false, error: { code, message, ...extra } }); -} - -/** - * Emit a success body in the DECLARED envelope — `{ success: true, data }`. - * - * The three success bodies keep their payload keys, one level deeper: - * `{ manifests }` → `{ success: true, data: { manifests } }`. The - * `GET /:namespace` payload is `SettingsNamespacePayloadSchema` - * (`{ manifest, values }`) and moves under `data` whole, so that schema still - * describes exactly what the route returns — now as the envelope's `data` - * rather than as the entire body. - */ -function sendOk(res: IHttpResponse, data: unknown, status = 200) { - res.status(status).json({ success: true, data }); -} - export function registerSettingsRoutes( http: IHttpServer, service: SettingsService, diff --git a/packages/services/service-storage/package.json b/packages/services/service-storage/package.json index d39bb1af57..099c6fda3e 100644 --- a/packages/services/service-storage/package.json +++ b/packages/services/service-storage/package.json @@ -21,7 +21,8 @@ "@objectstack/core": "workspace:*", "@objectstack/observability": "workspace:*", "@objectstack/platform-objects": "workspace:*", - "@objectstack/spec": "workspace:*" + "@objectstack/spec": "workspace:*", + "@objectstack/types": "workspace:*" }, "peerDependencies": { "@aws-sdk/client-s3": "^3.0.0", diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index 22f891edc0..9edea6eaf9 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -2,6 +2,8 @@ import { randomUUID } from 'node:crypto'; import type { IHttpServer, IHttpRequest, IHttpResponse, IStorageService } from '@objectstack/spec/contracts'; +// The declared envelope is written in ONE place for the whole platform (#3973). +import { sendOk, sendError } from '@objectstack/types'; import type { StorageMetadataStore, FileRecord } from './metadata-store.js'; import type { LocalStorageAdapter } from './local-storage-adapter.js'; import { contentDispositionValue } from './content-disposition.js'; @@ -9,53 +11,6 @@ import { contentDispositionValue } from './content-disposition.js'; /** Authorization verdict for an attachments-scope download (#2970 item 2). */ export type FileReadVerdict = 'allow' | 'deny' | 'unauthenticated'; -/** - * Emit an error in the DECLARED envelope — `BaseResponseSchema` + - * `ApiErrorSchema` (`packages/spec/src/api/contract.zod.ts`), i.e. - * `{ success: false, error: { code, message } }` with `code` a semantic - * STRING. - * - * These routes used to emit a bare `{ error: '' }`, with the code — - * where one existed at all — as a SIBLING of `error` rather than a field of - * it. So a caller reading `body.error.message` got `undefined` here and the - * real message from the dispatcher (#3675). `ObjectStackClient` papers over - * the difference by sniffing four shapes; that shim is the consumer-side - * symptom Prime Directive #12 says to cure at the producer. - */ -function sendError(res: IHttpResponse, status: number, code: string, message: string): void { - res.status(status).json({ success: false, error: { code, message } }); -} - -/** - * Emit a success body in the DECLARED envelope — `BaseResponseSchema` - * (`packages/spec/src/api/contract.zod.ts`), i.e. `{ success: true, data }`. - * That is what `storage.zod.ts` has declared for these routes all along, and - * what `ObjectStackClient.unwrapResponse` keys on. - * - * #3675 normalized the error path and deliberately left this one alone, - * because unlike the errors it is not additive. The success bodies were three - * shapes, none carrying `success` (#3689): - * - * - the six upload routes: `{ data: {…} }` — flag missing, payload already in place - * - `GET /files/:fileId/url`: `{ url }` — no envelope at all - * - `PUT /_local/raw/:token`: `{ ok: true, key }` — a second, private word for `success` - * - * so the last two MOVE their payload under `data`. The consumers were taught - * both shapes first — `storage.getDownloadUrl` reads through the SDK's - * standard `unwrapResponse`, the console's two attachment openers already read - * `body?.url ?? body?.data?.url` — so the repos are not coupled by merge - * order, the same way the error path was landed. `ok` is dropped rather than - * kept beside `success`: one envelope, one word for it. - * - * Every success body in this module goes through here, so the shape lives in - * exactly one line. `success-envelope.conformance.test.ts` drives each route, - * parses the result against the real spec schemas, and scans this source so a - * new route cannot quietly reintroduce a bare one. - */ -function sendOk(res: IHttpResponse, data: unknown): void { - res.json({ success: true, data }); -} - /** * Options for the storage route registration helper. */ diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index 5274def0ce..6904af9903 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -4,6 +4,7 @@ export * from './degraded-boot.js'; export * from './env.js'; export * from './error-leak.js'; export * from './module-not-found.js'; +export * from './response-envelope.js'; // Placeholder for Kernel interface to avoid circular dependency // The actual Kernel implementation will satisfy this interface. diff --git a/packages/types/src/response-envelope.test.ts b/packages/types/src/response-envelope.test.ts new file mode 100644 index 0000000000..8e6c17a8a4 --- /dev/null +++ b/packages/types/src/response-envelope.test.ts @@ -0,0 +1,123 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3973 — the shared envelope builders. + * + * Seven route modules used to carry their own `sendOk` / `sendError` pair. Now + * they call these, so this is the one place the declared envelope gets written + * and the only place a mistake in it can originate. + * + * Two things are pinned here that the per-module driven suites cannot pin as + * cheaply: + * + * 1. the bodies parse against the REAL spec schemas — `BaseResponseSchema` and + * `envelopeViolations`, imported rather than restated, so these assertions + * track the contract if the contract moves; + * 2. the parameter plumbing each module relies on — a non-200 success status, + * `extra` merged into `error` — because a module that lost one of those in + * the conversion would otherwise only fail in its own suite, if at all. + * + * The STATIC half — proving no route module hand-builds a body instead of + * calling these — is `scripts/check-route-envelope.mjs`, repo-wide and outside + * any package on purpose. + */ + +import { describe, it, expect } from 'vitest'; +import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api'; +import { sendOk, sendError, type EnvelopeResponse } from './response-envelope.js'; + +/** Captures what a route would have put on the wire. */ +function capture() { + const seen: { status: number; body: unknown } = { status: 0, body: undefined }; + const res: EnvelopeResponse = { + status(code: number) { seen.status = code; return res; }, + json(body: unknown) { seen.body = body; return undefined; }, + }; + return { res, seen }; +} + +describe('sendOk', () => { + it('emits the declared success envelope with the payload under `data`', () => { + const { res, seen } = capture(); + sendOk(res, { locales: ['en', 'zh-CN'] }); + + expect(seen.status).toBe(200); + expect(seen.body).toEqual({ success: true, data: { locales: ['en', 'zh-CN'] } }); + }); + + it('parses against BaseResponseSchema and violates nothing', () => { + const { res, seen } = capture(); + sendOk(res, { id: 'lnk_1' }); + + expect(BaseResponseSchema.safeParse(seen.body).success).toBe(true); + expect(envelopeViolations(seen.body)).toEqual([]); + }); + + it('honours a non-200 success status (the 201 the create routes answer)', () => { + const { res, seen } = capture(); + sendOk(res, { datasource: { name: 'crm' } }, 201); + + expect(seen.status).toBe(201); + expect(seen.body).toEqual({ success: true, data: { datasource: { name: 'crm' } } }); + }); + + it('does not spread the payload beside the flag', () => { + // `{ success: true, data: link, link }` parses clean against the schema + // and is still drift — it shipped on /share-links for as long as nobody + // looked (#4038). One `data` slot is what prevents re-inventing it. + const { res, seen } = capture(); + sendOk(res, { token: 't' }); + + expect(Object.keys(seen.body as object)).toEqual(['success', 'data']); + }); +}); + +describe('sendError', () => { + it('nests `message` inside `error` rather than beside it', () => { + // The pre-#3675 dialect put the message as a SIBLING of `error`, so a + // caller reading `body.error.message` got `undefined`. + const { res, seen } = capture(); + sendError(res, 404, 'RESOURCE_NOT_FOUND', 'Datasource "crm" does not exist.'); + + expect(seen.status).toBe(404); + expect(seen.body).toEqual({ + success: false, + error: { code: 'RESOURCE_NOT_FOUND', message: 'Datasource "crm" does not exist.' }, + }); + expect((seen.body as any).error.message).toBe('Datasource "crm" does not exist.'); + }); + + it('parses against BaseResponseSchema and violates nothing', () => { + const { res, seen } = capture(); + sendError(res, 503, 'SERVICE_UNAVAILABLE', 'The datasource-admin service is not available.'); + + expect(BaseResponseSchema.safeParse(seen.body).success).toBe(true); + expect(envelopeViolations(seen.body)).toEqual([]); + }); + + it('merges `extra` into `error` — the `details` slot the callers use', () => { + const { res, seen } = capture(); + sendError(res, 400, 'PACKAGE_DELETE_PARTIAL', 'Deleting crm left 2 item(s) behind.', { + details: { failed: ['a', 'b'], cleanups: [] }, + }); + + expect(seen.body).toEqual({ + success: false, + error: { + code: 'PACKAGE_DELETE_PARTIAL', + message: 'Deleting crm left 2 item(s) behind.', + details: { failed: ['a', 'b'], cleanups: [] }, + }, + }); + expect(BaseResponseSchema.safeParse(seen.body).success).toBe(true); + }); + + it('rejects an unregistered code at COMPILE time, not just on parse', () => { + // `code` is `ErrorCode` — `StandardErrorCode ∪ ERROR_CODE_LEDGER`. The + // seven copies typed it `string`, so this was caught at runtime only, + // and only on a route some test happened to drive. + const { res } = capture(); + // @ts-expect-error 'NOT_A_REGISTERED_CODE' is not in the ADR-0112 union. + sendError(res, 400, 'NOT_A_REGISTERED_CODE', 'invented'); + }); +}); diff --git a/packages/types/src/response-envelope.ts b/packages/types/src/response-envelope.ts new file mode 100644 index 0000000000..28a13ef921 --- /dev/null +++ b/packages/types/src/response-envelope.ts @@ -0,0 +1,134 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ONE writer for the declared REST response envelope (#3973). + * + * `BaseResponseSchema` (`packages/spec/src/api/contract.zod.ts`) declares one + * envelope for every REST body the platform emits: + * + * { success: true, data } + * { success: false, error: { code, message } } + * + * The schema declares it once. Until this file, the code that *wrote* it was + * copied per route module — seven `sendOk` / `sendError` pairs after #3843 and + * #3983 converted the last drifting one, so the envelope's shape lived in + * fourteen places rather than one. + * + * ## Why a shared builder rather than seven agreeing copies + * + * `scripts/check-route-envelope.mjs` proves the copies agree today, and that is + * exactly why this is a cleanup and not a bug fix. But a guard proves agreement; + * it does not create it. An eighth module starts by copying the pair again — + * which is not hypothetical, it is the observed history: `share-link-routes.ts` + * was found by the repo-wide scan already drifting, and its drift had broken + * `client.shareLinks.create()` / `.list()` through `unwrapResponse` (#3983). + * + * ## Why here + * + * Placement was the open question in #3973, not design. `packages/spec` is + * schemas-only (Prime Directive #2), and the callers span `packages/rest`, four + * `services/*` and one `plugins/*`, which rules out anything that depends on + * them. `@objectstack/types` depends on nothing but `@objectstack/spec`, so + * every caller can reach it, and it is where the repo already puts a helper the + * HTTP boundaries share: {@link looksLikeInternalErrorLeak} lives one file over + * for the same reason, and made the same argument first — "do not ship driver + * internals to clients" is a property of the boundary, not of one router. + * + * Writing the declared envelope is the same kind of property. + * + * ## What this does NOT change + * + * Every byte on the wire. The seven pairs were already identical modulo the + * optional `status` and `extra` parameters unioned below; this file is their + * union, and each module's driven conformance suite still parses its real + * bodies against the real spec schemas. + * + * The dispatcher surface (`packages/runtime/src/domains/*`) is deliberately not + * a caller: those handlers RETURN `{ status, body }` for a central sender rather + * than writing to a response, so they are already consolidated behind their own + * `deps.success` / `deps.error` helpers and audited by the other half of + * `check-route-envelope.mjs`. + */ + +import type { ErrorCode } from '@objectstack/spec/api'; + +/** + * The only thing an envelope builder needs from a response object. + * + * Structural on purpose, so this file depends on no HTTP contract at all: + * `IHttpResponse` (`@objectstack/spec/contracts`) satisfies it, and so does the + * `any`-typed `res` the three older route modules still carry. That is what lets + * a package import the builders without also importing a server abstraction. + */ +export interface EnvelopeResponse { + status(code: number): EnvelopeResponse; + json(body: unknown): unknown; +} + +/** + * Emit a success body in the DECLARED envelope — `{ success: true, data }`. + * + * `data` carries the route's payload; it is not spread. A payload duplicated + * into a stray top-level key (`{ success: true, data: link, link }`) parses + * clean against `BaseResponseSchema` and is still drift — that shipped on + * `/share-links` for as long as nobody looked (#4038), which is why + * `envelopeViolations` exists beside the schema and why there is one `data` + * slot here rather than a spread. + * + * `status` defaults to 200 and is set explicitly even then. Five of the seven + * modules already did that; the two that called `res.json(...)` bare are + * unaffected, because the default they were relying on is the value now passed. + */ +export function sendOk(res: EnvelopeResponse, data: unknown, status = 200): void { + res.status(status).json({ success: true, data }); +} + +/** + * Emit an error in the DECLARED envelope — `{ success: false, error: { code, + * message } }`, with `code` a semantic STRING and `message` a field OF `error` + * rather than a sibling of it. + * + * Both halves of that sentence were once wrong somewhere: `error` was a bare + * string in `service-storage` and `admin-routes` (so `body.error.message` read + * `undefined`), and `code` was the human message in `package-routes` (#3675 → + * #3689 → #3843). + * + * ## `code` is the closed ADR-0112 vocabulary, not `string` + * + * All seven copies typed this parameter `string`, so an invented code was caught + * only at runtime, by a conformance suite parsing a driven body against + * `ApiErrorSchema` — i.e. only on the routes a test happened to drive. `ErrorCode` + * is `StandardErrorCode ∪ ERROR_CODE_LEDGER` (`error-code-ledger.zod.ts`), the + * same union that schema validates against, so consolidating here moves the check + * to compile time for every call site at once. It cost no call-site churn: every + * code the seven modules emit was already registered. + * + * A new code is registered in `ERROR_CODE_LEDGER` under its owning package — + * and if the condition is generic (not found / permission / validation), the + * standard catalog is used instead of registering a synonym for it. + * + * ## `extra` + * + * Merged into `error`, for the declared optional fields `ApiErrorSchema` carries + * beside `code` and `message` — `details`, `category`, `requestId`, `httpStatus`. + * `details` is the slot for structured context: `package-routes` puts a partial + * delete's per-item failures there, `settings-routes` the whole + * `SettingsActionResult`. + * + * It is typed `Record` rather than the declared fields because + * `settings-routes` also passes `namespace` / `key` / `reason` / `fields` as + * SIBLINGS of `code`, which `ApiErrorSchema` does not declare — `z.object` + * strips them, so those bodies are conformant by stripping rather than by + * declaration. Tightening this parameter is a wire change to that module's error + * bodies, deliberately out of scope for a consolidation that changes no bytes; + * it is tracked separately. + */ +export function sendError( + res: EnvelopeResponse, + status: number, + code: ErrorCode, + message: string, + extra?: Record, +): void { + res.status(status).json({ success: false, error: { code, message, ...extra } }); +} diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 372ebcc091..e9cbf52dc4 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1564,6 +1564,9 @@ importers: '@objectstack/spec': specifier: workspace:* version: link:../../spec + '@objectstack/types': + specifier: workspace:* + version: link:../../types devDependencies: '@types/node': specifier: ^26.1.1 @@ -2010,6 +2013,9 @@ importers: '@objectstack/spec': specifier: workspace:* version: link:../../spec + '@objectstack/types': + specifier: workspace:* + version: link:../../types devDependencies: '@types/node': specifier: ^26.1.1 @@ -2220,6 +2226,9 @@ importers: '@objectstack/spec': specifier: workspace:* version: link:../../spec + '@objectstack/types': + specifier: workspace:* + version: link:../../types devDependencies: '@types/node': specifier: ^26.1.1 diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index 90689102fd..088b4db3d3 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -22,8 +22,11 @@ * * The platform answers REST from two kinds of file, and this audits both: * - * 1. **Route modules** (`*-routes.ts`) — call `res.json(...)`. Counted below - * in `MODULES`. + * 1. **Route modules** (`*-routes.ts`) — write to a response object. Since + * #3973 every one of them writes through ONE shared pair — `sendOk` / + * `sendError` in `@objectstack/types` — so a converted module calls + * `res.json` zero times itself. Counted below in `MODULES`; the shared + * pair is pinned separately in `SHARED_BUILDER`. * 2. **Dispatcher domains** (`runtime/src/domains/*.ts`) — RETURN * `{ status, body }` for a central sender, so they never call `res.json` * and the first scan cannot see them. Counted in `DISPATCHER_DOMAINS`. @@ -36,13 +39,22 @@ * ## Why a whole-repo scan rather than a per-module test * * The load-bearing check is structural, not per-route: it COUNTS the sites where - * a response gets built. When every body in a module goes through its `sendOk` / - * `sendError` pair, that count is fixed at two and does not grow with the route - * list — so a *new* route that hand-rolls a body moves the count and fails here, - * which is the one thing a driven-body test can never cover (it can only drive - * the routes that existed the day it was written). The domain table applies the - * same idea from the other end: a domain that answers only through the `deps.*` - * helpers hand-builds zero responses, so any hand-built one has to be classified. + * a response gets built. A module that routes every body through the shared + * `sendOk` / `sendError` builds none itself, so that count is fixed at ZERO and + * does not grow with the route list — a *new* route that hand-rolls a body moves + * it off zero and fails here, which is the one thing a driven-body test can never + * cover (it can only drive the routes that existed the day it was written). The + * domain table applies the same idea from the other end: a domain that answers + * only through the `deps.*` helpers hand-builds zero responses, so any hand-built + * one has to be classified. + * + * Until #3973 each module carried its own copy of the pair and so declared a + * fixed `2 / 1 / 1`. That proved the seven copies agreed — it could not stop an + * eighth module from copying the pair again, which is what moving them onto one + * writer does. So what the count asserts per module is now strictly + * stronger: not "your two builders are the enveloped ones" but "you have no + * builders". The two write sites that remain in the whole surface are the shared + * pair itself, pinned by `SHARED_BUILDER` below. * * #3675 / #3689 shipped this as a regex block copied into each converted * package. Three copies was the signal it wanted lifting (#3843 option 3), and @@ -55,7 +67,7 @@ * than surveying. * * A module discovered by the scan but absent from the table is an ERROR, not a - * default: silently applying `2 / 1 / 1` to an unknown module would let a new + * default: silently applying `0 / 0 / 0` to an unknown module would let a new * one pass by coincidence. * * ## Why AST, not regex @@ -113,21 +125,25 @@ const ROOT = join(fileURLToPath(new URL('.', import.meta.url)), '..'); * There is no fourth state where nobody looked. */ const MODULES = { - // ── Conformant (#3675, #3689, #3843) ──────────────────────────────────── - 'packages/services/service-storage/src/storage-routes.ts': { responses: 2, ok: 1, err: 1 }, - 'packages/services/service-settings/src/settings-routes.ts': { responses: 2, ok: 1, err: 1 }, - 'packages/services/service-datasource/src/admin-routes.ts': { responses: 2, ok: 1, err: 1 }, - 'packages/rest/src/external-datasource-routes.ts': { responses: 2, ok: 1, err: 1 }, - 'packages/rest/src/package-routes.ts': { responses: 2, ok: 1, err: 1 }, - // Consolidated by #3973: #3636 put the right envelope on its three read routes - // but built it inline in four places, so this module carried a ratchet at 5 / 4 / 1 - // until those collapsed behind a `sendOk`. - 'packages/services/service-i18n/src/i18n-service-plugin.ts': { responses: 2, ok: 1, err: 1 }, + // ── Conformant (#3675, #3689, #3843, #3983 — consolidated by #3973) ────── + // + // All zero: every body these seven emit is written by the shared pair, so + // none of them builds a response itself. They each declared `2 / 1 / 1` while + // they carried their own copy of that pair. + 'packages/services/service-storage/src/storage-routes.ts': { responses: 0, ok: 0, err: 0 }, + 'packages/services/service-settings/src/settings-routes.ts': { responses: 0, ok: 0, err: 0 }, + 'packages/services/service-datasource/src/admin-routes.ts': { responses: 0, ok: 0, err: 0 }, + 'packages/rest/src/external-datasource-routes.ts': { responses: 0, ok: 0, err: 0 }, + 'packages/rest/src/package-routes.ts': { responses: 0, ok: 0, err: 0 }, + // #3636 put the right envelope on this module's three read routes but built it + // inline in four places, so it carried a ratchet at 5 / 4 / 1 until those + // collapsed behind a local `sendOk` — and then onto the shared one. + 'packages/services/service-i18n/src/i18n-service-plugin.ts': { responses: 0, ok: 0, err: 0 }, // Converted by #3983, the last ratchet. This module was never emitting a // `success` flag at all, which broke `client.shareLinks.create()`/`.list()` // through `unwrapResponse`; it converged onto the shapes its dispatcher twin // (`runtime/src/domains/share-links.ts`) had always returned. - 'packages/plugins/plugin-sharing/src/share-link-routes.ts': { responses: 2, ok: 1, err: 1 }, + 'packages/plugins/plugin-sharing/src/share-link-routes.ts': { responses: 0, ok: 0, err: 0 }, // ── Exempt ────────────────────────────────────────────────────────────── @@ -155,6 +171,26 @@ const MODULES = { /** Identifiers whose `.json()` READS a request rather than writing a response. */ const REQUEST_RECEIVERS = new Set(['req', 'request']); +/** + * The one file that writes the envelope for surface 1, and the counts that make + * it that file (#3973). + * + * Every zero in `MODULES` above is only meaningful because these two write sites + * exist somewhere: a module can reach `0 / 0 / 0` either by routing everything + * through the shared pair, or by answering nothing at all. Pinning the pair here + * keeps the invariant total rather than per-module — two write sites, one + * `success: true`, one `success: false`, for every REST body the route modules + * emit. + * + * It also makes deleting or relocating the builders a loud failure instead of a + * silent one: the path is asserted to exist, so moving it without updating this + * line fails the gate rather than quietly leaving nothing pinned. + */ +const SHARED_BUILDER = { + file: 'packages/types/src/response-envelope.ts', + counts: { responses: 2, ok: 1, err: 1 }, +}; + // ── The dispatcher's OTHER surface ─────────────────────────────────────────── /** @@ -398,7 +434,8 @@ function audit() { if (!declared) { problems.push( `${file}\n NOT DECLARED. Add it to MODULES in scripts/check-route-envelope.mjs.\n` + - ` If it emits the envelope, declare { responses: 2, ok: 1, err: 1 }. If it still\n` + + ` If it emits the envelope through the shared sendOk/sendError from\n` + + ` @objectstack/types, declare { responses: 0, ok: 0, err: 0 }. If it still\n` + ` drifts, declare its CURRENT counts plus a \`ratchet\` naming the issue that\n` + ` will fix it — never leave a route module unaudited.`, ); @@ -414,7 +451,9 @@ function audit() { problems.push( `${file}\n ${key}: found ${got[key]}, declared ${want[key]}` + (key === 'responses' && got[key] > want[key] - ? `\n A route is building its own body instead of calling the envelope helper.` + + ? `\n A route is building its own body instead of calling the shared envelope` + + `\n helper. Import { sendOk, sendError } from '@objectstack/types' rather than` + + `\n writing the shape again — see packages/types/src/response-envelope.ts.` + `\n Write sites: ${got.sites.join(', ')}` : '') + (want.ratchet ? `\n (ratchet for ${want.ratchet} — the declared numbers pin current drift)` : ''), @@ -429,6 +468,31 @@ function audit() { } } + // ── The shared builder every zero above depends on ──────────────────────── + let sharedSource; + try { + sharedSource = readFileSync(join(ROOT, SHARED_BUILDER.file), 'utf8'); + } catch { + problems.push( + `${SHARED_BUILDER.file}\n the shared envelope builder is MISSING. Every route module declares\n` + + ` 0 write sites because this file holds them; without it nothing pins the\n` + + ` envelope's shape at all. Restore it, or update SHARED_BUILDER if it moved.`, + ); + } + if (sharedSource !== undefined) { + const got = scanSource(sharedSource, SHARED_BUILDER.file); + for (const [key, want] of Object.entries(SHARED_BUILDER.counts)) { + if (got[key] !== want) { + problems.push( + `${SHARED_BUILDER.file}\n ${key}: found ${got[key]}, declared ${want}\n` + + ` This is the ONE writer for every route module's bodies. A third write site\n` + + ` here is a third dialect for all of them at once; a missing one means a half\n` + + ` of the envelope nobody emits. Sites: ${got.sites.join(', ') || '(none)'}`, + ); + } + } + } + // ── The dispatcher domains ──────────────────────────────────────────────── const domains = discoverDomains(); @@ -478,8 +542,9 @@ function audit() { for (const p of problems) console.error(' ' + p + '\n'); console.error( 'Every REST body must be built in ONE place per surface, in the envelope\n' + - 'BaseResponseSchema declares — a route module\'s sendOk / sendError pair, or a\n' + - 'dispatcher domain\'s deps.* helpers. See scripts/check-route-envelope.mjs.', + 'BaseResponseSchema declares — the shared sendOk / sendError in\n' + + '@objectstack/types for route modules, or a dispatcher domain\'s deps.* helpers.\n' + + 'See scripts/check-route-envelope.mjs.', ); process.exit(1); } @@ -492,6 +557,10 @@ function audit() { `✓ Route-envelope conformance — ${discovered.length} route module(s) audited: ` + `${conformant} conformant, ${ratcheted.length} ratcheted, ${exempt.length} exempt`, ); + console.log( + ` all bodies written by ${SHARED_BUILDER.file} ` + + `(${SHARED_BUILDER.counts.responses} write sites, pinned)`, + ); for (const [file, m] of ratcheted) { console.log(` ⚠ ratchet ${m.ratchet}: ${file} (pinned at current drift, not conformant)`); } @@ -527,6 +596,32 @@ function selfTest() { let r = scanSource(sound); assert(r.responses === 2 && r.ok === 1 && r.err === 1, `sound module → ${JSON.stringify(r)}`); + // The shape every route module has had since #3973: the builders live in + // @objectstack/types, so the module itself writes NOTHING. This is the state + // the MODULES table now declares, and the reason it declares zeros. + r = scanSource(` + import { sendOk, sendError } from '@objectstack/types'; + http.get('/a', (q, res) => sendOk(res, { a: 1 })); + http.post('/b', (q, res) => sendOk(res, { b: 2 }, 201)); + http.get('/c', (q, res) => sendError(res, 404, 'RESOURCE_NOT_FOUND', 'gone')); + `); + assert( + r.responses === 0 && r.ok === 0 && r.err === 0, + `a fully consolidated module must scan to zero → ${JSON.stringify(r)}`, + ); + + // …and a module that consolidated MOST of its bodies is exactly what the + // zeros are for: one hand-rolled body moves the count off zero and fails. + r = scanSource(` + import { sendOk } from '@objectstack/types'; + http.get('/a', (q, res) => sendOk(res, { a: 1 })); + http.get('/b', (q, res) => res.status(200).json({ success: true, data: { b: 2 } })); + `); + assert( + r.responses === 1 && r.ok === 1, + `one hand-rolled body among helper calls must be seen → ${JSON.stringify(r)}`, + ); + // (1) A `//` inside a string truncated the rest of the line for the regex // version, hiding the response write after it. r = scanSource(`const base = 'http://local'; res.json({ success: true, data });`);