From d2ccb2f79a56ec1bf85134f404e4db2fdfed776f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 01:42:44 +0000 Subject: [PATCH 1/2] fix(api): envelope the plugin-mounted Hono error paths (#9364) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six hand-built refusal bodies on plugin-mounted Hono routes departed from BaseResponseSchema, invisible to every check in the repo until #9267 added the route-envelope gate's third surface. This converts the error-path half: - plugin-hono-server/src/adapter.ts — the 404, 405, and two 500 refusals now answer { success: false, error: { code, message } }; the 405's method/path/ allowed move into error.details and its METHOD_NOT_ALLOWED code moves from beside `error` to inside it (wire value unchanged). - adapters/hono/src/index.ts — errorJson wrote the HTTP status into error.code; it now derives the ADR-0112 member from the status through resolveThrownHttpError, the rule the REST and dispatcher doors already read. - cli/src/commands/serve.ts — the unbound-hostname 404 drops its bare-string error and two stray top-level keys. qa/http-conformance's NodeHttpServer mirrors the adapter's unmatched-request bodies byte-for-byte by design and is locked to them cross-adapter by fallback-seam.conformance.test.ts, so it moves in the same commit. check-route-envelope.mjs counters banked: adapter.ts and serve.ts to conformant, adapters/hono keeps unenveloped 2 (retargeted to #9436). Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_012WKSnqAaoqtW3QX7SSf1Vk --- .changeset/plugin-route-refusals-enveloped.md | 72 +++++++++++++++++ .../src/hono-wildcard-fallthrough.test.ts | 8 +- packages/adapters/hono/src/index.ts | 33 +++++++- packages/cli/src/commands/serve.ts | 18 ++++- .../plugins/plugin-hono-server/src/adapter.ts | 79 +++++++++++++++---- .../src/fallback-seam.test.ts | 17 ++-- .../src/handler-throw-logging.test.ts | 30 +++++-- .../src/notfound-405.test.ts | 12 ++- .../qa/http-conformance/src/adapter.test.ts | 12 ++- packages/qa/http-conformance/src/adapter.ts | 27 ++++--- .../src/conformance.integration.test.ts | 12 ++- .../src/fallback-seam.conformance.test.ts | 15 +++- scripts/check-route-envelope.mjs | 29 +++---- 13 files changed, 290 insertions(+), 74 deletions(-) create mode 100644 .changeset/plugin-route-refusals-enveloped.md diff --git a/.changeset/plugin-route-refusals-enveloped.md b/.changeset/plugin-route-refusals-enveloped.md new file mode 100644 index 0000000000..379a833d43 --- /dev/null +++ b/.changeset/plugin-route-refusals-enveloped.md @@ -0,0 +1,72 @@ +--- +"@objectstack/plugin-hono-server": minor +"@objectstack/hono": minor +"@objectstack/cli": minor +--- + +fix(api): the plugin-mounted Hono error paths answer the declared envelope — six refusal bodies stop speaking the pre-#3675 dialect (#9364) + +Six hand-built refusal bodies on plugin-mounted Hono routes departed from +`BaseResponseSchema`. They were invisible to every check in the repo until +#9267 added the gate's third surface, which discovers these routes by parsing +rather than by filename. This converts the **error-path** half of what that +first run measured; the bare pre-auth discovery payloads it also found are a +separate wire ruling (#9389) and are untouched here. + +**If you branch on these bodies, this is the change.** Every one of them was +readable only by reaching for a key the contract does not declare, so no +consumer that followed `ApiErrorSchema` was reading them successfully in the +first place — `body.error.message` read `undefined` on all six. + +`@objectstack/plugin-hono-server` — the adapter's own refusals, the answer any +host using it as its transport gets for an unmatched request or a handler that +produced nothing: + +| status | was | now | +|:--|:--|:--| +| 404 unmatched path | `{ error: 'Not found' }` | `{ success: false, error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' } }` | +| 405 method mismatch | `{ error, code, message, method, path, allowed }` | `{ success: false, error: { code: 'METHOD_NOT_ALLOWED', message, details: { method, path, allowed } } }` | +| 500 handler wrote nothing | `{ error: 'No response from handler' }` | `{ success: false, error: { code: 'INTERNAL_ERROR', message: 'No response from handler' } }` | +| 500 fallback threw | `{ error: 'Fallback handler failed' }` | `{ success: false, error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' } }` | + +The 405 is the sharpest of the four: it already carried a real semantic code, +but placed it BESIDE `error` rather than inside it, so `body.error.code` read +`undefined` while `body.code` worked — the #7035 dialect. Its `code` **value** +is unchanged (`METHOD_NOT_ALLOWED`, a `StandardErrorCode` member); only its +position moved, along with the three context keys, which are now +`error.details` — the slot `ApiErrorSchema` declares for exactly that. The +`Allow` header is unchanged and remains the primary channel for it. + +`@objectstack/hono` — the shared `errorJson` helper wrote the HTTP **status** +into `error.code`, so every refusal from this mount shipped `error.code: 404` +or `500` where `ApiErrorSchema.code` declares a closed STRING vocabulary +(ADR-0112 D3/D4). It now derives the standard member for the status through +`resolveThrownHttpError` (`@objectstack/types`) — the one rule the REST and +dispatcher doors already read for this question, so this third door does not +become a fourth dialect. A 404 from this mount now carries +`error.code: 'RESOURCE_NOT_FOUND'`; the numeric status stays where it is +authoritative, on the response line. + +`@objectstack/cli` — the unbound-hostname 404 from `os serve`'s +`OS_ROOT_DOMAIN` guard answered +`{ error: 'environment_not_found', message, hostname }`: a bare-string error +with two stray top-level keys, and a lowercase code where error codes are +`SCREAMING_SNAKE`. It is now +`{ success: false, error: { code: 'ENVIRONMENT_NOT_FOUND', message, details: { hostname } } }`. +The `Accept: text/html` branch still serves the styled 404 page, unchanged. + +**The cross-adapter reference implementation moved with it.** +`@objectstack/http-conformance`'s zero-dependency `NodeHttpServer` mirrors the +adapter's unmatched-request bodies byte-for-byte on purpose — the whole point +of that package is proving the transport port is free of framework-isms, and +`fallback-seam.conformance.test.ts` runs the same cases against both. Leaving +it behind would have made "both adapters agree" false in the suite that exists +to assert it. + +Every converted body is judged by `scripts/check-route-envelope.mjs`, whose +per-file counters for these three modules go to zero and are banked as +conformant. The literals are deliberately written INLINE at each `c.json(...)` +call rather than hoisted into shared constants: the gate reads the object +literal, and an identifier reads to it as a relayed body it must not police — +hoisting would have zeroed the counters by hiding the bodies from the scanner +instead of by conforming them. diff --git a/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts b/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts index fbeecab0c2..5dc23745c8 100644 --- a/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts +++ b/packages/adapters/hono/src/hono-wildcard-fallthrough.test.ts @@ -140,6 +140,12 @@ describe('the enveloped 404 survives when nothing anywhere claims the path', () expect(res.status).toBe(404); // Not Hono's bare "404 Not Found" text — the platform envelope is preserved. - expect(await res.json()).toEqual({ success: false, error: { message: 'Not Found', code: 404 } }); + // `error.code` carries the ADR-0112 SEMANTIC member for the status, not the + // status itself: this body used to read `code: 404`, a number in the slot + // `ApiErrorSchema` declares as a closed string vocabulary (#9364). + expect(await res.json()).toEqual({ + success: false, + error: { code: 'RESOURCE_NOT_FOUND', message: 'Not Found' }, + }); }); }); diff --git a/packages/adapters/hono/src/index.ts b/packages/adapters/hono/src/index.ts index a59cfcd71b..d76836b344 100644 --- a/packages/adapters/hono/src/index.ts +++ b/packages/adapters/hono/src/index.ts @@ -11,6 +11,7 @@ import { readEnvWithDeprecation, looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE, + resolveThrownHttpError, } from '@objectstack/types'; /** @@ -202,8 +203,36 @@ export function createHonoApp(options: ObjectStackHonoOptions): Hono { } } - const errorJson = (c: any, message: string, code: number = 500) => { - return c.json({ success: false, error: { message, code } }, code); + /** + * This mount's shared refusal body, in the envelope `BaseResponseSchema` + * declares. + * + * `error.code` is the SEMANTIC slot: `ApiErrorSchema.code` is a closed + * STRING vocabulary (`StandardErrorCode` ∪ the ledger — ADR-0112 D3/D4). + * This helper used to write the HTTP **status** into it — `{ message, code }` + * with `code` the same number handed to `c.json` as the status — so every + * refusal from this adapter shipped `error.code: 404` / `500` where callers + * were promised a code they could branch on, and the body failed its own + * contract while looking correctly nested. That is #3842's drift, one door + * over. + * + * The status→member mapping is NOT re-spelled here. `resolveThrownHttpError` + * (`@objectstack/types`) is the one rule both the REST and dispatcher doors + * read for this question (ADR-0112, #9106), and it derives the standard + * member from the status whenever the producer declared no registered code + * of its own — so routing through it keeps this third door from becoming a + * fourth dialect. The numeric status stays where it is authoritative: the + * response line. + * + * The parameter is named `status`, not `code`, and that is load-bearing + * rather than cosmetic: `scripts/check-route-envelope.mjs` flags an + * `error.code` shorthand whose identifier is the SAME one passed as the + * status argument — precisely the shape this file shipped — so the old name + * would keep the counter alive even with the value fixed. + */ + const errorJson = (c: any, message: string, status: number = 500) => { + const { code } = resolveThrownHttpError({ status }, status); + return c.json({ success: false, error: { code, message } }, status); }; const toResponse = (c: any, result: HttpDispatcherResult) => { diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index 608e93666b..23cacd74ef 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -2008,11 +2008,23 @@ export default class Serve extends Command { `; return c.html(html, 404); } + // The declared `BaseResponseSchema` refusal envelope. This + // used to answer `{ error: 'environment_not_found', message, + // hostname }` — the pre-#3675 dialect, where `error` is a bare + // string so `body.error.message` reads `undefined`, with two + // stray top-level keys beside it. `hostname` is context and + // moved into `error.details`, which `ApiErrorSchema` declares + // for exactly that; the code is now the ADR-0112 + // SCREAMING_SNAKE spelling of the same condition, in the + // semantic slot consumers branch on. return c.json( { - error: 'environment_not_found', - message: `No environment is bound to hostname '${host}'.`, - hostname: host, + success: false, + error: { + code: 'ENVIRONMENT_NOT_FOUND', + message: `No environment is bound to hostname '${host}'.`, + details: { hostname: host }, + }, }, 404, ); diff --git a/packages/plugins/plugin-hono-server/src/adapter.ts b/packages/plugins/plugin-hono-server/src/adapter.ts index 57edba4859..c527410f00 100644 --- a/packages/plugins/plugin-hono-server/src/adapter.ts +++ b/packages/plugins/plugin-hono-server/src/adapter.ts @@ -258,7 +258,13 @@ export class HonoHttpServer implements IHttpServer { private wrap(handler: RouteHandler) { return async (c: any) => { const { response } = await this.runHandler(c, handler); - return response ?? c.json({ error: 'No response from handler' }, 500); + return response ?? c.json( + { + success: false, + error: { code: 'INTERNAL_ERROR', message: 'No response from handler' }, + }, + 500, + ); }; } @@ -707,7 +713,13 @@ export class HonoHttpServer implements IHttpServer { // a broken consumer, and reporting its failure as this // adapter's ordinary 404 would hide it behind the most // unremarkable status on the wire. - return c.json({ error: 'Fallback handler failed' }, 500); + return c.json( + { + success: false, + error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' }, + }, + 500, + ); } // Wrote nothing — the documented way to say "not mine". Fall // through to the standard answer, unchanged. @@ -723,28 +735,63 @@ export class HonoHttpServer implements IHttpServer { * * Hono routes a method mismatch to the SAME `notFound` sink as a genuinely * missing path, so a `POST` to a `PUT`-only route (e.g. the metadata save - * endpoint, see #2684) would otherwise return an opaque - * `{ error: 'Not found' }` 404 with no hint that the path exists under - * another verb. We re-match the request path against the registered route - * patterns: if it lines up with routes under other methods, answer `405 - * Method Not Allowed` with an accurate `Allow` header so callers can - * self-correct. A path that matches nothing stays a 404. This is - * framework-wide — every registered endpoint benefits, not just metadata. + * endpoint, see #2684) would otherwise return an opaque 404 with no hint + * that the path exists under another verb. We re-match the request path + * against the registered route patterns: if it lines up with routes under + * other methods, answer `405 Method Not Allowed` with an accurate `Allow` + * header so callers can self-correct. A path that matches nothing stays a + * 404. This is framework-wide — every registered endpoint benefits, not + * just metadata. + * + * ## Envelope + * + * Both answers are the declared `BaseResponseSchema` refusal envelope — + * `{ success: false, error: { code, message } }` — with the ADR-0112 + * semantic code in `error.code` and the HTTP status carried only by the + * response line. Until this was converted the two bodies spoke the + * pre-#3675 dialect (`error` a bare STRING, so `body.error.message` read + * `undefined`) and the 405 additionally put `code`/`method`/`path`/ + * `allowed` BESIDE `error` (#7035's twin, so `body.error.code` read + * `undefined` too). The 405's three context keys moved into + * `error.details`, which `ApiErrorSchema` declares for exactly this. + * + * The wire `code` VALUES are unchanged — `METHOD_NOT_ALLOWED` was already + * the spelling this route shipped, and it is a `StandardErrorCode` member + * — so a caller that already branched on `body.code` reads the same string + * one level in. `ENDPOINT_NOT_FOUND` is the standard catalog's 404 member + * for "API endpoint not found"; the 404 previously carried no code at all. + * + * ⚠️ The literals here are deliberately INLINE rather than hoisted into a + * shared constant: `scripts/check-route-envelope.mjs` judges the object + * LITERAL passed to `c.json(...)`, and an identifier reads to it as a + * relayed body it must not police. Hoisting would zero this file's + * counters by hiding the bodies from the scanner rather than by + * conforming them — and would leave every later edit to them unaudited. + * `packages/qa/http-conformance`'s `NodeHttpServer` mirrors these bodies + * byte-for-byte and is locked to them cross-adapter by + * `fallback-seam.conformance.test.ts`; change one and you must change both. */ private unmatchedResponse(c: any) { const allowed = this.allowedMethodsForPath(c.req.path); if (allowed.length > 0 && !allowed.includes(c.req.method)) { c.header('Allow', allowed.join(', ')); return c.json({ - error: 'Method Not Allowed', - code: 'METHOD_NOT_ALLOWED', - message: `${c.req.method} is not supported for ${c.req.path}. Allowed: ${allowed.join(', ')}.`, - method: c.req.method, - path: c.req.path, - allowed, + success: false, + error: { + code: 'METHOD_NOT_ALLOWED', + message: `${c.req.method} is not supported for ${c.req.path}. Allowed: ${allowed.join(', ')}.`, + details: { + method: c.req.method, + path: c.req.path, + allowed, + }, + }, }, 405); } - return c.json({ error: 'Not found' }, 404); + return c.json({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }, 404); } /** diff --git a/packages/plugins/plugin-hono-server/src/fallback-seam.test.ts b/packages/plugins/plugin-hono-server/src/fallback-seam.test.ts index 097aa5cbb2..aab84cce8f 100644 --- a/packages/plugins/plugin-hono-server/src/fallback-seam.test.ts +++ b/packages/plugins/plugin-hono-server/src/fallback-seam.test.ts @@ -175,7 +175,10 @@ describe('guarantee 4 — a fallback that writes nothing leaves the standard ans expect(res.status).toBe(404); expect(await res.text()).toBe(await baseline.text()); expect(JSON.parse(await (await call(serverWithRoutes(), '/api/v1/nope')).text())) - .toEqual({ error: 'Not found' }); + .toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('keeps the 405 + `Allow` answer for a method mismatch (#5040 §7-1)', async () => { @@ -190,8 +193,9 @@ describe('guarantee 4 — a fallback that writes nothing leaves the standard ans expect(res.status).toBe(405); expect(res.headers.get('Allow')).toBe('PUT'); const body = await res.json(); - expect(body.code).toBe('METHOD_NOT_ALLOWED'); - expect(body.allowed).toEqual(['PUT']); + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(['PUT']); expect(seen).toEqual(['DELETE /api/v1/only-put']); }); @@ -214,7 +218,10 @@ describe('failure modes', () => { server.setFallbackHandler(() => { throw new Error('boom'); }); const res = await call(server, '/api/v1/apps/showcase/tasks'); expect(res.status).toBe(500); - expect(await res.json()).toEqual({ error: 'Fallback handler failed' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' }, + }); }); it('honours a status set without a body (the `res.end()` shape)', async () => { @@ -229,7 +236,7 @@ describe('failure modes', () => { server.get('/api/v1/thing', (_req, res) => { res.json({ ok: true }); }); const res = await server.getRawApp().fetch(new Request('http://localhost/api/v1/nope')); expect(res.status).toBe(404); - // Hono's built-in answer — NOT this adapter's `{ error: 'Not found' }`. + // Hono's built-in answer — NOT this adapter's enveloped 404 body. expect(await res.text()).toBe('404 Not Found'); }); }); diff --git a/packages/plugins/plugin-hono-server/src/handler-throw-logging.test.ts b/packages/plugins/plugin-hono-server/src/handler-throw-logging.test.ts index 5bf7a6c2c6..8b71a343d3 100644 --- a/packages/plugins/plugin-hono-server/src/handler-throw-logging.test.ts +++ b/packages/plugins/plugin-hono-server/src/handler-throw-logging.test.ts @@ -78,8 +78,16 @@ function serverWithLogger() { const call = (server: HonoHttpServer, path: string, init?: RequestInit) => server.getRawApp().fetch(new Request(`http://localhost${path}`, init)); -/** The exact bytes `wrap()` answers when a handler produced no response. */ -const FALLBACK_BODY = '{"error":"No response from handler"}'; +/** + * The exact bytes `wrap()` answers when a handler produced no response. + * + * The declared `BaseResponseSchema` refusal envelope since #9364 — it was the + * bare `{"error":"No response from handler"}` (the pre-#3675 dialect) while + * that conversion was still undecided. The MESSAGE is unchanged; what moved is + * the shape around it and the ADR-0112 code beside it. + */ +const FALLBACK_BODY = + '{"success":false,"error":{"code":"INTERNAL_ERROR","message":"No response from handler"}}'; describe('an escaped handler throw is reported', () => { it('logs the original Error — message AND stack — for an async rejection', async () => { @@ -201,17 +209,23 @@ describe('the fallback (notFound) seam reports too', () => { const res = await call(server, '/nothing/here'); expect(res.status).toBe(500); - expect(await res.text()).toBe('{"error":"Fallback handler failed"}'); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' }, + }); expect((errors()[0]!.error as Error).message).toBe('fallback exploded'); expect(errors()[0]!.meta).toEqual({ method: 'GET', path: '/nothing/here' }); }); }); -describe('the response shape is unchanged (explicitly OUT of scope for #5848)', () => { - it('still answers the byte-identical bare 500 body', async () => { - // Folding this into a declared envelope would change a live response - // shape and is an undecided contract question. Pinned so it cannot - // arrive as a rider on the logging fix. +describe('the response shape is the declared envelope (decided by #9364)', () => { + it('answers the byte-identical enveloped 500 body', async () => { + // #5848 deliberately left this body alone — folding it into a declared + // envelope was a live wire change and an undecided contract question, + // so it was pinned here to stop the conversion arriving as a rider on + // a logging fix. #9364 is where that question was answered; the pin + // survives with its new bytes, and still holds the shape against an + // unrelated change drifting it again. const { server } = serverWithLogger(); server.get('/api/v1/shape', async () => { throw new Error('boom'); }); diff --git a/packages/plugins/plugin-hono-server/src/notfound-405.test.ts b/packages/plugins/plugin-hono-server/src/notfound-405.test.ts index 86ab97d3eb..280be0018b 100644 --- a/packages/plugins/plugin-hono-server/src/notfound-405.test.ts +++ b/packages/plugins/plugin-hono-server/src/notfound-405.test.ts @@ -72,8 +72,11 @@ describe('notFound → 405 Method Not Allowed (#2684)', () => { const allow = (res.headers.get('Allow') || '').split(',').map((s) => s.trim()); expect(allow).toEqual(expect.arrayContaining(['GET', 'PUT', 'DELETE'])); const body = await res.json(); - expect(body.code).toBe('METHOD_NOT_ALLOWED'); - expect(body.allowed).toEqual(expect.arrayContaining(['PUT'])); + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(expect.arrayContaining(['PUT'])); + expect(body.error.details.method).toBe('POST'); + expect(body.error.details.path).toBe('/api/v1/meta/view/my_view'); }); it('still routes the correct method to its handler (no false 405)', async () => { @@ -85,6 +88,9 @@ describe('notFound → 405 Method Not Allowed (#2684)', () => { it('keeps a genuine 404 for a path that matches no registered route', async () => { const res = await fetch('POST', '/api/v1/does/not/exist'); expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); }); diff --git a/packages/qa/http-conformance/src/adapter.test.ts b/packages/qa/http-conformance/src/adapter.test.ts index 79a10bfa7e..e289efdd87 100644 --- a/packages/qa/http-conformance/src/adapter.test.ts +++ b/packages/qa/http-conformance/src/adapter.test.ts @@ -109,7 +109,7 @@ describe('NodeHttpServer (live socket)', () => { it('500s when a handler resolves without responding', async () => { const res = await fetch(`${base}/silent`); expect(res.status).toBe(500); - expect((await res.json()).error).toBe('No response from handler'); + expect((await res.json()).error.message).toBe('No response from handler'); }); // Locks the formal streaming contract on `IHttpResponse.write`/`end` @@ -125,7 +125,10 @@ describe('NodeHttpServer (live socket)', () => { it('404s unknown paths with the shared not-found body', async () => { const res = await fetch(`${base}/nope`); expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('405s a method mismatch with an accurate Allow header', async () => { @@ -133,8 +136,9 @@ describe('NodeHttpServer (live socket)', () => { expect(res.status).toBe(405); expect(res.headers.get('allow')).toBe('POST'); const body = await res.json(); - expect(body.code).toBe('METHOD_NOT_ALLOWED'); - expect(body.allowed).toEqual(['POST']); + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(['POST']); }); it('answers HEAD from GET routes without a body', async () => { diff --git a/packages/qa/http-conformance/src/adapter.ts b/packages/qa/http-conformance/src/adapter.ts index 8693812a83..9f162ddc49 100644 --- a/packages/qa/http-conformance/src/adapter.ts +++ b/packages/qa/http-conformance/src/adapter.ts @@ -224,18 +224,21 @@ export class NodeHttpServer implements IHttpServer { nodeRes.setHeader('Allow', allowed.join(', ')); nodeRes.setHeader('Content-Type', 'application/json; charset=utf-8'); nodeRes.end(JSON.stringify({ - error: 'Method Not Allowed', - code: 'METHOD_NOT_ALLOWED', - message: `${method} is not supported for ${path}. Allowed: ${allowed.join(', ')}.`, - method, - path, - allowed, + success: false, + error: { + code: 'METHOD_NOT_ALLOWED', + message: `${method} is not supported for ${path}. Allowed: ${allowed.join(', ')}.`, + details: { method, path, allowed }, + }, })); return; } nodeRes.statusCode = 404; nodeRes.setHeader('Content-Type', 'application/json; charset=utf-8'); - nodeRes.end(JSON.stringify({ error: 'Not found' })); + nodeRes.end(JSON.stringify({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + })); } private match(method: string, path: string): { route: CompiledRoute; params: Record } | undefined { @@ -381,7 +384,10 @@ export class NodeHttpServer implements IHttpServer { nodeRes.statusCode = 500; nodeRes.setHeader('Content-Type', 'application/json; charset=utf-8'); } - nodeRes.end(JSON.stringify({ error: 'Fallback handler failed' })); + nodeRes.end(JSON.stringify({ + success: false, + error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' }, + })); } return; } @@ -406,7 +412,10 @@ export class NodeHttpServer implements IHttpServer { if (!nodeRes.writableEnded && !streaming) { nodeRes.statusCode = 500; nodeRes.setHeader('Content-Type', 'application/json; charset=utf-8'); - nodeRes.end(JSON.stringify({ error: 'No response from handler' })); + nodeRes.end(JSON.stringify({ + success: false, + error: { code: 'INTERNAL_ERROR', message: 'No response from handler' }, + })); } } catch (err: any) { if (!nodeRes.writableEnded) { diff --git a/packages/qa/http-conformance/src/conformance.integration.test.ts b/packages/qa/http-conformance/src/conformance.integration.test.ts index 773ac219ce..6de8df2acd 100644 --- a/packages/qa/http-conformance/src/conformance.integration.test.ts +++ b/packages/qa/http-conformance/src/conformance.integration.test.ts @@ -191,7 +191,10 @@ describe.each(ADAPTERS)('IHttpServer conformance on $label adapter', ({ makePlug it('404s unknown paths with the shared not-found body', async () => { const res = await fetch(`${base}/api/v1/this-route-does-not-exist`); expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('405s a method mismatch with an Allow header', async () => { @@ -201,7 +204,7 @@ describe.each(ADAPTERS)('IHttpServer conformance on $label adapter', ({ makePlug const res = await fetch(`${base}/api/v1/analytics/query`, { method: 'PUT' }); expect(res.status).toBe(405); expect(res.headers.get('allow')).toContain('POST'); - expect((await res.json()).code).toBe('METHOD_NOT_ALLOWED'); + expect((await res.json()).error.code).toBe('METHOD_NOT_ALLOWED'); }); }); @@ -231,7 +234,10 @@ describe('analytics capability-conditional mounting (no service installed)', () it.each(['POST', 'PUT', 'GET'])('%s /api/v1/analytics/query answers the shared 404', async (method) => { const res = await fetch(`${stack.base}/api/v1/analytics/query`, { method }); expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('GET /api/v1/analytics/meta answers the shared 404 too', async () => { diff --git a/packages/qa/http-conformance/src/fallback-seam.conformance.test.ts b/packages/qa/http-conformance/src/fallback-seam.conformance.test.ts index 8cb3d0e506..9ed502708a 100644 --- a/packages/qa/http-conformance/src/fallback-seam.conformance.test.ts +++ b/packages/qa/http-conformance/src/fallback-seam.conformance.test.ts @@ -241,7 +241,10 @@ describe.each(ADAPTERS)('IHttpServer.setFallbackHandler conformance on $label ad expect(baseline.status).toBe(404); expect(res.status).toBe(404); expect(await res.text()).toBe(baselineText); - expect(JSON.parse(baselineText)).toEqual({ error: 'Not found' }); + expect(JSON.parse(baselineText)).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('G4-b: a declining fallback keeps 405 + `Allow` for a method mismatch (#5040 §7-1)', async () => { @@ -258,8 +261,9 @@ describe.each(ADAPTERS)('IHttpServer.setFallbackHandler conformance on $label ad expect(res.status).toBe(405); expect(res.headers.get('allow')).toBe('PUT'); const body = await res.json(); - expect(body.code).toBe('METHOD_NOT_ALLOWED'); - expect(body.allowed).toEqual(['PUT']); + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(['PUT']); expect(seen).toEqual(['DELETE /api/v1/only-put']); }); @@ -277,6 +281,9 @@ describe.each(ADAPTERS)('IHttpServer.setFallbackHandler conformance on $label ad const res = await fetch(`${base}/api/v1/apps/showcase/tasks`); expect(res.status).toBe(500); - expect(await res.json()).toEqual({ error: 'Fallback handler failed' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'INTERNAL_ERROR', message: 'Fallback handler failed' }, + }); }); }); diff --git a/scripts/check-route-envelope.mjs b/scripts/check-route-envelope.mjs index e7facf71f6..3dc715c78a 100644 --- a/scripts/check-route-envelope.mjs +++ b/scripts/check-route-envelope.mjs @@ -542,25 +542,22 @@ const PLUGIN_ROUTE_MODULES = { ratchet: '#9364 (envelope the bare plugin-route payloads)', note: 'nine `{ authenticated, userId, … }` bodies with no `success` flag — the same bare-payload class as runtime-config, read directly by the Console', }, - 'packages/plugins/plugin-hono-server/src/adapter.ts': { - unenveloped: 4, - stringError: 4, - siblingCode: 1, - ratchet: '#9364 (convert the adapter refusals onto the declared envelope)', - note: 'the adapter\'s own refusals — `{ error: \'Not found\' }` 404, `{ error: \'No response from handler\' }` 500, `{ error: \'Fallback handler failed\' }` 500, and the 405 that adds `code`/`method`/`path`/`allowed` beside `error`. The pre-#3675 dialect and its #7035 sibling, alive at a door no scan reached', - }, + // Converted by #9364: the four refusals now answer the declared envelope, + // `{ success: false, error: { code, message } }`, with the 405's + // `method`/`path`/`allowed` moved into `error.details`. Conformant, so it + // joins the zero-entries above — and `@objectstack/http-conformance`'s + // `NodeHttpServer` mirrors these bodies byte-for-byte, locked cross-adapter + // by `fallback-seam.conformance.test.ts`. + 'packages/plugins/plugin-hono-server/src/adapter.ts': {}, 'packages/adapters/hono/src/index.ts': { unenveloped: 2, - errorCodeNotString: 1, - ratchet: '#9364 (envelope the hono adapter bodies)', - note: 'two `{ data }` discovery bodies with no `success`, plus a shared `errorJson` writing the HTTP status into `error.code` — a number where ApiErrorSchema declares a string enum', - }, - 'packages/cli/src/commands/serve.ts': { - unenveloped: 1, - stringError: 1, - ratchet: '#9364 (envelope the serve host-resolution refusal)', - note: 'the unbound-hostname 404 — `{ error: \'environment_not_found\', message, hostname }`, a bare-string error with two stray top-level keys', + ratchet: '#9436 (envelope the hono adapter discovery bodies; Blocked-by #9389)', + note: 'two `{ data }` discovery bodies with no `success`. #9364 removed this file\'s `errorCodeNotString 1` — the shared `errorJson` wrote the HTTP status into `error.code` and now derives the ADR-0112 member from it through `resolveThrownHttpError`. What is left is the same PRE-AUTH bare-payload fork #9389 rules on, but on a different consumer population (SDKs and codegen read this mount\'s discovery, not the Console SPA), so #9389\'s closed three-file list does not reach it', }, + // Converted by #9364: `{ error: \'environment_not_found\', message, hostname }` + // became the declared envelope with `ENVIRONMENT_NOT_FOUND` in the semantic + // slot and `hostname` under `error.details`. + 'packages/cli/src/commands/serve.ts': {}, 'packages/plugins/plugin-auth/src/auth-plugin.ts': { unenveloped: 3, ratchet: '#9364 (envelope the bare plugin-route payloads)', From b378fbed70c5923b95de964f28a6a0e532a13fbb Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 02:33:55 +0000 Subject: [PATCH 2/2] test(runtime): move the consumer pins onto the converted refusal envelope (#9364) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit packages/runtime drives the hono adapter through the dispatcher plugin, and its integration tests pin the adapter's refusal bodies. Those pins still asserted the pre-conversion shapes. Adapter pins, moved — verified as adapter pins rather than another door's contract by reading what each case exists to prove: - dispatcher-plugin.multi-tenant-endpoint (2 sites) and dispatcher-plugin.ready — both assert "the transport's own 404 stands", one as the decline branch of environment resolution, one as an explicit anti-vacuity control that the harness mirrors prod. - dispatcher-plugin.endpoint-fallback — BARE_NOT_FOUND renamed TRANSPORT_NOT_FOUND (the name described the old shape) across 6 uses, plus the 405 case whose code/allowed moved into error / error.details. - mcp-skill-method-not-allowed — the boundary case now reads error.details.allowed, asserted by value (the four-verb set #7649's own fix produces) rather than by presence. ⚠️ The conversion also drained the POST/DELETE cases in that last file of their discriminating power without failing: they separated the two doors by `typeof body.error !== 'string'` and three top-level not.toHaveProperty checks, all four of which became true of the ADAPTER answer too once it gained the same envelope. Replaced with the distinction that still holds — the domain fills error.httpStatus and no details, the adapter the reverse — so the suite still fails in the world it exists to catch. Assertions are at least as strong as before; none weakened or skipped, no ratchet touched. check-route-envelope counters unchanged. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_012WKSnqAaoqtW3QX7SSf1Vk --- ...ugin.endpoint-fallback.integration.test.ts | 43 +++++++++++----- ....multi-tenant-endpoint.integration.test.ts | 12 +++-- ...ispatcher-plugin.ready.integration.test.ts | 7 ++- ...ethod-not-allowed.hono.integration.test.ts | 50 ++++++++++++++++--- 4 files changed, 86 insertions(+), 26 deletions(-) diff --git a/packages/runtime/src/dispatcher-plugin.endpoint-fallback.integration.test.ts b/packages/runtime/src/dispatcher-plugin.endpoint-fallback.integration.test.ts index 65f5624b15..43763ae934 100644 --- a/packages/runtime/src/dispatcher-plugin.endpoint-fallback.integration.test.ts +++ b/packages/runtime/src/dispatcher-plugin.endpoint-fallback.integration.test.ts @@ -19,8 +19,8 @@ * * The load-bearing assertion in most of these is a NEGATIVE one: that adding * this seam changed nothing for anybody who did not ask for it. The unmatched - * answers — the bare 404 and the 405 + `Allow` — must come back byte for byte - * for every path no declaration owns. Since the #5040 E7 publish flip that is + * answers — the transport's 404 and the 405 + `Allow` — must come back byte for + * byte for every path no declaration owns. Since the #5040 E7 publish flip that is * the assertion's whole weight: stacks CAN declare endpoints now, so "the * fallback stays silent unless a declaration matches" is a promise to live * deployments rather than a property of a surface nothing could reach. @@ -123,8 +123,18 @@ async function shutdown(kernel: LiteKernel | undefined) { ]); } -/** The transport's unmatched answer, captured from a boot with no seam armed. */ -const BARE_NOT_FOUND = { error: 'Not found' }; +/** + * The transport's unmatched answer, captured from a boot with no seam armed. + * + * The hono adapter's `unmatchedResponse()` — the declared refusal envelope + * since #9364, previously the bare `{ error: 'Not found' }`. Every use below + * is asserting "the transport answered, the seam did not", so the value has to + * track the adapter; what those cases pin is the ROUTING, not the wording. + */ +const TRANSPORT_NOT_FOUND = { + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, +}; describe('metadata slot carries no matchEndpoint — a fully working passthrough', () => { let kernel: LiteKernel; @@ -139,7 +149,7 @@ describe('metadata slot carries no matchEndpoint — a fully working passthrough it('answers an endpoint-shaped path with the transport\'s own 404, unchanged', async () => { const res = await fetch(`${baseUrl}/api/v1/apps/showcase/tasks`); expect(res.status).toBe(404); - expect(await res.json()).toEqual(BARE_NOT_FOUND); + expect(await res.json()).toEqual(TRANSPORT_NOT_FOUND); }); it('serves the dispatcher\'s own routes normally', async () => { @@ -158,7 +168,7 @@ describe('no metadata service at all', () => { it('answers 404 rather than failing the request', async () => { const res = await fetch(`${baseUrl}/api/v1/apps/showcase/tasks`); expect(res.status).toBe(404); - expect(await res.json()).toEqual(BARE_NOT_FOUND); + expect(await res.json()).toEqual(TRANSPORT_NOT_FOUND); }); }); @@ -198,7 +208,7 @@ describe('matcher present — the endpoint dispatch step (#5090)', () => { it('answers a MISS under the mount with the transport\'s 404, unchanged', async () => { const res = await fetch(`${baseUrl}/api/v1/apps/showcase/nope`); expect(res.status).toBe(404); - expect(await res.json()).toEqual(BARE_NOT_FOUND); + expect(await res.json()).toEqual(TRANSPORT_NOT_FOUND); expect(queries).toContainEqual({ path: '/api/v1/apps/showcase/nope', method: 'GET' }); }); @@ -206,7 +216,7 @@ describe('matcher present — the endpoint dispatch step (#5090)', () => { queries.length = 0; const res = await fetch(`${baseUrl}/api/v1/nope`); expect(res.status).toBe(404); - expect(await res.json()).toEqual(BARE_NOT_FOUND); + expect(await res.json()).toEqual(TRANSPORT_NOT_FOUND); const notFoundOnRoot = await fetch(`${baseUrl}/totally/unrouted`); expect(notFoundOnRoot.status).toBe(404); expect(queries, 'the endpoint step ran for a path outside `/api/v1/apps/`').toEqual([]); @@ -231,9 +241,16 @@ describe('matcher present — the endpoint dispatch step (#5090)', () => { const res = await fetch(`${baseUrl}/api/v1/apps/showcase/tasks`, { method: 'DELETE' }); expect(res.status).toBe(405); expect(res.headers.get('Allow')).toBe('PUT'); - const body = await res.json() as { code: string; allowed: string[] }; - expect(body.code).toBe('METHOD_NOT_ALLOWED'); - expect(body.allowed).toEqual(['PUT']); + // The adapter's 405, in the declared envelope since #9364 — `code` and + // the `allowed` hint moved from top-level siblings into `error` and + // `error.details` respectively. Same two facts, one level in. + const body = await res.json() as { + success: boolean; + error: { code: string; details: { allowed: string[] } }; + }; + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(['PUT']); }); it('answers an execution result, never the "nothing was wired" 501', async () => { @@ -518,11 +535,11 @@ describe('the wired chain — policies, then the real pipeline (#5129)', () => { queries.length = 0; const outside = await fetch(`${baseUrl}/api/v1/nope`); expect(outside.status).toBe(404); - expect(await outside.json()).toEqual(BARE_NOT_FOUND); + expect(await outside.json()).toEqual(TRANSPORT_NOT_FOUND); const missUnderMount = await fetch(`${baseUrl}/api/v1/apps/showcase/not-declared`); expect(missUnderMount.status).toBe(404); - expect(await missUnderMount.json()).toEqual(BARE_NOT_FOUND); + expect(await missUnderMount.json()).toEqual(TRANSPORT_NOT_FOUND); expect(queries).toEqual([{ path: '/api/v1/apps/showcase/not-declared', method: 'GET' }]); }); diff --git a/packages/runtime/src/dispatcher-plugin.multi-tenant-endpoint.integration.test.ts b/packages/runtime/src/dispatcher-plugin.multi-tenant-endpoint.integration.test.ts index 9d971096b2..a9b7dba271 100644 --- a/packages/runtime/src/dispatcher-plugin.multi-tenant-endpoint.integration.test.ts +++ b/packages/runtime/src/dispatcher-plugin.multi-tenant-endpoint.integration.test.ts @@ -256,11 +256,14 @@ describe('#5040 E5b — multi-tenant resolution finds no environment: decline + // single-environment host that never had a resolver. expect(resolverCalls).toEqual([undefined]); - // Declining writes nothing, so the bare Hono 404 stands. The host + // Declining writes nothing, so the transport's own 404 stands. The host // DECLARES this path and could have served it: without the branch this // is a 200 carrying `servedBy: 'host'`. expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); // The decline happened BEFORE the probe. A declaration must never be // consulted for a request that was not placed in an environment. @@ -284,7 +287,10 @@ describe('#5040 E5b — multi-tenant resolution finds no environment: decline + expect(resolverCalls).toEqual(['env-does-not-exist']); expect(res.status).toBe(404); - expect(await res.json()).toEqual({ error: 'Not found' }); + expect(await res.json()).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); expect(matchQueries).toEqual([]); }, 60_000); }); diff --git a/packages/runtime/src/dispatcher-plugin.ready.integration.test.ts b/packages/runtime/src/dispatcher-plugin.ready.integration.test.ts index b33e679287..5f7b075106 100644 --- a/packages/runtime/src/dispatcher-plugin.ready.integration.test.ts +++ b/packages/runtime/src/dispatcher-plugin.ready.integration.test.ts @@ -59,14 +59,17 @@ describe('GET /ready over a real HTTP server (integration)', () => { expect(body.data.status).toBe('ok'); }); - it('proves the harness mirrors prod: an unmounted path 404s with the Hono not-found body', async () => { + it('proves the harness mirrors prod: an unmounted path 404s with the adapter not-found body', async () => { // This is the exact response /ready produced BEFORE the fix. Asserting it // here shows the test would have failed against the old code (the /ready // assertion above would have returned this body), not passed vacuously. const res = await fetch(`${baseUrl}/api/v1/this-route-does-not-exist`); expect(res.status).toBe(404); const body = await res.json(); - expect(body).toEqual({ error: 'Not found' }); + expect(body).toEqual({ + success: false, + error: { code: 'ENDPOINT_NOT_FOUND', message: 'Not found' }, + }); }); it('returns 503 while the kernel is shutting down (drain signal)', async () => { diff --git a/packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts b/packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts index 82f57fd4f8..e10c6c6d32 100644 --- a/packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts +++ b/packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts @@ -25,6 +25,15 @@ import { createDispatcherPlugin } from './dispatcher-plugin.js'; * `{success:false, error:{code, message, httpStatus}}` carrying the documented * message "Method not allowed — use GET". * + * ⚠️ The quoted body above is the shape MEASURED IN #7627, and the adapter no + * longer emits it: #9364 converted `unmatchedResponse()` onto the same declared + * envelope, nesting `method`/`path`/`allowed` under `error.details`. The defect + * this file guards is unchanged — the wrong DOOR answering — but the two doors + * are no longer told apart by envelope shape, only by which optional member + * each fills (`httpStatus` for the domain, `details` for the adapter). See the + * note on the negative case below; several assertions here had to move for + * exactly that reason, and would otherwise have gone quietly vacuous. + * * ## Why the defect was invisible to the existing tests * * The 405 branch is NOT missing. `handleMcpSkillRequest` has had one since @@ -146,13 +155,23 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, it('POST does not answer with `unmatchedResponse()`\'s shape', async () => { const { body } = await call('POST'); - // The four keys that identify the adapter's unmatched-route answer. - // `error` as a STRING is the tell — the standard envelope nests an - // object there, so this assertion cannot be satisfied by both shapes. - expect(typeof body.error).not.toBe('string'); - expect(body).not.toHaveProperty('method'); - expect(body).not.toHaveProperty('path'); - expect(body).not.toHaveProperty('allowed'); + // ⚠️ The tell CHANGED with #9364, and the old one would now pass + // vacuously. Before that card the adapter's 405 answered + // `error: 'Method Not Allowed'` (a STRING) with `method`/`path`/ + // `allowed` as TOP-LEVEL siblings, so `typeof body.error !== 'string'` + // and three `not.toHaveProperty` checks genuinely separated the two + // doors. #9364 put the adapter's refusal into the same declared + // envelope, nesting its context under `error.details` — so all four of + // those assertions are now true of the ADAPTER answer as well, and + // would keep this test green in exactly the world it exists to catch. + // + // What still separates the doors is which OPTIONAL member each fills: + // the dispatcher's `buildApiError` carries `httpStatus` and no + // `details`; the adapter's `unmatchedResponse` carries + // `details.{method,path,allowed}` and no `httpStatus`. Both halves are + // asserted here so this case discriminates on its own. + expect(body.error).not.toHaveProperty('details'); + expect(body.error.httpStatus).toBe(405); }); // The `Allow` header CHANGES with this fix, which is worth stating exactly @@ -201,6 +220,21 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, it('PUT — unmounted — still falls through to the adapter (boundary, not a regression)', async () => { const { res, body } = await call('PUT'); expect(res.status).toBe(405); - expect(body).toHaveProperty('allowed'); + + // The adapter's own 405. Since #9364 it speaks the SAME declared + // envelope as the domain branch, so `success`/`error.code` no longer + // tell the two apart — `error.details.allowed` is what does, and it is + // the direct successor of the top-level `allowed` this case used to + // read. Asserted by VALUE, not by presence — and the value is the + // whole four-verb set because #7649's fix mounts GET/POST/DELETE, with + // Hono registering HEAD implicitly alongside GET. That is precisely + // why PUT is the boundary probe: it is the one verb left unmounted. + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.details.allowed).toEqual(['DELETE', 'GET', 'HEAD', 'POST']); + expect(body.error.details.method).toBe('PUT'); + expect(body.error.details.path).toBe(SKILL_PATH); + // …and it is NOT the domain branch's answer, which fills `httpStatus` + // and no `details`. + expect(body.error).not.toHaveProperty('httpStatus'); }); });