Skip to content

Commit bd68f08

Browse files
os-zhuangclaude
andauthored
fix(service-storage,service-i18n): emit the declared error envelope, not a bare { error } (#3675) (#3687)
* 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`) declaring `{ success: false, error: { code, message } }`. So the same SDK method returned two different error shapes depending on which provider mounted the route. All 32 sites (27 in `storage-routes.ts`, 5 in `i18n-service-plugin.ts`) now go through one `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. The codes moved, and that is the breaking part. `AUTH_REQUIRED`, `ATTACHMENT_DOWNLOAD_DENIED` and `FILE_DOWNLOAD_DENIED` were at `body.code` and are now at `body.error.code`. The SDK is unaffected — it already reads `errorBody?.code || errorBody?.error?.code`, one of 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 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. A 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 that could drift from the schema it claims to check — and scans the module source so a new route cannot quietly reintroduce the bare shape. Without the second half the suite would only ever cover the branches that existed the day it was written. Both halves are mutation-checked. The route ledgers (#3563#3656) could not have caught this: they audit which routes exist and whether the SDK can address them, not what comes back. Recorded as §12 of the audit doc, with the four error dialects found live. Measured and deliberately 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. Now pinned to exactly one deviating field by a test rather than prose. Also unchanged: service-storage's success bodies are still three shapes of their own, none carrying `success: true` — non-additive, so it gets its own issue instead of a quiet ride along with this one. Co-Authored-By: Claude <noreply@anthropic.com> * docs(wire-format): note that service-mounted routes answer in the declared envelope (#3675) `wire-format.mdx` §7 describes the flat `{ error, code }` envelope, which is what the kernel REST server emits for `/api/v1/data/*`. With the storage and i18n routes moved to the declared `BaseResponse` shape, a reader following §7 would look for `body.code` on those routes and find nothing. Says which envelope belongs to which surface, and that SDK callers need not branch because `ObjectStackClient` normalizes both. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c2603b9 commit bd68f08

11 files changed

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

content/docs/api/wire-format.mdx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -384,6 +384,27 @@ Returned when an `If-Match` / `expectedVersion` token no longer matches the stor
384384
**Error Codes:** See the [Error Catalog](/docs/api/error-catalog) for the complete list of error codes and their meanings.
385385
</Callout>
386386

387+
### Service-mounted routes use the declared envelope
388+
389+
The flat envelope above is what the kernel REST server emits for `/api/v1/data/*`.
390+
Routes mounted directly by a service plugin — `/api/v1/storage/*` and
391+
`/api/v1/i18n/*` — instead return the `BaseResponse` shape their contract
392+
declares, with the code **inside** the error object:
393+
394+
```json
395+
{
396+
"success": false,
397+
"error": {
398+
"code": "ATTACHMENT_DOWNLOAD_DENIED",
399+
"message": "You do not have access to a record this file is attached to"
400+
}
401+
}
402+
```
403+
404+
Read `body.error.code`, not `body.code`, on these routes. `ObjectStackClient`
405+
normalizes both — `error.code` and `error.message` are populated whichever
406+
envelope the server used — so SDK callers do not need to branch.
407+
387408
---
388409

389410
## 8. Batch Operations

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,37 @@ capstone also had to prefer exact rows over wildcard families when matching —
271271
otherwise every `/auth/*` URL would still have been absorbed by `* /auth/**`
272272
and the new ledger would have changed nothing.
273273

274+
## 12. What the ledgers cannot see: response bodies (#3675)
275+
276+
Worth recording as a **boundary of this audit family**, not an oversight in it.
277+
Every guard from §1 to §11 answers one question — does this route exist, and
278+
can the SDK address it? None of them looks at what comes back. That is how
279+
service-i18n and service-storage carried green `sdk` rows for surfaces that
280+
emitted a bare `{ error: '<message>' }` against a contract declaring
281+
`{ success: false, error: { code, message } }`.
282+
283+
Four error dialects were live in-repo at the time:
284+
285+
| Producer | Shape |
286+
|---|---|
287+
| `contract.zod.ts` (declared) | `{ success, error: { code: string, message, … } }` |
288+
| `http-dispatcher.ts` | `{ success: false, error: { message, code: <HTTP number>, details } }` |
289+
| `rest-server.ts` | `{ error: <string>, code: <SEMANTIC_STRING> }` |
290+
| `settings-routes.ts`, `share-link-routes.ts` | `{ error: { code, message } }` — no `success` |
291+
| service-i18n, service-storage | `{ error: <string> }`, sometimes `+ code` at the top level |
292+
293+
#3675 moved the last row to the contract. The rest are unchanged, and the
294+
dispatcher's deviation is now pinned to exactly one field (`error.code` carries
295+
the HTTP status where a semantic string is declared) by a test rather than by
296+
prose — it parks the real code in `details` to work around its own occupied
297+
field, which is the tell.
298+
299+
The generalisable lesson matches §11's: a guard only covers the question it
300+
asks. "The route exists" and "the route answers in the declared shape" are two
301+
questions, and the second one needs the contract imported into the assertion —
302+
`BaseResponseSchema.safeParse(body)`, not a hand-copied restatement that drifts
303+
from the schema it claims to check.
304+
274305
## Follow-up slicing (proposed)
275306

276307
1. **`client.actions.invoke(...)`** — closes the largest hole (3 routes).
@@ -286,6 +317,9 @@ and the new ledger would have changed nothing.
286317
11. **Control-plane surface** (§10) — #3655, needs a ledger in the `cloud` repo.
287318
12. **Enumerate `/auth/**`** (§11) — done in #3656; wildcard ratchet 60 → 3.
288319
13. **Enumerate `/ai/**`** — the last dynamic family, 3 SDK methods.
320+
14. **Response-shape conformance** (§12) — error path done in #3675 for both
321+
services; the storage **success** bodies (three shapes, none carrying
322+
`success: true`) and the dispatcher's numeric `error.code` remain.
289323

290324
Each gap closed must flip its ledger row to `sdk` and lower the ratchet bound
291325
in the conformance test — the guard enforces both directions from PR-1 onward.

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ describe('attachments permission matrix (#2755)', () => {
157157
body: JSON.stringify({ filename: 'x.txt', mimeType: 'text/plain', size: 1, scope: 'attachments' }),
158158
});
159159
expect(res.status).toBe(401);
160-
expect(((await res.json()) as any).code).toBe('AUTH_REQUIRED');
160+
expect(((await res.json()) as any).error?.code).toBe('AUTH_REQUIRED');
161161
});
162162

