refactor(types,rest,services,plugin-sharing): one shared writer for the response envelope, and error.code is enforced at compile time (#3973)

.changeset/shared-response-envelope-builders.md

@@ -0,0 +1,103 @@
+---
+"@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.
+
+## `extra` is closed at the same place
+
+`sendError`'s last parameter is `Pick<ApiError, 'category' | 'httpStatus' |
+'details' | 'requestId'>` — exactly what `ApiErrorSchema` declares beside `code`
+and `message`.
+
+It was `Record<string, unknown>` while `settings-routes` still hung `namespace` /
+`key` / `reason` / `fields` beside `code`. Those bodies passed every gate anyway:
+`ApiErrorSchema` is a plain `z.object`, so unknown keys were STRIPPED rather than
+rejected, and `envelopeViolations` inspects only the body's top level —
+conformant *by stripping* rather than by declaration. #4224 moved that module
+onto `details`, which is what lets the parameter close here. Closing it at the
+shared builder is the part that lasts: an undeclared sibling is now a compile
+error in every module at once, rather than a key that quietly evaporates in
+whichever module reintroduces it.
+
+## 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.

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",

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<any[]>` — 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: <payload> }`: 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 {

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: '<string>' }`, 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: '<string>' }`,
-   * 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.');
 

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: '<string>' }` throughout — and the
@@ -63,36 +62,17 @@ 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
  * package is `RESOURCE_NOT_FOUND`, an unexpected throw is `INTERNAL_ERROR`. Only
  * 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;
       }