fix(service-storage,service-i18n): emit the declared error envelope, not a bare { error } (#3675)

.changeset/service-error-envelope-conformance.md

@@ -0,0 +1,50 @@
+---
+"@objectstack/service-storage": patch
+"@objectstack/service-i18n": patch
+---
+
+fix(service-storage,service-i18n): emit the declared error envelope, not a bare `{ error }` (#3675)
+
+#3636 aligned the **success** bodies of the autonomously-mounted service
+routes because those were the ones breaking `ObjectStackClient.unwrapResponse`.
+The error bodies were left alone and stayed a bare `{ error: '<message>' }` —
+with the code, where one existed at all, as a *sibling* of `error` rather than
+a field of it — against a contract (`BaseResponseSchema` + `ApiErrorSchema`)
+that declares `{ success: false, error: { code, message } }`.
+
+So the same SDK method returned two different error shapes depending on which
+provider mounted the route: a caller reading `body.error.message` got the real
+message from the dispatcher and `undefined` from these services. All 32 sites
+(27 in `storage-routes.ts`, 5 in `i18n-service-plugin.ts`) now go through a
+single `sendError` helper per module — the nested-`error` shape the sibling
+services already use (`settings-routes.ts`, `share-link-routes.ts`), plus the
+`success` flag those two still omit and the contract requires.
+
+**Codes moved, and that is the breaking part.** `AUTH_REQUIRED`,
+`ATTACHMENT_DOWNLOAD_DENIED` and `FILE_DOWNLOAD_DENIED` used to sit at
+`body.code`; they now sit at `body.error.code`. The SDK is unaffected — it
+already reads `errorBody?.code || errorBody?.error?.code`, one of the four
+shapes its error path sniffs for, which is the consumer-side shim Prime
+Directive #12 says to cure at the producer. The console's attachment panel
+was NOT: it read the top level only, so every gated download would have
+degraded from "You don't have access to download this attachment." to
+"Download failed (403)". Fixed in objectui to read both dialects, since a
+console build ships independently of the server it talks to.
+
+**Guarded both ways.** New `error-envelope.conformance.test.ts` in each
+service drives every distinct error branch through the real registrar and
+parses the body against the real `BaseResponseSchema` imported from
+`packages/spec` — not a local restatement of it — and scans the module source
+so a new route cannot quietly reintroduce the bare shape. The route ledgers
+(#3563 → #3656) could never have caught this: they audit which routes exist
+and whether the SDK can address them, not what comes back.
+
+Measured and left alone: the dispatcher does not conform either — it puts the
+HTTP status in `error.code`, where the contract declares a semantic string,
+and parks the real code in `details` to work around its own occupied field.
+That deviation is now pinned to exactly one field by a test in
+`http-dispatcher.test.ts` rather than described in prose. Also unchanged:
+service-storage's success bodies are still three shapes of their own
+(`{ data }`, bare `{ url }`, `{ ok, key }`, none with `success: true`) — a
+non-additive change that needs its own issue, not a quiet ride along with this
+one.

content/docs/api/wire-format.mdx

@@ -384,6 +384,27 @@ Returned when an `If-Match` / `expectedVersion` token no longer matches the stor
 **Error Codes:** See the [Error Catalog](/docs/api/error-catalog) for the complete list of error codes and their meanings.
 </Callout>
 
+### Service-mounted routes use the declared envelope
+
+The flat envelope above is what the kernel REST server emits for `/api/v1/data/*`.
+Routes mounted directly by a service plugin — `/api/v1/storage/*` and
+`/api/v1/i18n/*` — instead return the `BaseResponse` shape their contract
+declares, with the code **inside** the error object:
+
+```json
+{
+  "success": false,
+  "error": {
+    "code": "ATTACHMENT_DOWNLOAD_DENIED",
+    "message": "You do not have access to a record this file is attached to"
+  }
+}
+```
+
+Read `body.error.code`, not `body.code`, on these routes. `ObjectStackClient`
+normalizes both — `error.code` and `error.message` are populated whichever
+envelope the server used — so SDK callers do not need to branch.
+
 ---
 
 ## 8. Batch Operations

docs/audits/2026-07-dispatcher-client-route-coverage.md

@@ -271,6 +271,37 @@ capstone also had to prefer exact rows over wildcard families when matching —
 otherwise every `/auth/*` URL would still have been absorbed by `* /auth/**`
 and the new ledger would have changed nothing.
 
+## 12. What the ledgers cannot see: response bodies (#3675)
+
+Worth recording as a **boundary of this audit family**, not an oversight in it.
+Every guard from §1 to §11 answers one question — does this route exist, and
+can the SDK address it? None of them looks at what comes back. That is how
+service-i18n and service-storage carried green `sdk` rows for surfaces that
+emitted a bare `{ error: '<message>' }` against a contract declaring
+`{ success: false, error: { code, message } }`.
+
+Four error dialects were live in-repo at the time:
+
+| Producer | Shape |
+|---|---|
+| `contract.zod.ts` (declared) | `{ success, error: { code: string, message, … } }` |
+| `http-dispatcher.ts` | `{ success: false, error: { message, code: <HTTP number>, details } }` |
+| `rest-server.ts` | `{ error: <string>, code: <SEMANTIC_STRING> }` |
+| `settings-routes.ts`, `share-link-routes.ts` | `{ error: { code, message } }` — no `success` |
+| service-i18n, service-storage | `{ error: <string> }`, sometimes `+ code` at the top level |
+
+#3675 moved the last row to the contract. The rest are unchanged, and the
+dispatcher's deviation is now pinned to exactly one field (`error.code` carries
+the HTTP status where a semantic string is declared) by a test rather than by
+prose — it parks the real code in `details` to work around its own occupied
+field, which is the tell.
+
+The generalisable lesson matches §11's: a guard only covers the question it
+asks. "The route exists" and "the route answers in the declared shape" are two
+questions, and the second one needs the contract imported into the assertion —
+`BaseResponseSchema.safeParse(body)`, not a hand-copied restatement that drifts
+from the schema it claims to check.
+
 ## Follow-up slicing (proposed)
 
 1. **`client.actions.invoke(...)`** — closes the largest hole (3 routes).
@@ -286,6 +317,9 @@ and the new ledger would have changed nothing.
 11. **Control-plane surface** (§10) — #3655, needs a ledger in the `cloud` repo.
 12. **Enumerate `/auth/**`** (§11) — done in #3656; wildcard ratchet 60 → 3.
 13. **Enumerate `/ai/**`** — the last dynamic family, 3 SDK methods.
+14. **Response-shape conformance** (§12) — error path done in #3675 for both
+    services; the storage **success** bodies (three shapes, none carrying
+    `success: true`) and the dispatcher's numeric `error.code` remain.
 
 Each gap closed must flip its ledger row to `sdk` and lower the ratchet bound
 in the conformance test — the guard enforces both directions from PR-1 onward.

packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts

@@ -157,7 +157,7 @@ describe('attachments permission matrix (#2755)', () => {
       body: JSON.stringify({ filename: 'x.txt', mimeType: 'text/plain', size: 1, scope: 'attachments' }),
     });
     expect(res.status).toBe(401);
-    expect(((await res.json()) as any).code).toBe('AUTH_REQUIRED');
+    expect(((await res.json()) as any).error?.code).toBe('AUTH_REQUIRED');
   });
 
   it('(e) authenticated upload succeeds and sys_file.owner_id is server-stamped', async () => {
@@ -319,13 +319,13 @@ describe('attachments permission matrix (#2755)', () => {
     // Anonymous → 401 (was a 200 capability URL before #2970).
     const anon = await stack.api(`/storage/files/${adminFile}/url`);
     expect(anon.status).toBe(401);
-    expect(((await anon.json()) as any).code).toBe('AUTH_REQUIRED');
+    expect(((await anon.json()) as any).error?.code).toBe('AUTH_REQUIRED');
 
     // memberB is authenticated but cannot read att_secret and is not the
     // owner → 403.
     const denied = await stack.apiAs(memberBTok, 'GET', `/storage/files/${adminFile}/url`);
     expect(denied.status).toBe(403);
-    expect(((await denied.json()) as any).code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
+    expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
 
     // The owner (admin) → 200 with a signed URL.
     const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`);

packages/runtime/src/http-dispatcher.test.ts

@@ -2,6 +2,7 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import { HttpDispatcher } from './http-dispatcher.js';
 import { ObjectKernel } from '@objectstack/core';
+import { ApiErrorSchema } from '@objectstack/spec/api';
 
 describe('HttpDispatcher', () => {
     let kernel: ObjectKernel;
@@ -1837,6 +1838,35 @@ describe('HttpDispatcher', () => {
             expect(result.response?.body?.error?.message).toBe('Missing locale parameter');
         });
 
+        /**
+         * The dispatcher's error body is the SHAPE the autonomously-mounted
+         * i18n/storage services were aligned to in #3675 — nested `error`, with
+         * the `success` flag. It is not yet the CONTRACT: `ApiErrorSchema`
+         * declares `code` as a semantic string ('validation_error'), and this
+         * emits the HTTP status as a number. The dispatcher already works
+         * around its own field being occupied — `this.error(msg, 403, { code:
+         * 'PERMISSION_DENIED' })` parks the real code in `details` — which is
+         * the tell that the number is in the wrong place.
+         *
+         * Pinned rather than fixed: `error.code` is read across the SDK, the
+         * console and the dogfood suite, so moving it is its own change.
+         * `toEqual(['code'])` is deliberately exact — if a second field starts
+         * deviating this fails, and if the dispatcher is fixed it also fails
+         * and this pin should be deleted.
+         */
+        it('pins the ONE field where the dispatcher deviates from ApiErrorSchema (#3675)', async () => {
+            const result = await dispatcher.handleI18n('/translations', 'GET', {}, { request: {} });
+            const body = result.response?.body as { success?: boolean; error?: unknown };
+
+            expect(body.success).toBe(false);
+            expect(typeof body.error).toBe('object');
+
+            const parsed = ApiErrorSchema.safeParse(body.error);
+            expect(parsed.success).toBe(false);
+            expect(parsed.error!.issues.map((i) => i.path.join('.'))).toEqual(['code']);
+            expect((body.error as { code: unknown }).code).toBe(400);
+        });
+
         it('should fallback to deriving labels from translations when getFieldLabels is missing', async () => {
             delete mockI18nService.getFieldLabels;
             mockI18nService.getTranslations.mockReturnValue({

packages/services/service-i18n/src/error-envelope.conformance.test.ts

@@ -0,0 +1,192 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Error-envelope conformance for the autonomously-mounted `/api/v1/i18n/*`
+ * routes (#3675) — the error-path twin of the success-path fix in #3636.
+ *
+ * #3636 aligned the SUCCESS bodies because those were the ones breaking
+ * `ObjectStackClient.unwrapResponse`. The error bodies stayed a bare
+ * `{ error: '<message>' }` while the dispatcher's `/i18n` domain — the OTHER
+ * provider of these same three shapes — emitted
+ * `{ success: false, error: { … } }` for the identical condition. Same SDK
+ * method, two error shapes, decided by which plugin mounted the route.
+ *
+ * Both directions are guarded: every error branch is driven and parsed against
+ * the real `BaseResponseSchema`, and the module source is scanned so a new
+ * route cannot quietly reintroduce the bare shape.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { readFileSync } from 'node:fs';
+import { BaseResponseSchema } from '@objectstack/spec/api';
+import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts';
+import { I18nServicePlugin } from './i18n-service-plugin';
+
+const BASE = '/api/v1/i18n';
+
+/**
+ * Mount the plugin's routes through its REAL lifecycle and hand back the
+ * handlers, so these assertions run against the routes the plugin actually
+ * registers rather than a hand-copied table.
+ */
+async function mount(i18nOverrides: Record<string, unknown> = {}) {
+  const routes = new Map<string, RouteHandler>();
+  const server = {
+    get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); },
+    post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(),
+    listen: vi.fn().mockResolvedValue(undefined),
+    close: vi.fn().mockResolvedValue(undefined),
+  };
+  const hooks = new Map<string, Array<(...a: unknown[]) => Promise<void>>>();
+  const services = new Map<string, unknown>();
+  const ctx = {
+    registerService: vi.fn((name: string, svc: unknown) => { services.set(name, svc); }),
+    replaceService: vi.fn(),
+    getService: vi.fn((name: string) => {
+      if (name === 'http-server') return server;
+      if (services.has(name)) return services.get(name);
+      throw new Error(`Service '${name}' not found`);
+    }),
+    getServices: vi.fn(() => new Map()),
+    getKernel: vi.fn(),
+    hook: vi.fn((name: string, h: (...a: unknown[]) => Promise<void>) => {
+      if (!hooks.has(name)) hooks.set(name, []);
+      hooks.get(name)!.push(h);
+    }),
+    trigger: vi.fn(async (name: string, ...a: unknown[]) => {
+      for (const h of hooks.get(name) ?? []) await h(...a);
+    }),
+    logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
+  };
+
+  const plugin = new I18nServicePlugin();
+  await plugin.init!(ctx as never);
+  // Sabotage the registered service where a test needs the `catch` arm — the
+  // routes close over whatever `init` registered.
+  const svc = services.get('i18n') as Record<string, unknown> | undefined;
+  if (svc) Object.assign(svc, i18nOverrides);
+  await plugin.start!(ctx as never);
+  await ctx.trigger('kernel:ready');
+  return routes;
+}
+
+async function drive(
+  routes: Map<string, RouteHandler>,
+  path: string,
+  req: Partial<IHttpRequest> = {},
+): Promise<{ status: number; body: any }> {
+  const handler = routes.get(`GET:${path}`);
+  if (!handler) throw new Error(`no handler for GET ${path} (have: ${[...routes.keys()].join(', ')})`);
+  const captured = { status: 200, body: undefined as any };
+  const res: any = {
+    json(data: any) { captured.body = data; },
+    send() {},
+    status(code: number) { captured.status = code; return res; },
+    header() { return res; },
+  };
+  await handler(
+    { params: {}, query: {}, body: undefined, headers: {}, method: 'GET', path, ...req } as IHttpRequest,
+    res as IHttpResponse,
+  );
+  return captured;
+}
+
+describe('i18n error envelope (#3675)', () => {
+  const CASES: Array<{
+    name: string;
+    status: number;
+    code: string;
+    run: () => Promise<{ status: number; body: any }>;
+  }> = [
+    {
+      name: 'translations without a locale',
+      status: 400,
+      code: 'INVALID_REQUEST',
+      run: async () => drive(await mount(), `${BASE}/translations/:locale`, { params: {} }),
+    },
+    {
+      name: 'labels without an object or locale',
+      status: 400,
+      code: 'INVALID_REQUEST',
+      run: async () => drive(await mount(), `${BASE}/labels/:object/:locale`, { params: {} }),
+    },
+    {
+      name: 'a throwing i18n service on /locales',
+      status: 500,
+      code: 'INTERNAL',
+      run: async () =>
+        drive(
+          await mount({ getLocales: () => { throw new Error('adapter exploded'); } }),
+          `${BASE}/locales`,
+        ),
+    },
+    {
+      name: 'a throwing i18n service on /translations/:locale',
+      status: 500,
+      code: 'INTERNAL',
+      run: async () =>
+        drive(
+          await mount({ getTranslations: () => { throw new Error('adapter exploded'); } }),
+          `${BASE}/translations/:locale`,
+          { params: { locale: 'en' } },
+        ),
+    },
+    {
+      name: 'a throwing i18n service on /labels/:object/:locale',
+      status: 500,
+      code: 'INTERNAL',
+      run: async () =>
+        drive(
+          await mount({
+            getFieldLabels: () => { throw new Error('adapter exploded'); },
+            getTranslations: () => { throw new Error('adapter exploded'); },
+          }),
+          `${BASE}/labels/:object/:locale`,
+          { params: { object: 'customer', locale: 'en' } },
+        ),
+    },
+  ];
+
+  for (const c of CASES) {
+    it(`${c.name} → ${c.status} ${c.code}, in the declared envelope`, async () => {
+      const { status, body } = await c.run();
+      expect(status).toBe(c.status);
+
+      const parsed = BaseResponseSchema.safeParse(body);
+      expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true);
+
+      expect(body.success).toBe(false);
+      expect(body.error.code).toBe(c.code);
+      expect(typeof body.error.message).toBe('string');
+      expect(body.error.message.length).toBeGreaterThan(0);
+
+      // The pre-#3675 shape, explicitly dead.
+      expect(typeof body.error).not.toBe('string');
+    });
+  }
+
+  it('a 500 still carries a message when the thrown value has none', async () => {
+    // `error.message` is REQUIRED by ApiErrorSchema, so passing a raw thrown
+    // value straight through would emit an invalid body for anything that is
+    // not an Error.
+    const routes = await mount({ getLocales: () => { throw 'a bare string'; } });
+    const { status, body } = await drive(routes, `${BASE}/locales`);
+    expect(status).toBe(500);
+    expect(BaseResponseSchema.safeParse(body).success).toBe(true);
+    expect(body.error.message).toBe('Internal error');
+  });
+
+  it('routes every error through `sendError` — no route may reintroduce the bare shape', () => {
+    // Comments stripped first: this module's prose quotes both shapes, and a
+    // doc comment is not a code path.
+    const source = readFileSync(new URL('./i18n-service-plugin.ts', import.meta.url), 'utf8')
+      .replace(/\/\*[\s\S]*?\*\//g, '')
+      .replace(/\/\/[^\n]*/g, '');
+
+    const bare = [...source.matchAll(/res\s*\.\s*status\([^)]*\)\s*\.\s*json\(\s*\{\s*error/g)];
+    expect(bare, `bare error bodies found: ${bare.map((m) => m[0]).join(', ')}`).toHaveLength(0);
+
+    // The envelope is built in exactly one place.
+    expect([...source.matchAll(/success:\s*false/g)]).toHaveLength(1);
+  });
+});