refactor(service-settings)!: settings error bodies use the declared details slot (#4224)

.changeset/settings-error-details-declared-slot.md

@@ -0,0 +1,58 @@
+---
+"@objectstack/service-settings": minor
+---
+
+refactor!: settings error bodies stop hanging undeclared keys beside `code`/`message` (#4224)
+
+Four `/api/settings/*` error branches spread ad-hoc context as SIBLINGS of `code`
+and `message` inside `error`. `ApiErrorSchema` declares `code`, `message`,
+`category?`, `httpStatus?`, `details?`, `requestId?` — and none of `namespace`,
+`key`, `reason`, `fields`. The 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. They were conformant
+*by stripping*, not by declaration. The same module already used the declared
+slot correctly one branch over (`SETTINGS_ACTION_FAILED` → `error.details`), so
+this is one file speaking two dialects, not a missing capability.
+
+**Wire change — FROM → TO.** In every case the values are unchanged; only their
+position moves, into the `details` slot the contract declares:
+
+| Code | HTTP | FROM | TO |
+|---|---|---|---|
+| `SETTINGS_FORBIDDEN` | 403 | `error.namespace` | `error.details.namespace` |
+| `UNKNOWN_KEY` | 400 | `error.namespace`, `error.key` | `error.details.namespace`, `error.details.key` |
+| `SETTINGS_LOCKED` | 409 | `error.namespace`, `error.key`, `error.reason` | `error.details.namespace`, `error.details.key`, `error.details.reason` |
+| `SETTINGS_VALIDATION` | 400 | `error.namespace`, `error.fields` | `error.details.namespace`, `error.details.fields` |
+
+**One-line fix for a consumer:** read `error.details.<key>` where you read
+`error.<key>`, or `error.details?.<key> ?? error.<key>` if you support servers on
+both sides of the change. The console's own fix (objectui#3078) is the tolerant
+form.
+
+**`SETTINGS_VALIDATION.fields` also changes shape**, because `fields` is the name
+ADR-0114 (#3977) closed for `FieldError[]` and keeping a map under it would leave
+one spelling meaning two shapes:
+
+- **FROM** `{ [key]: message }` — a `Record<string, string>`, the constraint named
+  only in the prose of the message.
+- **TO** `FieldError[]` — `{ field, code, message, label, constraint? }`, where
+  `code` is a member of the closed field-level catalog: `required` for an empty
+  required specifier, `invalid_format` for a value that misses its declared
+  `pattern` (which travels as `constraint.pattern`).
+
+A consumer that rendered the map's values reads `f.message` per entry instead;
+one that wants to branch on *why* a value was rejected can now read `f.code`
+rather than substring-matching English. objectui's `extractFieldErrors` already
+reads `details.fields`, so settings validation failures become renderable
+per-field there with no further change.
+
+**The exported `SettingsValidationError.fields` changes with it** — same
+`Record<string, string>` → `FieldError[]` mapping — since the route only relays
+what the service throws, and the constraint kind is knowable at the throw site
+and nowhere after it.
+
+`sendError`'s last parameter is tightened from `extra?: Record<string, unknown>`
+to `ApiError`'s own optional fields, and its `code` from `string` to the closed
+ADR-0112 `ErrorCode` union. That is what keeps this fixed: an undeclared sibling
+is now a compile error at the call site rather than a key that quietly evaporates
+at the schema boundary.

content/docs/kernel/runtime-services/settings-service.mdx

@@ -26,7 +26,9 @@ services.settings.subscribe(namespace: string | undefined, handler: SettingsChan
 - `SETTINGS_LOCKED` (`SettingsLockedError`)
 - `SETTINGS_UNKNOWN_NAMESPACE` (`UnknownNamespaceError`)
 - `SETTINGS_UNKNOWN_KEY` (`UnknownKeyError`)
-- `SETTINGS_VALIDATION` (`SettingsValidationError`) — thrown by `set` / `setMany` when a write would leave a visible `required` specifier empty or violate its `pattern`
+- `SETTINGS_VALIDATION` (`SettingsValidationError`) — thrown by `set` / `setMany` when a write would leave a visible `required` specifier empty or violate its `pattern`. Its `fields` is a `FieldError[]` (ADR-0114), one entry per offending key, with `code` naming the constraint: `required` or `invalid_format`.
+
+Over REST, each of these puts its context in the declared `error.details` slot — `{ namespace, key, reason, fields }` as the branch carries them — never as siblings of `error.code` / `error.message` (#4224).
 
 ## Notes
 

packages/services/service-settings/src/envelope.conformance.test.ts

@@ -32,7 +32,7 @@
  */
 
 import { describe, expect, it, vi } from 'vitest';
-import { BaseResponseSchema, envelopeViolations } from '@objectstack/spec/api';
+import { ApiErrorSchema, BaseResponseSchema, FieldErrorSchema, envelopeViolations } from '@objectstack/spec/api';
 import { SettingsNamespacePayloadSchema } from '@objectstack/spec/system';
 import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts';
 import { SettingsService } from './settings-service';
@@ -288,10 +288,162 @@ describe('settings envelope (#3843) — error bodies', () => {
       // bare-string dialect the sibling modules carried cannot land quietly.
       expect(typeof body.error).not.toBe('string');
       expect(body.code).toBeUndefined();
+
+      // [#4224] No key inside `error` that `ApiErrorSchema` does not declare.
+      //
+      // This is the assertion neither gate above can make. `safeParse` STRIPS
+      // unknown keys (`ApiErrorSchema` is a plain `z.object`), so it passed
+      // `error.namespace` / `.key` / `.reason` / `.fields` for as long as this
+      // module emitted them; `envelopeViolations` deliberately inspects only the
+      // body's top level. Between them a body could carry four undeclared keys
+      // and read as fully conformant — conformant *by stripping*, which is not
+      // the same claim as conformant by declaration.
+      //
+      // Derived from `ApiErrorSchema.shape` rather than a hand-written list, so
+      // a field added to the contract is allowed here the moment it is declared,
+      // and one removed stops being allowed — the failure mode of a restated
+      // list is that it silently keeps blessing a retired key.
+      expect(
+        Object.keys(body.error).filter((k) => !(k in (ApiErrorSchema as any).shape)),
+        `error carries keys ApiErrorSchema does not declare: ${JSON.stringify(body.error)}`,
+      ).toEqual([]);
     });
   }
 });
 
+describe('settings envelope (#4224) — the four ad-hoc keys travel in the declared slot', () => {
+  /**
+   * Each of these branches used to spread its context as SIBLINGS of `code` and
+   * `message`. They now use `error.details`, the slot `ApiErrorSchema` declares
+   * for exactly this and the one `SETTINGS_ACTION_FAILED` was already using one
+   * branch over — so the module speaks one dialect rather than two.
+   *
+   * Both directions are asserted per case: the value is under `details`, AND the
+   * old top-level spelling is gone. Asserting only the first would pass a body
+   * that emitted both, which is how a "migration" quietly becomes a permanent
+   * dual-write.
+   */
+  const CASES: Array<{
+    name: string;
+    status: number;
+    code: string;
+    details: Record<string, unknown>;
+    gone: string[];
+    run: () => Promise<Captured>;
+  }> = [
+    {
+      name: 'SETTINGS_FORBIDDEN carries its namespace',
+      status: 403,
+      code: 'SETTINGS_FORBIDDEN',
+      details: { namespace: 'branding' },
+      gone: ['namespace'],
+      run: async () => {
+        const { http } = mount(anon);
+        return drive(http, 'GET /api/settings/:namespace', { params: { namespace: 'branding' } });
+      },
+    },
+    {
+      name: 'UNKNOWN_KEY carries its namespace and key',
+      status: 400,
+      code: 'UNKNOWN_KEY',
+      details: { namespace: 'branding', key: 'not_a_key' },
+      gone: ['namespace', 'key'],
+      run: async () => {
+        const { http } = mount();
+        return drive(http, 'PUT /api/settings/:namespace', {
+          params: { namespace: 'branding' },
+          body: { not_a_key: 1 },
+        });
+      },
+    },
+    {
+      name: 'SETTINGS_LOCKED carries its namespace, key and reason',
+      status: 409,
+      code: 'SETTINGS_LOCKED',
+      details: { namespace: 'branding', key: 'workspace_name', reason: 'locked-by-env' },
+      gone: ['namespace', 'key', 'reason'],
+      run: async () => {
+        // An `OS_BRANDING_WORKSPACE_NAME` in the environment locks the key, so a
+        // write to it is refused — the one branch that needs a locked namespace.
+        const { http } = mount(admin, { OS_BRANDING_WORKSPACE_NAME: 'Locked Co' });
+        return drive(http, 'PUT /api/settings/:namespace', {
+          params: { namespace: 'branding' },
+          body: { workspace_name: 'Acme' },
+        });
+      },
+    },
+  ];
+
+  for (const c of CASES) {
+    it(`${c.name} under error.details, not beside code/message`, async () => {
+      const { status, body } = await c.run();
+      expect(status).toBe(c.status);
+      expect(body.error.code).toBe(c.code);
+      expect(body.error.details).toMatchObject(c.details);
+      for (const k of c.gone) {
+        expect(body.error[k], `error.${k} is still a sibling of code/message`).toBeUndefined();
+      }
+    });
+  }
+});
+
+describe('settings envelope (#4224) — SETTINGS_VALIDATION speaks the field-level vocabulary', () => {
+  /**
+   * The decision #4224 asked for: `fields` was a `Record<key, message>` hung
+   * beside `code`, and `fields` is the name ADR-0114 (#3977) closed for
+   * `FieldError[]`. Keeping the map under that name would have left one spelling
+   * meaning two shapes — so it became the declared array, in the declared slot.
+   */
+  const lockedPattern = () => {
+    const { http, service } = mount();
+    service.registerManifest({
+      namespace: 'validated',
+      label: 'Validated',
+      writePermission: 'setup.write',
+      readPermission: 'setup.access',
+      specifiers: [
+        { key: 'model', type: 'text', label: 'Model', pattern: '^[a-z]+/[a-z]+$', description: 'Use provider/model.' },
+        { key: 'token', type: 'text', label: 'API token', required: true },
+      ],
+    } as any);
+    return http;
+  };
+
+  it('every entry parses as the declared FieldError', async () => {
+    const http = lockedPattern();
+    const { status, body } = await drive(http, 'PUT /api/settings/:namespace', {
+      params: { namespace: 'validated' },
+      body: { model: 'gpt-4o', token: '' },
+    });
+    expect(status).toBe(400);
+    expect(body.error.code).toBe('SETTINGS_VALIDATION');
+
+    const fields = body.error.details?.fields;
+    expect(Array.isArray(fields), `details.fields is not an array: ${JSON.stringify(body.error)}`).toBe(true);
+    expect(fields.length).toBeGreaterThan(0);
+    for (const f of fields) {
+      const parsed = FieldErrorSchema.safeParse(f);
+      expect(parsed.success, `not a FieldError: ${JSON.stringify(parsed.error ?? f)}`).toBe(true);
+    }
+    // The codes come from the closed ADR-0114 catalog, so a consumer can branch
+    // on the constraint instead of substring-matching the message.
+    expect(fields.map((f: any) => f.code).sort()).toEqual(['invalid_format', 'required']);
+  });
+
+  it('the pre-#4224 map is gone from both of its old spellings', async () => {
+    const http = lockedPattern();
+    const { body } = await drive(http, 'PUT /api/settings/:namespace', {
+      params: { namespace: 'validated' },
+      body: { token: '' },
+    });
+    // Not a sibling of code/message any more …
+    expect(body.error.fields).toBeUndefined();
+    expect(body.error.namespace).toBeUndefined();
+    // … and not the `key → message` object under its new home either.
+    expect(Array.isArray(body.error.details.fields)).toBe(true);
+  });
+});
+
 describe('settings envelope (#3843) — a reported action failure keeps its detail', () => {
   it('carries the whole SettingsActionResult under error.details', async () => {
     const { http } = mount();

packages/services/service-settings/src/settings-routes.ts

@@ -13,6 +13,7 @@
  * `SettingsService`.
  */
 
+import type { ApiError, ErrorCode } from '@objectstack/spec/api';
 import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts';
 import { SettingsService } from './settings-service.js';
 import {
@@ -57,8 +58,31 @@ const defaultContext = (_req: IHttpRequest): SettingsContext => ({ enforced: tru
  * 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.
+ *
+ * ## Why the last parameter is the declared fields, not a `Record` (#4224)
+ *
+ * It used to be `extra?: Record<string, unknown>`, spread straight into `error`,
+ * and four branches used it to hang `namespace` / `key` / `reason` / `fields`
+ * beside `code` and `message`. `ApiErrorSchema` declares none of those. The
+ * bodies still passed every gate — `ApiErrorSchema` is a plain `z.object`, so
+ * unknown keys were STRIPPED rather than rejected, and `envelopeViolations`
+ * inspects only the body's top level — which made them conformant *by
+ * stripping* rather than by declaration, and made a `safeParse` pass mean less
+ * than it looked like it meant.
+ *
+ * Typing the parameter as `ApiError`'s own optional fields is what stops that
+ * coming back: an undeclared sibling is now a compile error at the call site
+ * instead of a key that quietly evaporates at the schema boundary. `code` is
+ * likewise the closed ADR-0112 `ErrorCode` union rather than `string`, so an
+ * unregistered code fails to typecheck rather than to parse.
  */
-function sendError(res: IHttpResponse, status: number, code: string, message: string, extra?: Record<string, unknown>) {
+function sendError(
+  res: IHttpResponse,
+  status: number,
+  code: ErrorCode,
+  message: string,
+  extra?: Pick<ApiError, 'category' | 'httpStatus' | 'details' | 'requestId'>,
+) {
   res.status(status).json({ success: false, error: { code, message, ...extra } });
 }
 
@@ -91,7 +115,7 @@ export function registerSettingsRoutes(
       sendOk(res, { manifests });
     } catch (err: any) {
       if (err instanceof SettingsForbiddenError) {
-        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace });
+        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } });
       } else {
         sendError(res, 500, 'INTERNAL_ERROR', err?.message ?? 'Failed to list manifests');
       }
@@ -106,7 +130,7 @@ export function registerSettingsRoutes(
       sendOk(res, payload);
     } catch (err: any) {
       if (err instanceof SettingsForbiddenError) {
-        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace });
+        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } });
       } else if (err instanceof UnknownNamespaceError) {
         sendError(res, 404, 'UNKNOWN_NAMESPACE', err.message);
       } else {
@@ -144,21 +168,28 @@ export function registerSettingsRoutes(
       sendOk(res, { values: result });
     } catch (err: any) {
       if (err instanceof SettingsForbiddenError) {
-        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace });
+        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } });
       } else if (err instanceof SettingsLockedError) {
         sendError(res, 409, 'SETTINGS_LOCKED', err.message, {
-          namespace: err.namespace,
-          key: err.key,
-          reason: err.reason,
+          details: { namespace: err.namespace, key: err.key, reason: err.reason },
         });
       } else if (err instanceof UnknownNamespaceError) {
         sendError(res, 404, 'UNKNOWN_NAMESPACE', err.message);
       } else if (err instanceof UnknownKeyError) {
-        sendError(res, 400, 'UNKNOWN_KEY', err.message, { namespace: err.namespace, key: err.key });
+        sendError(res, 400, 'UNKNOWN_KEY', err.message, {
+          details: { namespace: err.namespace, key: err.key },
+        });
       } else if (err instanceof SettingsValidationError) {
+        // `details.fields` is `FieldError[]` (ADR-0114) — the same array shape
+        // the record validators and the dispatcher's validation exit carry, so
+        // the console's field-error extractor reads this one with no per-surface
+        // special case. It is `details.fields` rather than a top-level
+        // `error.fields` because `fields` is declared on
+        // `EnhancedApiErrorSchema`, not on the base `ApiErrorSchema` these
+        // routes emit; `validation-failure.ts` puts the array in the same place
+        // for the same reason.
         sendError(res, 400, 'SETTINGS_VALIDATION', err.message, {
-          namespace: err.namespace,
-          fields: err.fields,
+          details: { namespace: err.namespace, fields: err.fields },
         });
       } else {
         sendError(res, 500, 'INTERNAL_ERROR', err?.message ?? 'Failed to write namespace');
@@ -190,7 +221,7 @@ export function registerSettingsRoutes(
       }
     } catch (err: any) {
       if (err instanceof SettingsForbiddenError) {
-        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { namespace: err.namespace });
+        sendError(res, 403, 'SETTINGS_FORBIDDEN', err.message, { details: { namespace: err.namespace } });
       } else if (err instanceof UnknownNamespaceError) {
         sendError(res, 404, 'UNKNOWN_NAMESPACE', err.message);
       } else {

packages/services/service-settings/src/settings-service.test.ts

@@ -311,7 +311,15 @@ describe('SettingsService — save-time validation (required/visible/pattern)',
       }),
     ).rejects.toMatchObject({
       code: 'SETTINGS_VALIDATION',
-      fields: { cloudflare_api_key: expect.stringContaining('required') },
+      // `FieldError[]` since #4224 — the constraint is named by `code`, not
+      // only described in the prose of `message` (ADR-0114).
+      fields: [
+        {
+          field: 'cloudflare_api_key',
+          code: 'required',
+          message: expect.stringContaining('required'),
+        },
+      ],
     });
     // Nothing was persisted — the batch is atomic.
     expect((await svc.get('ai', 'provider')).source).toBe('default');