163163
it('(e) authenticated upload succeeds and sys_file.owner_id is server-stamped', async () => {
@@ -319,13 +319,13 @@ describe('attachments permission matrix (#2755)', () => {
319319
// Anonymous → 401 (was a 200 capability URL before #2970).
320320
const anon = await stack.api(`/storage/files/${adminFile}/url`);
321321
expect(anon.status).toBe(401);
322-
expect(((await anon.json()) as any).code).toBe('AUTH_REQUIRED');
322+
expect(((await anon.json()) as any).error?.code).toBe('AUTH_REQUIRED');
323323

324324
// memberB is authenticated but cannot read att_secret and is not the
325325
// owner → 403.
326326
const denied = await stack.apiAs(memberBTok, 'GET', `/storage/files/${adminFile}/url`);
327327
expect(denied.status).toBe(403);
328-
expect(((await denied.json()) as any).code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
328+
expect(((await denied.json()) as any).error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED');
329329

330330
// The owner (admin) → 200 with a signed URL.
331331
const owner = await stack.apiAs(adminTok, 'GET', `/storage/files/${adminFile}/url`);

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
import { describe, it, expect, vi, beforeEach } from 'vitest';
33
import { HttpDispatcher } from './http-dispatcher.js';
44
import { ObjectKernel } from '@objectstack/core';
5+
import { ApiErrorSchema } from '@objectstack/spec/api';
56

67
describe('HttpDispatcher', () => {
78
let kernel: ObjectKernel;
@@ -1837,6 +1838,35 @@ describe('HttpDispatcher', () => {
18371838
expect(result.response?.body?.error?.message).toBe('Missing locale parameter');
18381839
});
18391840

1841+
/**
1842+
* The dispatcher's error body is the SHAPE the autonomously-mounted
1843+
* i18n/storage services were aligned to in #3675 — nested `error`, with
1844+
* the `success` flag. It is not yet the CONTRACT: `ApiErrorSchema`
1845+
* declares `code` as a semantic string ('validation_error'), and this
1846+
* emits the HTTP status as a number. The dispatcher already works
1847+
* around its own field being occupied — `this.error(msg, 403, { code:
1848+
* 'PERMISSION_DENIED' })` parks the real code in `details` — which is
1849+
* the tell that the number is in the wrong place.
1850+
*
1851+
* Pinned rather than fixed: `error.code` is read across the SDK, the
1852+
* console and the dogfood suite, so moving it is its own change.
1853+
* `toEqual(['code'])` is deliberately exact — if a second field starts
1854+
* deviating this fails, and if the dispatcher is fixed it also fails
1855+
* and this pin should be deleted.
1856+
*/
1857+
it('pins the ONE field where the dispatcher deviates from ApiErrorSchema (#3675)', async () => {
1858+
const result = await dispatcher.handleI18n('/translations', 'GET', {}, { request: {} });
1859+
const body = result.response?.body as { success?: boolean; error?: unknown };
1860+
1861+
expect(body.success).toBe(false);
1862+
expect(typeof body.error).toBe('object');
1863+
1864+
const parsed = ApiErrorSchema.safeParse(body.error);
1865+
expect(parsed.success).toBe(false);
1866+
expect(parsed.error!.issues.map((i) => i.path.join('.'))).toEqual(['code']);
1867+
expect((body.error as { code: unknown }).code).toBe(400);
1868+
});
1869+
18401870
it('should fallback to deriving labels from translations when getFieldLabels is missing', async () => {
18411871
delete mockI18nService.getFieldLabels;
18421872
mockI18nService.getTranslations.mockReturnValue({
Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* Error-envelope conformance for the autonomously-mounted `/api/v1/i18n/*`
5+
* routes (#3675) — the error-path twin of the success-path fix in #3636.
6+
*
7+
* #3636 aligned the SUCCESS bodies because those were the ones breaking
8+
* `ObjectStackClient.unwrapResponse`. The error bodies stayed a bare
9+
* `{ error: '<message>' }` while the dispatcher's `/i18n` domain — the OTHER
10+
* provider of these same three shapes — emitted
11+
* `{ success: false, error: { … } }` for the identical condition. Same SDK
12+
* method, two error shapes, decided by which plugin mounted the route.
13+
*
14+
* Both directions are guarded: every error branch is driven and parsed against
15+
* the real `BaseResponseSchema`, and the module source is scanned so a new
16+
* route cannot quietly reintroduce the bare shape.
17+
*/
18+
19+
import { describe, it, expect, vi } from 'vitest';
20+
import { readFileSync } from 'node:fs';
21+
import { BaseResponseSchema } from '@objectstack/spec/api';
22+
import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts';
23+
import { I18nServicePlugin } from './i18n-service-plugin';
24+
25+
const BASE = '/api/v1/i18n';
26+
27+
/**
28+
* Mount the plugin's routes through its REAL lifecycle and hand back the
29+
* handlers, so these assertions run against the routes the plugin actually
30+
* registers rather than a hand-copied table.
31+
*/
32+
async function mount(i18nOverrides: Record<string, unknown> = {}) {
33+
const routes = new Map<string, RouteHandler>();
34+
const server = {
35+
get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); },
36+
post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(),
37+
listen: vi.fn().mockResolvedValue(undefined),
38+
close: vi.fn().mockResolvedValue(undefined),
39+
};
40+
const hooks = new Map<string, Array<(...a: unknown[]) => Promise<void>>>();
41+
const services = new Map<string, unknown>();
42+
const ctx = {
43+
registerService: vi.fn((name: string, svc: unknown) => { services.set(name, svc); }),
44+
replaceService: vi.fn(),
45+
getService: vi.fn((name: string) => {
46+
if (name === 'http-server') return server;
47+
if (services.has(name)) return services.get(name);
48+
throw new Error(`Service '${name}' not found`);
49+
}),
50+
getServices: vi.fn(() => new Map()),
51+
getKernel: vi.fn(),
52+
hook: vi.fn((name: string, h: (...a: unknown[]) => Promise<void>) => {
53+
if (!hooks.has(name)) hooks.set(name, []);
54+
hooks.get(name)!.push(h);
55+
}),
56+
trigger: vi.fn(async (name: string, ...a: unknown[]) => {
57+
for (const h of hooks.get(name) ?? []) await h(...a);
58+
}),
59+
logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
60+
};
61+
62+
const plugin = new I18nServicePlugin();
63+
await plugin.init!(ctx as never);
64+
// Sabotage the registered service where a test needs the `catch` arm — the
65+
// routes close over whatever `init` registered.
66+
const svc = services.get('i18n') as Record<string, unknown> | undefined;
67+
if (svc) Object.assign(svc, i18nOverrides);
68+
await plugin.start!(ctx as never);
69+
await ctx.trigger('kernel:ready');
70+
return routes;
71+
}
72+
73+
async function drive(
74+
routes: Map<string, RouteHandler>,
75+
path: string,
76+
req: Partial<IHttpRequest> = {},
77+
): Promise<{ status: number; body: any }> {
78+
const handler = routes.get(`GET:${path}`);
79+
if (!handler) throw new Error(`no handler for GET ${path} (have: ${[...routes.keys()].join(', ')})`);
80+
const captured = { status: 200, body: undefined as any };
81+
const res: any = {
82+
json(data: any) { captured.body = data; },
83+
send() {},
84+
status(code: number) { captured.status = code; return res; },
85+
header() { return res; },
86+
};
87+
await handler(
88+
{ params: {}, query: {}, body: undefined, headers: {}, method: 'GET', path, ...req } as IHttpRequest,
89+
res as IHttpResponse,
90+
);
91+
return captured;
92+
}
93+
94+
describe('i18n error envelope (#3675)', () => {
95+
const CASES: Array<{
96+
name: string;
97+
status: number;
98+
code: string;
99+
run: () => Promise<{ status: number; body: any }>;
100+
}> = [
101+
{
102+
name: 'translations without a locale',
103+
status: 400,
104+
code: 'INVALID_REQUEST',
105+
run: async () => drive(await mount(), `${BASE}/translations/:locale`, { params: {} }),
106+
},
107+
{
108+
name: 'labels without an object or locale',
109+
status: 400,
110+
code: 'INVALID_REQUEST',
111+
run: async () => drive(await mount(), `${BASE}/labels/:object/:locale`, { params: {} }),
112+
},
113+
{
114+
name: 'a throwing i18n service on /locales',
115+
status: 500,
116+
code: 'INTERNAL',
117+
run: async () =>
118+
drive(
119+
await mount({ getLocales: () => { throw new Error('adapter exploded'); } }),
120+
`${BASE}/locales`,
121+
),
122+
},
123+
{
124+
name: 'a throwing i18n service on /translations/:locale',
125+
status: 500,
126+
code: 'INTERNAL',
127+
run: async () =>
128+
drive(
129+
await mount({ getTranslations: () => { throw new Error('adapter exploded'); } }),
130+
`${BASE}/translations/:locale`,
131+
{ params: { locale: 'en' } },
132+
),
133+
},
134+
{
135+
name: 'a throwing i18n service on /labels/:object/:locale',
136+
status: 500,
137+
code: 'INTERNAL',
138+
run: async () =>
139+
drive(
140+
await mount({
141+
getFieldLabels: () => { throw new Error('adapter exploded'); },
142+
getTranslations: () => { throw new Error('adapter exploded'); },
143+
}),
144+
`${BASE}/labels/:object/:locale`,
145+
{ params: { object: 'customer', locale: 'en' } },
146+
),
147+
},
148+
];
149+
150+
for (const c of CASES) {
151+
it(`${c.name}${c.status} ${c.code}, in the declared envelope`, async () => {
152+
const { status, body } = await c.run();
153+
expect(status).toBe(c.status);
154+
155+
const parsed = BaseResponseSchema.safeParse(body);
156+
expect(parsed.success, `body is not a BaseResponse: ${JSON.stringify(body)}`).toBe(true);
157+
158+
expect(body.success).toBe(false);
159+
expect(body.error.code).toBe(c.code);
160+
expect(typeof body.error.message).toBe('string');
161+
expect(body.error.message.length).toBeGreaterThan(0);
162+
163+
// The pre-#3675 shape, explicitly dead.
164+
expect(typeof body.error).not.toBe('string');
165+
});
166+
}
167+
168+
it('a 500 still carries a message when the thrown value has none', async () => {
169+
// `error.message` is REQUIRED by ApiErrorSchema, so passing a raw thrown
170+
// value straight through would emit an invalid body for anything that is
171+
// not an Error.
172+
const routes = await mount({ getLocales: () => { throw 'a bare string'; } });
173+
const { status, body } = await drive(routes, `${BASE}/locales`);
174+
expect(status).toBe(500);
175+
expect(BaseResponseSchema.safeParse(body).success).toBe(true);
176+
expect(body.error.message).toBe('Internal error');
177+
});
178+
179+
it('routes every error through `sendError` — no route may reintroduce the bare shape', () => {
180+
// Comments stripped first: this module's prose quotes both shapes, and a
181+
// doc comment is not a code path.
182+
const source = readFileSync(new URL('./i18n-service-plugin.ts', import.meta.url), 'utf8')
183+
.replace(/\/\*[\s\S]*?\*\//g, '')
184+
.replace(/\/\/[^\n]*/g, '');
185+
186+
const bare = [...source.matchAll(/res\s*\.\s*status\([^)]*\)\s*\.\s*json\(\s*\{\s*error/g)];
187+
expect(bare, `bare error bodies found: ${bare.map((m) => m[0]).join(', ')}`).toHaveLength(0);
188+
189+
// The envelope is built in exactly one place.
190+
expect([...source.matchAll(/success:\s*false/g)]).toHaveLength(1);
191+
});
192+
});

0 commit comments

Comments
 (0)