From b904123dd3a0411da44fbbb85bbaea2766e6163d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 09:53:36 +0000 Subject: [PATCH] fix(runtime): route every domain catch through errorFromThrown (#3918 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3867 taught `errorResponseBase` to read `status` (not just `statusCode`) and #3918 taught `errorFromThrown` the `VALIDATION_FAILED` shape. Both fixes were invisible to the tier underneath: the domain modules each caught their own errors and called `deps.error(e.message, e.statusCode || 500)` directly, bypassing `errorFromThrown` — 13 call sites, 9 of them in `/packages`. So on `/packages`, `/meta/_drafts`, `/ui`, `/security` and the `/mcp` transport, a deliberate 404 still rendered as a 500 (every protocol-layer domain error carries its status as `status`, the exact read #3867 fixed one tier up), and a `ValidationError` still lost its `fields[]` and its 400 — re-opening #3918 on the very routes it was filed against. Every such catch now calls `deps.errorFromThrown(e, …)`. Deliberate per-route fallbacks are preserved rather than flattened to 500: `/meta` save keeps 501, the `/meta` two-part lookup keeps 404 — but a validation failure on either now answers 400 with its fields instead of being swallowed by the fallback. `domains/keys.ts` is deliberately NOT converted: it discards the underlying error on purpose because the message could echo row contents, so its literal 'Failed to create API key' stays. Covered by `domains/error-passthrough.test.ts` — 13 cases driven through the real routes, 8 of which fail on the pre-fix source. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01LeZ7DmSKjiKmQzYh8L5CJS --- .changeset/domain-error-passthrough.md | 40 +++++ .../src/domains/error-passthrough.test.ts | 160 ++++++++++++++++++ packages/runtime/src/domains/mcp.ts | 2 +- packages/runtime/src/domains/meta.ts | 13 +- packages/runtime/src/domains/packages.ts | 29 +++- packages/runtime/src/domains/security.ts | 7 +- packages/runtime/src/domains/ui.ts | 2 +- 7 files changed, 235 insertions(+), 18 deletions(-) create mode 100644 .changeset/domain-error-passthrough.md create mode 100644 packages/runtime/src/domains/error-passthrough.test.ts diff --git a/.changeset/domain-error-passthrough.md b/.changeset/domain-error-passthrough.md new file mode 100644 index 0000000000..bdb3c189d4 --- /dev/null +++ b/.changeset/domain-error-passthrough.md @@ -0,0 +1,40 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): route every domain `catch` through `errorFromThrown` so status and `fields[]` survive (#3918 follow-up) + +#3867 taught `dispatcher-plugin`'s `errorResponseBase` to read an error's +`status` (not just `statusCode`), and #3918 taught +`HttpDispatcher.errorFromThrown` the `VALIDATION_FAILED` shape. Both fixes were +invisible to a whole tier of handlers underneath them: the domain modules each +caught their own errors and called `deps.error(e.message, e.statusCode || 500)` +directly, bypassing `errorFromThrown` entirely — 13 call sites, 9 of them in +`/packages` alone. + +The consequence on `/packages`, `/meta/_drafts`, `/ui`, `/security` and the +`/mcp` transport: + +- **A deliberate status was downgraded to 500.** Every protocol-layer domain + error in this codebase carries its HTTP status as `status`, not `statusCode` + (`OBJECT_NOT_FOUND`, `RECORD_NOT_FOUND`, `CLONE_DISABLED`, plugin-sharing's + `FORBIDDEN`, …) — the exact read #3867 fixed one tier up. So a 404 these + routes meant to return arrived as a 500, and the message was dragged through + the 5xx leak sanitiser on the way out. +- **A `ValidationError` still lost its `fields[]`** and its 400, re-opening + #3918 on precisely the routes it was filed against. + +Every one of those catches now calls `deps.errorFromThrown(e, …)`, so both +fixes finally reach the routes that need them. Deliberate per-route fallbacks +are preserved rather than flattened to 500: the `/meta` save fallback keeps +**501** (that branch is reached only when the protocol has no `saveMetaItem`, +so "unsupported" is the honest default) and the `/meta` two-part lookup keeps +**404** — but a validation failure on either now answers 400 with its fields +instead of being swallowed by the fallback. + +`domains/keys.ts` is deliberately **not** converted: it discards the underlying +error on purpose, because the message could echo row contents. Its literal +`'Failed to create API key'` is the correct answer there and stays. + +No behaviour change for errors that already carried `statusCode` — that read is +preserved, only widened. diff --git a/packages/runtime/src/domains/error-passthrough.test.ts b/packages/runtime/src/domains/error-passthrough.test.ts new file mode 100644 index 0000000000..b9429ed154 --- /dev/null +++ b/packages/runtime/src/domains/error-passthrough.test.ts @@ -0,0 +1,160 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #3918 follow-up — the THIRD tier of dispatcher error handling. + * + * #3867 taught `dispatcher-plugin`'s `errorResponseBase` to read `status` (not + * just `statusCode`) and #3918 taught `HttpDispatcher.errorFromThrown` the + * `VALIDATION_FAILED` shape. Both fixes were invisible to a whole tier of + * handlers underneath them: the domain modules each caught their own errors and + * called `deps.error(e.message, e.statusCode || 500)` directly, bypassing + * `errorFromThrown` entirely. So on `/packages`, `/meta/_drafts`, `/ui`, + * `/security` and the `/mcp` transport: + * + * - a domain error carrying `status` (the convention every protocol-layer + * error uses — `OBJECT_NOT_FOUND`, `RECORD_NOT_FOUND`, `CLONE_DISABLED`, + * plugin-sharing's `FORBIDDEN`, …) still rendered as a **500**, and + * - a record `ValidationError` still lost its `fields[]` and its 400. + * + * Every one of those catches now routes through `deps.errorFromThrown(e, …)`, + * so the two fixes above finally reach the routes that need them. The + * per-route fallback status is preserved where it was deliberate (`/meta` + * save → 501, the `/meta` two-part lookup → 404). + * + * `keys.ts` is deliberately NOT converted and is guarded below: it discards the + * underlying error on purpose, because the message could echo row contents. + */ + +import { describe, it, expect } from 'vitest'; + +import { HttpDispatcher } from './../http-dispatcher.js'; + +/** A `ValidationError` as it looks on the wire side of a throw. */ +const FIELDS = [{ field: 'name', code: 'required', message: 'name is required' }]; +function validationError() { + const err = new Error('name is required') as Error & { code: string; fields: unknown[] }; + err.name = 'ValidationError'; + err.code = 'VALIDATION_FAILED'; + err.fields = FIELDS; + return err; +} + +/** A protocol-layer domain error: status on `status`, never `statusCode`. */ +function domainNotFound() { + return Object.assign(new Error("Object 'ghost' is not registered"), { + code: 'OBJECT_NOT_FOUND', + status: 404, + }); +} + +/** + * Dispatch against a kernel whose `protocol` service throws `err` from every + * method these routes call. `objectql.registry` satisfies the `/packages` + * pre-check. + */ +async function dispatchWith(method: string, path: string, err: unknown, body: any = {}) { + const thrower = async () => { throw err; }; + const protocol = { + listCommits: thrower, + listDrafts: thrower, + getUiView: thrower, + updatePackage: thrower, + }; + const objectql = { registry: {} }; + const resolve = (name: string) => + name === 'protocol' ? protocol : name === 'objectql' ? objectql : undefined; + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + }; + const result: any = await new HttpDispatcher(kernel).dispatch(method, path, body, {}, {} as any); + return result.response; +} + +describe('#3918 follow-up — domain catches honour `status` instead of forcing 500', () => { + // One case per converted module, driven through the real route. + const routes: Array<[string, string, string]> = [ + ['/packages', 'GET', '/packages/demo/commits'], + ['/meta/_drafts', 'GET', '/meta/_drafts'], + ['/ui', 'GET', '/ui/view/account'], + ]; + + for (const [label, method, path] of routes) { + it(`${label}: a domain error's own 404 survives (was 500)`, async () => { + const res = await dispatchWith(method, path, domainNotFound()); + + expect(res.status).toBe(404); + // A 4xx message is a deliberate answer and must reach the caller. + expect(res.body.error.message).toContain("Object 'ghost' is not registered"); + expect(res.body.error.details?.code).toBe('OBJECT_NOT_FOUND'); + }); + + it(`${label}: a ValidationError keeps its 400 and its fields[]`, async () => { + const res = await dispatchWith(method, path, validationError()); + + expect(res.status).toBe(400); + expect(res.body.error.details).toEqual({ + code: 'VALIDATION_FAILED', + fields: FIELDS, + }); + }); + + it(`${label}: an ordinary error still falls back to 500`, async () => { + const res = await dispatchWith(method, path, new Error('backend unavailable')); + + expect(res.status).toBe(500); + expect(res.body.error.message).toBe('backend unavailable'); + }); + } + + it('/packages: `statusCode` still works for callers that use it', async () => { + // The property the old hand-rolled read honoured — converting must not + // drop it, only widen what else is understood. + const err = Object.assign(new Error('conflict'), { statusCode: 409 }); + const res = await dispatchWith('GET', '/packages/demo/commits', err); + + expect(res.status).toBe(409); + expect(res.body.error.message).toBe('conflict'); + }); + + it('/packages PATCH: the same holds on the write path', async () => { + // A real patch body — the route rejects an empty one with its own 400 + // before it ever reaches `updatePackage`. + const res = await dispatchWith('PATCH', '/packages/demo', domainNotFound(), { name: 'renamed' }); + + expect(res.status).toBe(404); + }); +}); + +describe('#3918 follow-up — deliberate per-route fallbacks are preserved', () => { + /** `/meta` PUT with a metadata service (no `saveMetaItem`) → the 501 branch. */ + async function putMetaViaMetadataService(err: unknown) { + const metadata = { saveItem: async () => { throw err; } }; + const resolve = (name: string) => (name === 'metadata' ? metadata : undefined); + const kernel: any = { + getService: resolve, + getServiceAsync: async (name: string) => resolve(name), + }; + const result: any = await new HttpDispatcher(kernel).dispatch( + 'PUT', '/meta/object/widget', { name: 'widget' }, {}, {} as any, + ); + return result.response; + } + + it('/meta save keeps 501 as the fallback for an ordinary failure', async () => { + // This branch is only reached when the protocol has no `saveMetaItem`, + // so "not supported" remains the honest default. + const res = await putMetaViaMetadataService(new Error('no backend')); + + expect(res.status).toBe(501); + }); + + it('/meta save still answers 400 when the failure is validation', async () => { + // …but a save rejected by validation is a fixable 400, not a capability + // gap. This is the case the 501 fallback used to swallow. + const res = await putMetaViaMetadataService(validationError()); + + expect(res.status).toBe(400); + expect(res.body.error.details.fields).toEqual(FIELDS); + }); +}); diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 1b9e82218b..4e7bd1b6f0 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -122,7 +122,7 @@ export async function handleMcpRequest(deps: DomainHandlerDeps, body: any, conte ...(grantedScopes ? { toolOptions: { grantedScopes } } : {}), }); } catch (err: any) { - return { handled: true, response: deps.error(err?.message ?? 'MCP request failed', 500) }; + return { handled: true, response: deps.errorFromThrown(err, 500) }; } // Convert the transport's buffered Web Response into the dispatcher's diff --git a/packages/runtime/src/domains/meta.ts b/packages/runtime/src/domains/meta.ts index f466b427cd..c442d5f6c0 100644 --- a/packages/runtime/src/domains/meta.ts +++ b/packages/runtime/src/domains/meta.ts @@ -174,7 +174,11 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin const data = await (metaSvc as any).saveItem(type, name, body); return { handled: true, response: deps.success(data) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message || 'Save not supported', 501) }; + // 501 stays the FALLBACK (this branch is reached only when + // the protocol has no `saveMetaItem`, so "unsupported" is + // the honest default) — but a save that fails validation is + // a 400 the caller can fix, not a capability gap. + return { handled: true, response: deps.errorFromThrown(e, 501) }; } } return { handled: true, response: deps.error('Save not supported', 501) }; @@ -261,8 +265,9 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin return { handled: true, response: deps.error('Not found', 404) }; } catch (e: any) { // Fallback: treat first part as object name if only 1 part (handled below) - // But here we are deep in 2 parts. Must be an error. - return { handled: true, response: deps.error(e.message, 404) }; + // But here we are deep in 2 parts. Must be an error — 404 remains the + // default, but an error carrying its own status keeps it. + return { handled: true, response: deps.errorFromThrown(e, 404) }; } } @@ -283,7 +288,7 @@ export async function handleMetadataRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(data) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } return { handled: true, response: deps.error('Draft listing not supported', 501) }; diff --git a/packages/runtime/src/domains/packages.ts b/packages/runtime/src/domains/packages.ts index efd4933a5a..024c54d79d 100644 --- a/packages/runtime/src/domains/packages.ts +++ b/packages/runtime/src/domains/packages.ts @@ -37,6 +37,17 @@ export function createPackagesDomain(deps: DomainHandlerDeps): DomainRoute { * - POST /packages/:id/revert → revert a package to last published state * * Uses ObjectQL SchemaRegistry directly (via the 'objectql' service). + * + * **Error handling.** Every `catch` here goes through `deps.errorFromThrown(e, + * 500)` — never `deps.error(e.message, …)`. Each of these routes calls into the + * protocol service, whose errors carry their own HTTP status as `status` (the + * codebase convention: `OBJECT_NOT_FOUND`, `RECORD_NOT_FOUND`, `CLONE_DISABLED`, + * …), and can be a record `ValidationError` carrying `fields[]`. Hand-rolling + * `e.statusCode || 500` saw neither — #3867 fixed exactly that read in + * `errorResponseBase` and #3918 taught `errorFromThrown` the validation shape, + * but these call sites bypassed both, so a deliberate 404 still rendered as a + * 500 and `fields[]` was still dropped. Route new handlers through the shared + * helper rather than re-deriving the status here. */ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: string, method: string, body: any, query: any, _context: HttpProtocolContext): Promise { const m = method.toUpperCase(); @@ -275,7 +286,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } return { handled: true, response: deps.error('Draft discarding not supported', 501) }; @@ -296,7 +307,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success({ commits }) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } return { handled: true, response: deps.error('Commit history not supported', 501) }; @@ -318,7 +329,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } return { handled: true, response: deps.error('Commit revert not supported', 501) }; @@ -341,7 +352,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } return { handled: true, response: deps.error('Commit rollback not supported', 501) }; @@ -387,7 +398,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } @@ -416,7 +427,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } @@ -457,7 +468,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin const updated = await (protocol as any).updatePackage({ packageId: id, patch }); return { handled: true, response: deps.success((updated as any)?.package ?? updated) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } // Fallback: no protocol service — in-memory registry only. @@ -490,7 +501,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin ...(keepData ? { keepData: true } : {}), }); } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } @@ -501,7 +512,7 @@ export async function handlePackagesRequest(deps: DomainHandlerDeps, path: strin return { handled: true, response: deps.success({ success: true, registryRemoved, persisted }) }; } } catch (e: any) { - return { handled: true, response: deps.error(e.message, e.statusCode || 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } return { handled: false }; diff --git a/packages/runtime/src/domains/security.ts b/packages/runtime/src/domains/security.ts index 088afdcb05..81e282d652 100644 --- a/packages/runtime/src/domains/security.ts +++ b/packages/runtime/src/domains/security.ts @@ -94,8 +94,9 @@ export async function handleSecurityRequest( } catch (err: any) { // The service throws typed errors carrying their HTTP status: // PermissionDeniedError → 403, SuggestionNotFoundError → 404, - // SuggestionStateError → 409. - const status = typeof err?.statusCode === 'number' ? err.statusCode : 500; - return { handled: true, response: deps.error(err?.message ?? 'Security operation failed', status) }; + // SuggestionStateError → 409. Read via `errorFromThrown` so `status` + // counts too, not just `statusCode` — the rest of the codebase's domain + // errors use the former (#3867). + return { handled: true, response: deps.errorFromThrown(err, 500) }; } } diff --git a/packages/runtime/src/domains/ui.ts b/packages/runtime/src/domains/ui.ts index 7e3f8a4735..dc0bbbcf74 100644 --- a/packages/runtime/src/domains/ui.ts +++ b/packages/runtime/src/domains/ui.ts @@ -41,7 +41,7 @@ export async function handleUiRequest( const result = await protocol.getUiView({ object: objectName, type }); return { handled: true, response: deps.success(result) }; } catch (e: any) { - return { handled: true, response: deps.error(e.message, 500) }; + return { handled: true, response: deps.errorFromThrown(e, 500) }; } } else { return { handled: true, response: deps.error('Protocol service not available', 503) };