@@ -321,10 +329,10 @@ describe('SettingsService — save-time validation (required/visible/pattern)',
     const svc = aiService();
     await expect(svc.setMany('ai', { provider: 'cloudflare' })).rejects.toMatchObject({
       code: 'SETTINGS_VALIDATION',
-      fields: {
-        cloudflare_account_id: expect.any(String),
-        cloudflare_api_key: expect.any(String),
-      },
+      fields: expect.arrayContaining([
+        expect.objectContaining({ field: 'cloudflare_account_id', code: 'required' }),
+        expect.objectContaining({ field: 'cloudflare_api_key', code: 'required' }),
+      ]),
     });
   });
 
@@ -357,7 +365,16 @@ describe('SettingsService — save-time validation (required/visible/pattern)',
       svc.setMany('ai', { provider: 'gateway', gateway_model: 'gpt-4o' }),
     ).rejects.toMatchObject({
       code: 'SETTINGS_VALIDATION',
-      fields: { gateway_model: expect.stringContaining('format') },
+      // A pattern miss is `invalid_format`, and the pattern it missed travels
+      // as a discrete `constraint` rather than only inside the sentence.
+      fields: [
+        {
+          field: 'gateway_model',
+          code: 'invalid_format',
+          message: expect.stringContaining('format'),
+          constraint: { pattern: expect.any(String) },
+        },
+      ],
     });
     await expect(
       svc.setMany('ai', { provider: 'gateway', gateway_model: 'anthropic/claude-sonnet-4.6' }),