From c92e05778f0ff9abe6f78ce590b213abebad3b28 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:51:21 +0000 Subject: [PATCH] fix(spec): declare the external-federation error family's HTTP status (#7739) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A write to a read-only federated external object was refused correctly — `ExternalWriteForbiddenError` names the datasource, its `schemaMode` and both flags that would be required, and nothing is applied — but reached the client as a bare 500 INTERNAL_ERROR with no `code`, indistinguishable from a crash. The `code` was never the missing half: `EXTERNAL_WRITE_FORBIDDEN` is already registered in the ADR-0112 error-code ledger. The missing half was an HTTP status, which no exit can invent — so the refusal fell past every structured branch of `mapDataError` and left through the terminal `UNCLASSIFIED_FAULT`, which sanitises to 500 and ships no code the producer never declared. Fixed at the producer, for the whole `EXTERNAL_ERROR_CODES` family at once: new `EXTERNAL_ERROR_HTTP_STATUS` maps every code to its status, and each error class carries it as `status`. Every HTTP exit in the repo already resolves `status` then `statusCode` (`declaredHttpStatus`, `resolveErrorResponse`, `HttpDispatcher.errorFromThrown`, `dispatcher-plugin.errorResponseBase`, `endpoint-executor`, `domains/actions`, `plugin-hono-server`), so one table fixes every door; a branch in `rest-server.ts` would have fixed one. `rest-server.ts` is untouched. EXTERNAL_WRITE_FORBIDDEN 403 policy refusal, not malformed input EXTERNAL_SCHEMA_MODE_VIOLATION 403 same, for DDL EXTERNAL_SCHEMA_MISMATCH 503 a deployment state, not the caller `satisfies Record` makes a future gate that adds a code without a status a compile error. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0158ZQo7LiHSxGWpYKuPq1wu --- .changeset/external-error-http-status.md | 46 +++ .../external-write-forbidden-envelope.test.ts | 358 ++++++++++++++++++ packages/spec/api-surface/shared.json | 1 + packages/spec/export-origins/shared.json | 1 + .../spec/src/shared/external-errors.test.ts | 58 +++ packages/spec/src/shared/external-errors.ts | 69 ++++ 6 files changed, 533 insertions(+) create mode 100644 .changeset/external-error-http-status.md create mode 100644 packages/rest/src/external-write-forbidden-envelope.test.ts diff --git a/.changeset/external-error-http-status.md b/.changeset/external-error-http-status.md new file mode 100644 index 0000000000..5b1e04df9c --- /dev/null +++ b/.changeset/external-error-http-status.md @@ -0,0 +1,46 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): the external-federation error family declares its HTTP status, so a write refusal stops leaking as a bare 500 (#7739) + +A write to a read-only federated external object was refused correctly on the +server — `ExternalWriteForbiddenError` names the datasource, its `schemaMode`, +and both flags that would be required, and nothing is applied (the gate throws +before the driver is reached) — but it reached the client as a bare +**500 INTERNAL_ERROR with no `code`**, indistinguishable from the server +falling over. + +The `code` existed the whole way down: `EXTERNAL_WRITE_FORBIDDEN` is registered +in the ADR-0112 error-code ledger. What the error did not carry was an HTTP +**status**, and no HTTP exit can invent one — so the refusal fell past every +structured branch of the REST boundary, matched no message heuristic, and left +through the terminal sanitised 500 that drops the `code` with it. + +**The whole family now declares its status, in one table.** New export +`EXTERNAL_ERROR_HTTP_STATUS` (`@objectstack/spec/shared`) maps every +`EXTERNAL_ERROR_CODES` member to the status it is reported with, and each of +the three error classes carries the corresponding value as `status`: + +| code | status | why | +| --- | --- | --- | +| `EXTERNAL_WRITE_FORBIDDEN` | **403** | a policy refusal — the identical body succeeds once `datasource.external.allowWrites` and `object.external.writable` are both on, so 400/422 ("fix your request") would be a lie and 409 promises a retry that cannot help | +| `EXTERNAL_SCHEMA_MODE_VIOLATION` | **403** | the same sentence about DDL: `schemaMode !== 'managed'` forbids it, and no rewritten request changes that | +| `EXTERNAL_SCHEMA_MISMATCH` | **503** | not the caller at all — the deployment's metadata and the remote table diverged, only an operator can reconcile them, and it may clear (the reading `ERR_DATASOURCE_UNAVAILABLE` already gets) | + +Declared at the **producer** rather than in a REST error map, because every +HTTP exit in the framework already resolves `status` then `statusCode` — +`mapDataError`'s `declaredHttpStatus`, `resolveErrorResponse`, +`HttpDispatcher.errorFromThrown`, `dispatcher-plugin.errorResponseBase`, +`endpoint-executor`, `domains/actions`, `plugin-hono-server`. One table fixes +every door at once; a branch in the REST server would have fixed one and left +the runtime dispatcher, the endpoint executor and the CLI answering 500 for the +same throw. `satisfies Record` makes a future gate +that adds a code without a status a compile error, so the family cannot +silently re-acquire a member that leaks as a 500. + +**What callers see now.** `POST /api/v1/data/` against a +read-only federated object answers `403` with `code: +"EXTERNAL_WRITE_FORBIDDEN"` and the refusal detail naming both flags to set, +instead of `500` with no `code`. Behaviour is unchanged — nothing was applied +before and nothing is applied now; only the envelope was wrong. diff --git a/packages/rest/src/external-write-forbidden-envelope.test.ts b/packages/rest/src/external-write-forbidden-envelope.test.ts new file mode 100644 index 0000000000..9c9e486682 --- /dev/null +++ b/packages/rest/src/external-write-forbidden-envelope.test.ts @@ -0,0 +1,358 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7739] A write refused by ADR-0015's Gate 3 reaches the client as a +// `403 EXTERNAL_WRITE_FORBIDDEN` envelope, not as a bare `500 INTERNAL_ERROR`. +// +// --------------------------------------------------------------------------- +// The defect, and where it actually lived +// +// `ExternalWriteForbiddenError` was already correct on every axis QA could see +// from the server side: it named the datasource, its `schemaMode`, and both +// flags that would be required, and it carried a ledgered ADR-0112 `code` +// (`EXTERNAL_WRITE_FORBIDDEN`, registered under `@objectstack/spec`). Nothing +// was applied — the gate throws before the driver is reached at all. +// +// What it did NOT carry was an HTTP status, and no exit can invent one. So the +// refusal fell past every structured branch of `mapDataError`, matched no +// message heuristic, and left through the terminal `UNCLASSIFIED_FAULT` as +// `500 INTERNAL_ERROR` **with no `code`** — the client could not tell a +// deliberate policy refusal from the server falling over, even though the +// discriminator existed the whole way down. +// +// The fix is at the PRODUCER (`packages/spec/src/shared/external-errors.ts`), +// not in a REST error map: the whole `EXTERNAL_ERROR_CODES` family now declares +// its status in one table, `EXTERNAL_ERROR_HTTP_STATUS`, and every HTTP exit in +// this repo already reads `status` → `statusCode` (`declaredHttpStatus` here, +// `resolveErrorResponse`, `HttpDispatcher.errorFromThrown`, +// `dispatcher-plugin.errorResponseBase`, `endpoint-executor`, `domains/actions`, +// `plugin-hono-server`). One mapping, every door — and `rest-server.ts`, a +// known 10k-line conflict site, is untouched by this change. +// +// --------------------------------------------------------------------------- +// Why the fixture is a REAL engine rather than a rejecting mock +// +// The card asks for two facts that a `mockRejectedValue` cannot pair: +// +// 1. the refusal answers 403 with its `code`, and +// 2. **nothing is applied**. +// +// A mock that rejects makes (2) vacuously true, so it would stay green against +// a future "fix" that got the status right by letting the write THROUGH on some +// other path. Here the protocol delegates to a real `ObjectQL` engine over an +// in-memory driver whose store this file can read, so (2) is measured against a +// store that CAN be written — §3's control case writes to it through the very +// same route to prove the fixture is not inert. +// +// --------------------------------------------------------------------------- +// Reverse verification (run before the assertions were finalised) +// +// Direction predicted: RED on the unfixed producer, and RED for the reason the +// card describes rather than an unrelated one. Measured by reverting +// `external-errors.ts` to `origin/main` (`git checkout origin/main -- `) +// and re-running this file: +// +// §1 status → expected 403, received 500 RED +// §1 code → expected 'EXTERNAL_WRITE_FORBIDDEN', received undefined RED +// §2 (update/delete) → same pair, both verbs RED +// §3 "nothing applied" → GREEN both before and after, BY CONSTRUCTION: +// the behaviour was never the bug. It is asserted anyway because it is the +// half that stops a status-only "fix" from passing — see above. +// +// So the file goes red on the defect and its "nothing applied" half is +// deliberately direction-insensitive. Recorded rather than reshaped, since a +// template that demands before-red/after-green on every assertion would have +// had this test lie about what it measures. +// --------------------------------------------------------------------------- + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { ObjectQL } from '@objectstack/objectql'; +import type { IDataDriver } from '@objectstack/spec/contracts'; +import { RestServer } from './rest-server.js'; + +const DATA_COLLECTION = '/api/v1/data/:object'; +const DATA_ITEM = '/api/v1/data/:object/:id'; + +// --------------------------------------------------------------------------- +// Fixture: a federated datasource whose rows this file can count +// --------------------------------------------------------------------------- + +interface Backing { + driver: IDataDriver; + /** Every row the driver holds, in insertion order. The "remote table". */ + rows: () => Array>; + /** Write attempts that reached the driver at all. */ + writes: () => string[]; +} + +function makeDriver(name: string): Backing { + const store = new Map>(); + const writes: string[] = []; + const driver = { + name, + version: '1.0.0', + async connect() {}, + async disconnect() {}, + async find() { return [...store.values()]; }, + async findOne() { return null; }, + async count() { return store.size; }, + async create(object: string, data: any) { + writes.push(`create:${object}`); + const id = data.id ?? String(store.size + 1); + const row = { ...data, id }; + store.set(`${object}:${id}`, row); + return row; + }, + async update(object: string, id: string, data: any) { + writes.push(`update:${object}`); + const row = { ...(store.get(`${object}:${id}`) ?? {}), ...data, id }; + store.set(`${object}:${id}`, row); + return row; + }, + async delete(object: string, id: string) { + writes.push(`delete:${object}`); + return store.delete(`${object}:${id}`); + }, + async bulkCreate(object: string, rows: any[]) { + return rows.map((r) => { + writes.push(`create:${object}`); + const id = r.id ?? String(store.size + 1); + const row = { ...r, id }; + store.set(`${object}:${id}`, row); + return row; + }); + }, + async syncSchema() {}, + async dropTable() {}, + } as unknown as IDataDriver; + return { driver, rows: () => [...store.values()], writes: () => [...writes] }; +} + +/** + * The reproduction from the card: a read-only federated external object. + * + * `schemaMode: 'external'` with `allowWrites` / `writable` both off is exactly + * the double opt-in Gate 3 requires, and the ONLY thing `writable` changes is + * whether the gate throws — so the two arms of this fixture differ by one flag. + */ +function makeEngine(opts: { writable?: boolean } = {}) { + const backing = makeDriver('warehouse'); + const engine = new ObjectQL(); + engine.registerDriver(makeDriver('default').driver, true); + engine.registerDriver(backing.driver); + engine.registerDatasourceDef({ + name: 'warehouse', + schemaMode: 'external', + external: { allowWrites: opts.writable ?? false }, + } as any); + engine.registerApp({ + id: 'wh_pkg', + name: 'Warehouse', + objects: [ + { + name: 'wh_order', + datasource: 'warehouse', + external: { remoteName: 'fact_orders', writable: opts.writable ?? false }, + fields: { order_id: { type: 'text' }, amount: { type: 'number' } }, + }, + ], + } as any); + return { engine, backing }; +} + +// --------------------------------------------------------------------------- +// The REST harness — the real CRUD routes, driven in process +// --------------------------------------------------------------------------- + +function createMockServer() { + return { + get: vi.fn(), 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), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +/** A protocol whose data verbs are the REAL engine — no rejecting stubs. */ +function setup(opts: { writable?: boolean } = {}) { + const { engine, backing } = makeEngine(opts); + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([{ name: 'wh_order' }]), + getMetaItem: vi.fn().mockResolvedValue({}), + findData: vi.fn(async (r: any) => engine.find(r.object, {})), + createData: vi.fn(async (r: any) => engine.insert(r.object, r.data)), + updateData: vi.fn(async (r: any) => engine.update(r.object, { id: r.id, ...r.data })), + deleteData: vi.fn(async (r: any) => engine.delete(r.object, { where: { id: r.id } })), + }; + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return { rest, backing }; +} + +function routeOf(rest: any, method: string, path: string) { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + return route; +} + +async function callPost(rest: any, object: string, body: Record) { + const res = makeRes(); + await routeOf(rest, 'POST', DATA_COLLECTION).handler( + { method: 'POST', params: { object }, query: {}, headers: {}, body }, + res, + ); + return res; +} + +async function callPatch(rest: any, object: string, id: string, body: Record) { + const res = makeRes(); + await routeOf(rest, 'PATCH', DATA_ITEM).handler( + { method: 'PATCH', params: { object, id }, query: {}, headers: {}, body }, + res, + ); + return res; +} + +async function callDelete(rest: any, object: string, id: string) { + const res = makeRes(); + await routeOf(rest, 'DELETE', DATA_ITEM).handler( + { method: 'DELETE', params: { object, id }, query: {}, headers: {} }, + res, + ); + return res; +} + +let errorSpy: ReturnType; +beforeEach(() => { errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); +afterEach(() => { errorSpy.mockRestore(); }); + +// --------------------------------------------------------------------------- +// §1 The reported request +// --------------------------------------------------------------------------- + +describe('[#7739] POST /api/v1/data/ on a read-only federated object', () => { + it('answers 403 with the stable code — not a bare 500 INTERNAL_ERROR', async () => { + const { rest } = setup(); + + const res = await callPost(rest, 'wh_order', { order_id: 'o1', amount: 10 }); + + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('EXTERNAL_WRITE_FORBIDDEN'); + // The two halves of the symptom, pinned negatively so a regression that + // reintroduces the generic envelope cannot pass on the positives alone. + expect(res.statusCode).not.toBe(500); + expect(res.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); + }, 60_000); + + it('keeps the refusal detail the error already computed', async () => { + // A 4xx message is addressed TO the caller and is the remedy, so the + // 4xx arm truncates rather than replaces it. This is the whole reason + // 403 was chosen over a 5xx: the 5xx band drops the prose, and this + // prose names precisely which two flags to set. + const { rest } = setup(); + + const res = await callPost(rest, 'wh_order', { order_id: 'o1' }); + + expect(res.body.error).toContain('wh_order'); + expect(res.body.error).toContain('warehouse'); + expect(res.body.error).toContain('schemaMode=external'); + expect(res.body.error).toContain('allowWrites'); + expect(res.body.error).toContain('writable'); + }, 60_000); + + it('names the object on the envelope', async () => { + const { rest } = setup(); + const res = await callPost(rest, 'wh_order', { order_id: 'o1' }); + expect(res.body.object).toBe('wh_order'); + }, 60_000); + + it('is not logged as an unhandled fault — a refusal is an expected outcome', async () => { + // 403 is an `isExpectedDataStatus`, so a deliberate refusal stops + // producing the "[REST] Unhandled error" line it emitted as a 500. + const { rest } = setup(); + + await callPost(rest, 'wh_order', { order_id: 'o1' }); + + const logged = errorSpy.mock.calls.some( + (call: unknown[]) => JSON.stringify(call.map(String)).includes('Unhandled error'), + ); + expect(logged).toBe(false); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// §2 The other two verbs the gate covers +// --------------------------------------------------------------------------- + +describe('[#7739] update and delete refusals get the same envelope', () => { + it('PATCH → 403 EXTERNAL_WRITE_FORBIDDEN', async () => { + const { rest } = setup(); + const res = await callPatch(rest, 'wh_order', 'rec1', { amount: 99 }); + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('EXTERNAL_WRITE_FORBIDDEN'); + }, 60_000); + + it('DELETE → 403 EXTERNAL_WRITE_FORBIDDEN', async () => { + const { rest } = setup(); + const res = await callDelete(rest, 'wh_order', 'rec1'); + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('EXTERNAL_WRITE_FORBIDDEN'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// §3 Nothing is applied — and the store can prove it +// --------------------------------------------------------------------------- + +describe('[#7739] the remote table is byte-identical after a refused write', () => { + it('POST leaves the store empty and never reaches the driver', async () => { + const { rest, backing } = setup(); + const before = JSON.stringify(backing.rows()); + + const res = await callPost(rest, 'wh_order', { order_id: 'o1', amount: 10 }); + + expect(res.statusCode).toBe(403); + expect(JSON.stringify(backing.rows())).toBe(before); + expect(backing.rows()).toEqual([]); + // Stronger than a row count: the gate refuses BEFORE the driver, so a + // create-then-rollback "fix" would fail here even with an empty table. + expect(backing.writes()).toEqual([]); + }, 60_000); + + it('PATCH and DELETE reach the driver no more than POST does', async () => { + const { rest, backing } = setup(); + await callPatch(rest, 'wh_order', 'rec1', { amount: 99 }); + await callDelete(rest, 'wh_order', 'rec1'); + expect(backing.writes()).toEqual([]); + }, 60_000); + + it('CONTROL: with the double opt-in the same route DOES write — the fixture is not inert', async () => { + // Without this, every "nothing was applied" assertion above would pass + // just as happily against a driver that cannot write at all, and the + // section would be measuring nothing. + const { rest, backing } = setup({ writable: true }); + + const res = await callPost(rest, 'wh_order', { order_id: 'o1', amount: 10 }); + + expect(res.statusCode).toBe(201); + expect(backing.rows()).toHaveLength(1); + expect(backing.rows()[0]).toMatchObject({ order_id: 'o1' }); + expect(backing.writes()).toContain('create:wh_order'); + }, 60_000); +}); diff --git a/packages/spec/api-surface/shared.json b/packages/spec/api-surface/shared.json index 6453bb870c..93627fad19 100644 --- a/packages/spec/api-surface/shared.json +++ b/packages/spec/api-surface/shared.json @@ -14,6 +14,7 @@ "CronExpressionInput (type)", "CronExpressionInputSchema (const)", "EXTERNAL_ERROR_CODES (const)", + "EXTERNAL_ERROR_HTTP_STATUS (const)", "EventName (type)", "EventNameSchema (const)", "Expression (type)", diff --git a/packages/spec/export-origins/shared.json b/packages/spec/export-origins/shared.json index b44f102c57..93a59de0a9 100644 --- a/packages/spec/export-origins/shared.json +++ b/packages/spec/export-origins/shared.json @@ -14,6 +14,7 @@ "CronExpressionInput": "src/shared/expression.zod.ts#CronExpressionInput (type)", "CronExpressionInputSchema": "src/shared/expression.zod.ts#CronExpressionInputSchema (const)", "EXTERNAL_ERROR_CODES": "src/shared/external-errors.ts#EXTERNAL_ERROR_CODES (const)", + "EXTERNAL_ERROR_HTTP_STATUS": "src/shared/external-errors.ts#EXTERNAL_ERROR_HTTP_STATUS (const)", "EventName": "src/shared/identifiers.zod.ts#EventName (type)", "EventNameSchema": "src/shared/identifiers.zod.ts#EventNameSchema (const)", "Expression": "src/shared/expression.zod.ts#Expression (type)", diff --git a/packages/spec/src/shared/external-errors.test.ts b/packages/spec/src/shared/external-errors.test.ts index 527a3322ed..1d7ff69279 100644 --- a/packages/spec/src/shared/external-errors.test.ts +++ b/packages/spec/src/shared/external-errors.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect } from 'vitest'; import { EXTERNAL_ERROR_CODES, + EXTERNAL_ERROR_HTTP_STATUS, renderDiffMessage, ExternalSchemaMismatchError, ExternalWriteForbiddenError, @@ -18,6 +19,63 @@ describe('External error codes (ADR-0015)', () => { }); }); +// --------------------------------------------------------------------------- +// [#7739] The status half of the contract. +// +// A code with no status is a code no HTTP exit can put on the wire: every exit +// in this repo resolves `status` → `statusCode` and, finding neither, answers +// the terminal sanitised 500 that has no `code` at all. So these assertions are +// not decoration on the table — they are what makes the ledgered codes +// REACHABLE by a client. +// --------------------------------------------------------------------------- + +describe('[#7739] EXTERNAL_ERROR_HTTP_STATUS', () => { + it('covers every code in the family — no gate can leak as a bare 500', () => { + // The `satisfies Record` makes a missing entry a + // compile error; this is the runtime twin, so a code added to the map but + // not to `EXTERNAL_ERROR_CODES` (or vice versa) is caught by `pnpm test` + // too. Sorted comparison: the map is keyed by VALUE, the codes object by + // name. + expect(Object.keys(EXTERNAL_ERROR_HTTP_STATUS).sort()).toEqual( + Object.values(EXTERNAL_ERROR_CODES).sort(), + ); + }); + + it('reports the two POLICY refusals as 403, not as a client-syntax error', () => { + // 403 rather than 400/422: the request is well-formed and would succeed + // unchanged once the opt-in flags are set — nothing for the caller to fix + // in the payload. Rather than 409: no state the caller can reconcile and + // retry. + expect(EXTERNAL_ERROR_HTTP_STATUS.EXTERNAL_WRITE_FORBIDDEN).toBe(403); + expect(EXTERNAL_ERROR_HTTP_STATUS.EXTERNAL_SCHEMA_MODE_VIOLATION).toBe(403); + }); + + it('reports a schema divergence as 503 — a deployment state, not the caller', () => { + // Same reading `ERR_DATASOURCE_UNAVAILABLE` gets: nothing about the request + // is wrong, only an operator can reconcile metadata with the remote table, + // and it may clear. A 4xx here would tell the caller to fix something they + // do not control. + expect(EXTERNAL_ERROR_HTTP_STATUS.EXTERNAL_SCHEMA_MISMATCH).toBe(503); + }); + + it('every status is inside the 400-599 band each HTTP exit reads', () => { + // Outside that band `declaredHttpStatus` (rest) and its siblings treat the + // declaration as absent, which would silently restore the 500 leak. + for (const status of Object.values(EXTERNAL_ERROR_HTTP_STATUS)) { + expect(status).toBeGreaterThanOrEqual(400); + expect(status).toBeLessThan(600); + } + }); + + it('each error instance carries its family status as `status`', () => { + // `status` (not `statusCode`) because that is the spelling BOTH doors read: + // `resolveErrorResponse` is deliberately `status`-only (#7525). + expect(new ExternalWriteForbiddenError().status).toBe(403); + expect(new ExternalSchemaModeViolationError().status).toBe(403); + expect(new ExternalSchemaMismatchError('warehouse', 'wh_order', []).status).toBe(503); + }); +}); + describe('renderDiffMessage', () => { it('renders a header with no entries', () => { const msg = renderDiffMessage('warehouse', 'wh_order', []); diff --git a/packages/spec/src/shared/external-errors.ts b/packages/spec/src/shared/external-errors.ts index 2b06fa797e..3099c521ba 100644 --- a/packages/spec/src/shared/external-errors.ts +++ b/packages/spec/src/shared/external-errors.ts @@ -27,6 +27,69 @@ export const EXTERNAL_ERROR_CODES = { export type ExternalErrorCode = (typeof EXTERNAL_ERROR_CODES)[keyof typeof EXTERNAL_ERROR_CODES]; +/** + * [#7739] The HTTP status each federation refusal is reported with — the whole + * family in ONE table, beside the codes it keys on. + * + * ## Why this exists + * + * Every gate above throws an error carrying a stable, ledgered `code` + * (`ERROR_CODE_LEDGER`, ADR-0112 D3) and, until now, no status. No HTTP exit + * can invent one for it, so a write refused by Gate 3 left `/api/v1/data` as + * `500 INTERNAL_ERROR` with no `code` at all — through `mapDataError`'s + * terminal `UNCLASSIFIED_FAULT`, indistinguishable from the server falling + * over, even though the refusal was correct and nothing was applied. The + * `code` the client needed existed the whole way down and was dropped at the + * boundary for want of a status to carry it. + * + * ## Why HERE and not in a REST error map + * + * ADR-0112: the PRODUCER names the condition. This repo's HTTP exits already + * agree on how to read one — `status` then `statusCode`, 400-599 — in + * `mapDataError`'s `declaredHttpStatus` (#7525), `resolveErrorResponse` + * (#5437/#5582), `HttpDispatcher.errorFromThrown` (#3867), + * `dispatcher-plugin.errorResponseBase`, `endpoint-executor`, + * `domains/actions` and `plugin-hono-server`. So declaring the status on the + * error is what routes the family through one place *for every door at once*; + * a branch in `rest-server.ts` would fix one door and leave the runtime + * dispatcher, the endpoint executor and the CLI answering 500 for the same + * throw. Same shape as `service-analytics`'s `dataset-refusal.ts` (#5367) and + * `storage-service.ts`'s `storageListRefusal` — "one condition, one wire + * shape, chosen by the producer that knows". + * + * ## Why these three statuses + * + * - **`writeForbidden` → 403.** A POLICY refusal, not malformed input: the + * identical body succeeds the moment `datasource.external.allowWrites` and + * `object.external.writable` are both on, so 400/422 ("fix your request") + * would be a lie, and 409 ("conflict with current state") promises a retry + * that cannot help. 403 is what this platform already answers for a + * capability a flag switched off for the object — `FEEDS_DISABLED`, + * `FILES_DISABLED`, `CLONE_DISABLED`, `RECORD_NOT_ACCESSIBLE` in + * `mapDataError`, and the standard catalog's `FORBIDDEN`. + * - **`schemaModeViolation` → 403.** The same sentence about DDL rather than + * rows: `schemaMode !== 'managed'` forbids it, and no request the caller + * can rewrite changes that. + * - **`schemaMismatch` → 503.** Deliberately NOT 4xx. Nothing about the + * request is wrong — the deployment's metadata and the remote table have + * diverged, only an operator can reconcile them, and it may clear. That is + * the reading `ERR_DATASOURCE_UNAVAILABLE` already gets ("the deployment + * cannot serve this object right now"), and both are `isExpectedDataStatus` + * lifecycle outcomes rather than crashes. The 5xx band withholds the + * message by design, so the structured `diffs` stay operator-side (where + * this gate's audience already is — it aborts boot) while the client still + * gets a `code` it can branch on instead of a bare 500. + * + * Exported so a consumer can assert the mapping rather than hardcode it, and + * `satisfies` so adding a fourth code without a status is a compile error — + * which is what stops the next gate from re-opening #7739 under a new code. + */ +export const EXTERNAL_ERROR_HTTP_STATUS = { + [EXTERNAL_ERROR_CODES.schemaMismatch]: 503, + [EXTERNAL_ERROR_CODES.writeForbidden]: 403, + [EXTERNAL_ERROR_CODES.schemaModeViolation]: 403, +} as const satisfies Record; + /** * The kinds of divergence the schema validator can report between a * federated `Object` definition and the remote table it binds to. @@ -106,6 +169,8 @@ export function renderDiffMessage( */ export class ExternalSchemaMismatchError extends Error { readonly code = EXTERNAL_ERROR_CODES.schemaMismatch; + /** [#7739] See {@link EXTERNAL_ERROR_HTTP_STATUS} — 503, not a client error. */ + readonly status = EXTERNAL_ERROR_HTTP_STATUS[EXTERNAL_ERROR_CODES.schemaMismatch]; constructor( readonly datasource: string, @@ -125,6 +190,8 @@ export class ExternalSchemaMismatchError extends Error { */ export class ExternalWriteForbiddenError extends Error { readonly code = EXTERNAL_ERROR_CODES.writeForbidden; + /** [#7739] See {@link EXTERNAL_ERROR_HTTP_STATUS} — 403, a policy refusal. */ + readonly status = EXTERNAL_ERROR_HTTP_STATUS[EXTERNAL_ERROR_CODES.writeForbidden]; constructor(message = 'Writes are forbidden on this external datasource.') { super(message); @@ -139,6 +206,8 @@ export class ExternalWriteForbiddenError extends Error { */ export class ExternalSchemaModeViolationError extends Error { readonly code = EXTERNAL_ERROR_CODES.schemaModeViolation; + /** [#7739] See {@link EXTERNAL_ERROR_HTTP_STATUS} — 403, a policy refusal. */ + readonly status = EXTERNAL_ERROR_HTTP_STATUS[EXTERNAL_ERROR_CODES.schemaModeViolation]; constructor( message = 'DDL is forbidden on a non-managed datasource (schemaMode != "managed").',