From bc2ab0ed1670f62e7734d250440a2f0c3057ec56 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 14:28:38 +0000 Subject: [PATCH 1/2] fix(service-storage,service-i18n): emit the declared error envelope, not a bare `{ error }` (#3675) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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: '' }` — 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 --- .../service-error-envelope-conformance.md | 50 ++++ ...026-07-dispatcher-client-route-coverage.md | 34 +++ ...achments-permission-matrix.dogfood.test.ts | 6 +- packages/runtime/src/http-dispatcher.test.ts | 30 ++ .../src/error-envelope.conformance.test.ts | 192 ++++++++++++ .../src/i18n-service-plugin.test.ts | 10 +- .../service-i18n/src/i18n-service-plugin.ts | 36 ++- .../src/error-envelope.conformance.test.ts | 283 ++++++++++++++++++ .../src/storage-routes.test.ts | 10 +- .../service-storage/src/storage-routes.ts | 106 ++++--- 10 files changed, 702 insertions(+), 55 deletions(-) create mode 100644 .changeset/service-error-envelope-conformance.md create mode 100644 packages/services/service-i18n/src/error-envelope.conformance.test.ts create mode 100644 packages/services/service-storage/src/error-envelope.conformance.test.ts diff --git a/.changeset/service-error-envelope-conformance.md b/.changeset/service-error-envelope-conformance.md new file mode 100644 index 0000000000..c7a050d892 --- /dev/null +++ b/.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: '' }` — +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. diff --git a/docs/audits/2026-07-dispatcher-client-route-coverage.md b/docs/audits/2026-07-dispatcher-client-route-coverage.md index e4c67b687b..6cee8a9412 100644 --- a/docs/audits/2026-07-dispatcher-client-route-coverage.md +++ b/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: '' }` 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: , details } }` | +| `rest-server.ts` | `{ error: , code: }` | +| `settings-routes.ts`, `share-link-routes.ts` | `{ error: { code, message } }` — no `success` | +| service-i18n, service-storage | `{ error: }`, 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. diff --git a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts b/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts index a12a71b2b8..b3c8dc252b 100644 --- a/packages/qa/dogfood/test/attachments-permission-matrix.dogfood.test.ts +++ b/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`); diff --git a/packages/runtime/src/http-dispatcher.test.ts b/packages/runtime/src/http-dispatcher.test.ts index 6634547ad4..3b5c5b9678 100644 --- a/packages/runtime/src/http-dispatcher.test.ts +++ b/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({ diff --git a/packages/services/service-i18n/src/error-envelope.conformance.test.ts b/packages/services/service-i18n/src/error-envelope.conformance.test.ts new file mode 100644 index 0000000000..833653c58c --- /dev/null +++ b/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: '' }` 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 = {}) { + const routes = new Map(); + 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 Promise>>(); + const services = new Map(); + 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) => { + 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 | 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, + path: string, + req: Partial = {}, +): 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); + }); +}); diff --git a/packages/services/service-i18n/src/i18n-service-plugin.test.ts b/packages/services/service-i18n/src/i18n-service-plugin.test.ts index a3069c9891..2c7a0f2206 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.test.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.test.ts @@ -234,7 +234,10 @@ describe('I18nServicePlugin', () => { await handler(req, res); expect(res._status).toBe(400); - expect(res._data).toEqual({ error: 'Missing locale parameter' }); + expect(res._data).toEqual({ + success: false, + error: { code: 'INVALID_REQUEST', message: 'Missing locale parameter' }, + }); }); it('GET /labels/:object/:locale should derive field labels from translation bundle', async () => { @@ -266,7 +269,10 @@ describe('I18nServicePlugin', () => { await handler(req, res); expect(res._status).toBe(400); - expect(res._data).toEqual({ error: 'Missing object or locale parameter' }); + expect(res._data).toEqual({ + success: false, + error: { code: 'INVALID_REQUEST', message: 'Missing object or locale parameter' }, + }); }); }); diff --git a/packages/services/service-i18n/src/i18n-service-plugin.ts b/packages/services/service-i18n/src/i18n-service-plugin.ts index ae69b64475..36c7ac4892 100644 --- a/packages/services/service-i18n/src/i18n-service-plugin.ts +++ b/packages/services/service-i18n/src/i18n-service-plugin.ts @@ -7,6 +7,27 @@ import type { II18nService } from '@objectstack/spec/contracts'; 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 } }); +} + /** * Configuration options for the I18nServicePlugin. */ @@ -144,6 +165,11 @@ export class I18nServicePlugin implements Plugin { * returning the declared unwrapped shape against the dispatcher. Same * method, two shapes, decided by which plugin happened to mount the route * (#3636). `data` did not move, so direct body readers are unaffected. + * + * Error bodies now carry the matching `{ success: false, error: { code, + * message } }` half via `sendError` (#3675). That was left out of #3636 + * because only the success bodies broke `unwrapResponse`, and unlike the + * success fix it is NOT additive — `error` goes from string to object. */ private registerI18nRoutes(httpServer: IHttpServer, ctx: PluginContext): void { if (!this.i18n) return; @@ -167,7 +193,7 @@ export class I18nServicePlugin implements Plugin { }, }); } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); @@ -176,13 +202,13 @@ export class I18nServicePlugin implements Plugin { try { const locale = req.params.locale; if (!locale) { - res.status(400).json({ error: 'Missing locale parameter' }); + sendError(res, 400, 'INVALID_REQUEST', 'Missing locale parameter'); return; } const translations = i18n.getTranslations(locale); res.json({ success: true, data: { locale, translations } }); } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); @@ -192,7 +218,7 @@ export class I18nServicePlugin implements Plugin { const objectName = req.params.object; const locale = req.params.locale; if (!objectName || !locale) { - res.status(400).json({ error: 'Missing object or locale parameter' }); + sendError(res, 400, 'INVALID_REQUEST', 'Missing object or locale parameter'); return; } // Some implementations may provide a dedicated getFieldLabels method @@ -215,7 +241,7 @@ export class I18nServicePlugin implements Plugin { res.json({ success: true, data: { object: objectName, locale, labels } }); } } catch (error: any) { - res.status(500).json({ error: error.message }); + sendError(res, 500, 'INTERNAL', error?.message ?? 'Internal error'); } }); diff --git a/packages/services/service-storage/src/error-envelope.conformance.test.ts b/packages/services/service-storage/src/error-envelope.conformance.test.ts new file mode 100644 index 0000000000..a6411f9723 --- /dev/null +++ b/packages/services/service-storage/src/error-envelope.conformance.test.ts @@ -0,0 +1,283 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Error-envelope conformance for the autonomously-mounted `/api/v1/storage/*` + * routes (#3675). + * + * The route ledgers (#3563 → #3656) audit which routes EXIST and whether the + * SDK can address them. They say nothing about what comes back, which is how + * these routes carried green `sdk` rows while emitting a bare + * `{ error: '' }` — with the code, where one existed at all, as a + * SIBLING of `error` rather than a field of it — against a contract that + * declares `{ success: false, error: { code, message } }`. + * + * This file closes that gap for the error path, in two directions: + * 1. every distinct error branch is DRIVEN and its body parsed against the + * real `BaseResponseSchema` from `packages/spec` — not a local copy, so + * the assertion tracks the contract if the contract moves; + * 2. the module source is scanned so a NEW route cannot quietly reintroduce + * the bare shape. Without (2) this suite only ever covers the branches + * that existed the day it was written. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { BaseResponseSchema } from '@objectstack/spec/api'; +import type { IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { LocalStorageAdapter } from './local-storage-adapter'; +import { StorageMetadataStore } from './metadata-store'; +import { registerStorageRoutes } from './storage-routes'; + +const BASE = '/api/v1/storage'; + +interface Captured { + status: number; + body: any; +} + +function mount(storage: any, store: any, routeOpts: any = {}) { + const routes = new Map(); + const http = { + get: (p: string, h: RouteHandler) => { routes.set(`GET:${p}`, h); }, + post: (p: string, h: RouteHandler) => { routes.set(`POST:${p}`, h); }, + put: (p: string, h: RouteHandler) => { routes.set(`PUT:${p}`, h); }, + delete: () => {}, + patch: () => {}, + use: () => {}, + listen: async () => {}, + close: async () => {}, + }; + registerStorageRoutes(http as any, storage, store, { basePath: BASE, ...routeOpts }); + return routes; +} + +async function drive( + routes: Map, + method: string, + path: string, + req: Partial = {}, +): Promise { + const handler = routes.get(`${method}:${path}`); + if (!handler) throw new Error(`no handler for ${method} ${path}`); + const captured: Captured = { status: 200, body: undefined }; + 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, path, ...req } as IHttpRequest, + res as IHttpResponse, + ); + return captured; +} + +/** A store whose every read throws — drives the `catch` arms. */ +const explodingStore = { + getFile: async () => { throw new Error('boom'); }, + getSession: async () => { throw new Error('boom'); }, + createFile: async () => { throw new Error('boom'); }, +} as any; + +async function tmpAdapter(): Promise { + const rootDir = join(tmpdir(), `os-err-env-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + return new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); +} + +async function committedAttachment(store: StorageMetadataStore, id: string, extra: any = {}) { + await store.createFile({ + id, + key: `attachments/${id}.bin`, + name: 'x.bin', + status: 'committed', + acl: 'private', + scope: 'attachments', + ...extra, + } as any); +} + +describe('storage error envelope (#3675)', () => { + /** + * One entry per distinct `code` the routes can emit. `expected` is asserted + * exactly, so retiring a code without retiring its case fails here rather + * than silently shrinking coverage. + */ + const CASES: Array<{ name: string; status: number; code: string; run: () => Promise }> = [ + { + name: 'missing required upload fields', + status: 400, + code: 'INVALID_REQUEST', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/presigned`, { body: {} }); + }, + }, + { + name: 'completing an upload for an unknown file', + status: 404, + code: 'FILE_NOT_FOUND', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/complete`, { body: { fileId: 'nope' } }); + }, + }, + { + name: 'completing an unknown chunked session', + status: 404, + code: 'UPLOAD_SESSION_NOT_FOUND', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'POST', `${BASE}/upload/chunked/:uploadId/complete`, { + params: { uploadId: 'nope' }, + }); + }, + }, + { + name: 'chunk upload with the wrong resume token', + status: 403, + code: 'INVALID_RESUME_TOKEN', + run: async () => { + const store = new StorageMetadataStore(null); + await store.createSession({ + id: 'sess-1', + file_id: 'f-1', + key: 'user/f-1.bin', + filename: 'f.bin', + mime_type: 'application/octet-stream', + total_size: 10, + chunk_size: 5, + total_chunks: 2, + resume_token: 'the-real-token', + status: 'in_progress', + } as any); + const routes = mount(await tmpAdapter(), store); + return drive(routes, 'PUT', `${BASE}/upload/chunked/:uploadId/chunk/:chunkIndex`, { + params: { uploadId: 'sess-1', chunkIndex: '0' }, + headers: { 'x-resume-token': 'wrong' }, + }); + }, + }, + { + name: 'anonymous upload when a session resolver is wired', + status: 401, + code: 'AUTH_REQUIRED', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null), { + resolveSession: async () => null, + }); + return drive(routes, 'POST', `${BASE}/upload/presigned`, { body: {} }); + }, + }, + { + name: 'denied download of an attachments-scope file', + status: 403, + code: 'ATTACHMENT_DOWNLOAD_DENIED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'a1'); + const routes = mount(await tmpAdapter(), store, { authorizeFileRead: async () => 'deny' }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'a1' } }); + }, + }, + { + name: 'denied download of a field-owned file', + status: 403, + code: 'FILE_DOWNLOAD_DENIED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'fo1', { + scope: 'user', + key: 'user/fo1.png', + ref_object: 'product', + ref_id: 'p1', + }); + const routes = mount(await tmpAdapter(), store, { authorizeFileRead: async () => 'deny' }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'fo1' } }); + }, + }, + { + name: 'unauthenticated download of an attachments-scope file', + status: 401, + code: 'AUTH_REQUIRED', + run: async () => { + const store = new StorageMetadataStore(null); + await committedAttachment(store, 'a2'); + const routes = mount(await tmpAdapter(), store, { + authorizeFileRead: async () => 'unauthenticated', + }); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'a2' } }); + }, + }, + { + name: 'raw upload against an adapter with no token support', + status: 501, + code: 'NOT_IMPLEMENTED', + run: async () => { + const routes = mount({} as any, new StorageMetadataStore(null)); + return drive(routes, 'PUT', `${BASE}/_local/raw/:token`, { params: { token: 'x' } }); + }, + }, + { + name: 'raw download with a forged token signature', + status: 403, + code: 'INVALID_TOKEN', + run: async () => { + const routes = mount(await tmpAdapter(), new StorageMetadataStore(null)); + return drive(routes, 'GET', `${BASE}/_local/raw/:token`, { params: { token: 'YWJj.ZGVm' } }); + }, + }, + { + name: 'an unexpected throw from the metadata store', + status: 500, + code: 'INTERNAL', + run: async () => { + const routes = mount(await tmpAdapter(), explodingStore); + return drive(routes, 'GET', `${BASE}/files/:fileId/url`, { params: { fileId: 'whatever' } }); + }, + }, + ]; + + 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); + + // The contract itself, imported — not a restatement of it. + 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 shapes, explicitly dead: `error` is no longer a bare + // string, and the code is no longer a SIBLING of `error`. + expect(typeof body.error).not.toBe('string'); + expect(body.code).toBeUndefined(); + }); + } + + it('routes every error through `sendError` — no route may reintroduce the bare shape', () => { + // Comments stripped first: this file's own prose quotes both the old and + // the new shape, and a doc comment is not a code path. + const source = readFileSync(new URL('./storage-routes.ts', import.meta.url), 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, ''); + + // `res.status(…).json({ error…` was the bare-shape idiom. Any reappearance + // means a new route bypassed the helper. + 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); + + // And the helper is the only place that builds one, so the shape lives in + // exactly one line of this package. + const builders = [...source.matchAll(/success:\s*false/g)]; + expect(builders).toHaveLength(1); + }); +}); diff --git a/packages/services/service-storage/src/storage-routes.test.ts b/packages/services/service-storage/src/storage-routes.test.ts index e63ddf923e..408ca9ce66 100644 --- a/packages/services/service-storage/src/storage-routes.test.ts +++ b/packages/services/service-storage/src/storage-routes.test.ts @@ -291,7 +291,7 @@ describe('Storage REST Routes', () => { for (const p of ['/api/v1/storage/files/:fileId/url', '/api/v1/storage/files/:fileId']) { const res = await hit(server, p, 'a1'); expect(res._status, p).toBe(401); - expect(res._json?.code, p).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code, p).toBe('AUTH_REQUIRED'); } }); @@ -300,7 +300,7 @@ describe('Storage REST Routes', () => { await commit(s, { id: 'a2' }); const res = await hit(server, '/api/v1/storage/files/:fileId/url', 'a2'); expect(res._status).toBe(403); - expect(res._json?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); + expect(res._json?.error?.code).toBe('ATTACHMENT_DOWNLOAD_DENIED'); }); it('allows the download when authorized, minting a short-lived signed URL', async () => { @@ -354,7 +354,7 @@ describe('Storage REST Routes', () => { expect(res._status).toBe(403); // Distinct from the attachments code — this file BELONGS to one // record, it is not attached to several. - expect(res._json?.code).toBe('FILE_DOWNLOAD_DENIED'); + expect(res._json?.error?.code).toBe('FILE_DOWNLOAD_DENIED'); expect(authorizeFileRead).toHaveBeenCalledOnce(); }); @@ -363,7 +363,7 @@ describe('Storage REST Routes', () => { await commit(s, owned('fo2')); const res = await hit(server, '/api/v1/storage/files/:fileId', 'fo2'); expect(res._status).toBe(401); - expect(res._json?.code).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code).toBe('AUTH_REQUIRED'); }); it('allows an authorized field-owned download', async () => { @@ -466,7 +466,7 @@ describe('Storage REST Routes', () => { const res = createMockRes(); await server._getHandler(method, path)!(createMockReq(reqOpts as any), res); expect(res._status, `${method} ${path}`).toBe(401); - expect(res._json?.code, `${method} ${path}`).toBe('AUTH_REQUIRED'); + expect(res._json?.error?.code, `${method} ${path}`).toBe('AUTH_REQUIRED'); } }); diff --git a/packages/services/service-storage/src/storage-routes.ts b/packages/services/service-storage/src/storage-routes.ts index a57db99528..4ca9ce7912 100644 --- a/packages/services/service-storage/src/storage-routes.ts +++ b/packages/services/service-storage/src/storage-routes.ts @@ -9,6 +9,28 @@ 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. + * + * Only the ERROR path is normalized here. The success bodies are still three + * shapes of their own (`{ data }`, bare `{ url }`, `{ ok, key }`, none with + * `success: true`) — a separate, non-additive change, tracked as its own + * issue rather than smuggled into this one. + */ +function sendError(res: IHttpResponse, status: number, code: string, message: string): void { + res.status(status).json({ success: false, error: { code, message } }); +} + /** * Options for the storage route registration helper. */ @@ -117,21 +139,15 @@ export function registerStorageRoutes( verdict = 'deny'; // a failed authz check must never fall open } if (verdict === 'unauthenticated') { - res.status(401).json({ error: 'Authentication required to download this file', code: 'AUTH_REQUIRED' }); + sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to download this file'); return false; } if (verdict === 'deny') { - res.status(403).json( - fieldOwned - ? { - error: 'You do not have access to the record this file belongs to', - code: 'FILE_DOWNLOAD_DENIED', - } - : { - error: 'You do not have access to a record this file is attached to', - code: 'ATTACHMENT_DOWNLOAD_DENIED', - }, - ); + if (fieldOwned) { + sendError(res, 403, 'FILE_DOWNLOAD_DENIED', 'You do not have access to the record this file belongs to'); + } else { + sendError(res, 403, 'ATTACHMENT_DOWNLOAD_DENIED', 'You do not have access to a record this file is attached to'); + } return false; } return downloadTtl; @@ -161,7 +177,7 @@ export function registerStorageRoutes( session = null; } if (!session?.userId) { - res.status(401).json({ error: 'Authentication required to upload files', code: 'AUTH_REQUIRED' }); + sendError(res, 401, 'AUTH_REQUIRED', 'Authentication required to upload files'); return false; } return session; @@ -176,7 +192,7 @@ export function registerStorageRoutes( if (session === false) return; const { filename, mimeType, size, scope, bucket } = req.body ?? {}; if (!filename || !mimeType || size == null) { - res.status(400).json({ error: 'filename, mimeType, and size are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'filename, mimeType, and size are required'); return; } @@ -225,7 +241,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -237,13 +253,13 @@ export function registerStorageRoutes( if ((await requireUploadSession(req, res)) === false) return; const { fileId, eTag } = req.body ?? {}; if (!fileId) { - res.status(400).json({ error: 'fileId is required' }); + sendError(res, 400, 'INVALID_REQUEST', 'fileId is required'); return; } const file = await store.getFile(fileId); if (!file) { - res.status(404).json({ error: 'File not found' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found'); return; } @@ -268,7 +284,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -281,7 +297,7 @@ export function registerStorageRoutes( if (session === false) return; const { filename, mimeType, totalSize, chunkSize: reqChunkSize, scope, bucket, metadata } = req.body ?? {}; if (!filename || !mimeType || !totalSize) { - res.status(400).json({ error: 'filename, mimeType, and totalSize are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'filename, mimeType, and totalSize are required'); return; } @@ -349,7 +365,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -362,20 +378,20 @@ export function registerStorageRoutes( const { uploadId, chunkIndex: chunkIndexStr } = req.params; const chunkIndex = parseInt(chunkIndexStr, 10); if (!uploadId || isNaN(chunkIndex)) { - res.status(400).json({ error: 'uploadId and chunkIndex are required' }); + sendError(res, 400, 'INVALID_REQUEST', 'uploadId and chunkIndex are required'); return; } const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } // Verify resume token const token = (req.headers['x-resume-token'] ?? '') as string; if (session.resume_token && token !== session.resume_token) { - res.status(403).json({ error: 'Invalid resume token' }); + sendError(res, 403, 'INVALID_RESUME_TOKEN', 'Invalid resume token'); return; } @@ -388,7 +404,7 @@ export function registerStorageRoutes( } else if (req.body instanceof ArrayBuffer) { data = Buffer.from(req.body); } else { - res.status(400).json({ error: 'Binary body required' }); + sendError(res, 400, 'INVALID_REQUEST', 'Binary body required'); return; } @@ -417,7 +433,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -430,7 +446,7 @@ export function registerStorageRoutes( const { uploadId } = req.params; const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } @@ -461,7 +477,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -474,7 +490,7 @@ export function registerStorageRoutes( const { uploadId } = req.params; const session = await store.getSession(uploadId); if (!session) { - res.status(404).json({ error: 'Upload session not found' }); + sendError(res, 404, 'UPLOAD_SESSION_NOT_FOUND', 'Upload session not found'); return; } @@ -500,7 +516,7 @@ export function registerStorageRoutes( }, }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -512,7 +528,7 @@ export function registerStorageRoutes( const { fileId } = req.params; const file = await store.getFile(fileId); if (!file || file.status !== 'committed') { - res.status(404).json({ error: 'File not found or not committed' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); return; } @@ -532,7 +548,7 @@ export function registerStorageRoutes( res.json({ url }); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -552,7 +568,7 @@ export function registerStorageRoutes( const { fileId } = req.params; const file = await store.getFile(fileId); if (!file || file.status !== 'committed') { - res.status(404).json({ error: 'File not found or not committed' }); + sendError(res, 404, 'FILE_NOT_FOUND', 'File not found or not committed'); return; } @@ -572,7 +588,7 @@ export function registerStorageRoutes( res.status(302).header('Location', url).send(''); } catch (err: any) { - res.status(500).json({ error: err.message ?? 'Internal error' }); + sendError(res, 500, 'INTERNAL', err?.message ?? 'Internal error'); } }); @@ -584,7 +600,7 @@ export function registerStorageRoutes( const { token } = req.params; const localAdapter = storage as LocalStorageAdapter; if (!localAdapter.verifyToken) { - res.status(501).json({ error: 'Presigned raw upload not supported by this adapter' }); + sendError(res, 501, 'NOT_IMPLEMENTED', 'Presigned raw upload not supported by this adapter'); return; } @@ -595,15 +611,20 @@ export function registerStorageRoutes( } else if (Buffer.isBuffer(req.body)) { data = req.body; } else { - res.status(400).json({ error: 'Binary body required' }); + sendError(res, 400, 'INVALID_REQUEST', 'Binary body required'); return; } await storage.upload(payload.k, data, { contentType: payload.ct }); res.json({ ok: true, key: payload.k }); } catch (err: any) { - const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500; - res.status(statusCode).json({ error: err.message ?? 'Upload failed' }); + const invalidToken = err?.message?.includes('expired') || err?.message?.includes('signature'); + sendError( + res, + invalidToken ? 403 : 500, + invalidToken ? 'INVALID_TOKEN' : 'INTERNAL', + err?.message ?? 'Upload failed', + ); } }); @@ -615,7 +636,7 @@ export function registerStorageRoutes( const { token } = req.params; const localAdapter = storage as LocalStorageAdapter; if (!localAdapter.verifyToken) { - res.status(501).json({ error: 'Presigned download not supported by this adapter' }); + sendError(res, 501, 'NOT_IMPLEMENTED', 'Presigned download not supported by this adapter'); return; } @@ -631,8 +652,13 @@ export function registerStorageRoutes( } res.send(data); } catch (err: any) { - const statusCode = err.message?.includes('expired') || err.message?.includes('signature') ? 403 : 500; - res.status(statusCode).json({ error: err.message ?? 'Download failed' }); + const invalidToken = err?.message?.includes('expired') || err?.message?.includes('signature'); + sendError( + res, + invalidToken ? 403 : 500, + invalidToken ? 'INVALID_TOKEN' : 'INTERNAL', + err?.message ?? 'Download failed', + ); } }); } From c43c89e3be8d5398d986ebd1bfeacd41d2e45129 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 14:31:56 +0000 Subject: [PATCH 2/2] docs(wire-format): note that service-mounted routes answer in the declared envelope (#3675) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- content/docs/api/wire-format.mdx | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/content/docs/api/wire-format.mdx b/content/docs/api/wire-format.mdx index c64d36f46a..55db92e101 100644 --- a/content/docs/api/wire-format.mdx +++ b/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. +### 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