From a01d0e18e69c10356497ec04607e84f366af03f2 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:50:22 +0000 Subject: [PATCH 1/2] fix(runtime): route POST /mcp/skill to the dispatcher's own 405 branch (#7649) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/v1/mcp/skill` answered 405 with the hono adapter's hand-rolled `{error, code, message, method, path, allowed}` body instead of the standard `{success:false, error:{code, message, httpStatus}}` envelope carrying "Method not allowed — use GET". The 405 branch was not missing. `handleMcpSkillRequest` has had one since #3842 routed it through `buildApiError`. The defect was one layer above: `createDispatcherPlugin` mounted `${prefix}/mcp/skill` for GET only, so a non-GET request matched no route, Hono sent it to `notFound`, and the adapter's `unmatchedResponse()` answered first — leaving the domain branch dead code on this adapter. Mount `/mcp/skill` for the same verb set as its sibling `/mcp` (GET + POST + DELETE) so the mismatch reaches the branch that already exists. No second 405 implementation is added, and the GET happy path is untouched. Tests: a real-Hono integration suite pinning the envelope field by field (a status-only assertion passes in both worlds, which is why this defect survived the existing direct-call unit test), plus a registration assertion alongside the sibling /mcp one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0158ZQo7LiHSxGWpYKuPq1wu --- .../mcp-skill-method-not-allowed-envelope.md | 39 ++++ .../src/dispatcher-plugin.routes.test.ts | 21 ++ packages/runtime/src/dispatcher-plugin.ts | 40 +++- ...ethod-not-allowed.hono.integration.test.ts | 185 ++++++++++++++++++ 4 files changed, 277 insertions(+), 8 deletions(-) create mode 100644 .changeset/mcp-skill-method-not-allowed-envelope.md create mode 100644 packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts diff --git a/.changeset/mcp-skill-method-not-allowed-envelope.md b/.changeset/mcp-skill-method-not-allowed-envelope.md new file mode 100644 index 0000000000..d9c4fa550e --- /dev/null +++ b/.changeset/mcp-skill-method-not-allowed-envelope.md @@ -0,0 +1,39 @@ +--- +"@objectstack/runtime": patch +--- + +fix(runtime): `POST /api/v1/mcp/skill` answers the standard error envelope, not the adapter's hand-rolled 405 (#7649) + +A method mismatch on the public SKILL.md route returned a body no other error on +this API returns: + +```json +{ "error": "Method Not Allowed", "code": "METHOD_NOT_ALLOWED", + "message": "POST is not supported for /api/v1/mcp/skill. Allowed: GET.", + "method": "POST", "path": "/api/v1/mcp/skill", "allowed": ["GET"] } +``` + +instead of the standard `{success:false, error:{code, message, httpStatus}}` +carrying the documented message *"Method not allowed — use GET"*. A client +branching on `error.code` read `undefined`, because `error` was a string. + +**The 405 branch was never missing.** `handleMcpSkillRequest` has had one since +#3842 routed it through `buildApiError`. The defect was one layer above it: +`createDispatcherPlugin` mounted `${prefix}/mcp/skill` for **GET only**. Since +GET is the only method the route serves, that read as correct — but an unmounted +verb never reaches the dispatcher at all. Hono sends it to `notFound`, where the +adapter's `unmatchedResponse()` re-matches the path across verbs and answers 405 +with its own shape. The domain's branch was dead code on this adapter, and the +API had two 405 envelopes depending on which route you hit. + +`/mcp/skill` is now mounted for the same verb set as its sibling `/mcp` +(GET + POST + DELETE), so the mismatch reaches the branch that already exists. +No new 405 logic was written, and `GET /api/v1/mcp/skill` is untouched — same +200, same `text/markdown`, same `cache-control: no-store`. + +Note for callers that parse the old body: the `method`, `path` and `allowed` +keys are gone from this route's 405, and `error` is now an object. The `Allow` +response header remains the interoperable place to read the hint, and now +reads `GET` — the domain branch's own literal — where the adapter previously +derived `GET, HEAD` from its route table (Hono registers HEAD implicitly +beside every GET). `HEAD /api/v1/mcp/skill` is still served either way. diff --git a/packages/runtime/src/dispatcher-plugin.routes.test.ts b/packages/runtime/src/dispatcher-plugin.routes.test.ts index 31a9185431..9bbe136b32 100644 --- a/packages/runtime/src/dispatcher-plugin.routes.test.ts +++ b/packages/runtime/src/dispatcher-plugin.routes.test.ts @@ -59,6 +59,27 @@ describe('createDispatcherPlugin — HTTP route registration', () => { expect(routes).toContain('POST /api/v1/keys'); }); + // Regression (#7649): /mcp/skill was mounted for GET ONLY. The route serves + // GET and nothing else, so that looked right — but the dispatcher owns a 405 + // branch for the other verbs ("Method not allowed — use GET", built through + // `buildApiError` since #3842), and an unmounted verb never reaches it: Hono + // sends it to `notFound`, where the adapter's `unmatchedResponse()` answers + // 405 with its own hand-rolled `{error, code, message, method, path, allowed}` + // body. Same status, different envelope, and the domain branch dead code. + // Mounting the verbs is what routes the mismatch to the branch that exists. + // The envelope itself is pinned end-to-end in + // `mcp-skill-method-not-allowed.hono.integration.test.ts` — a status-only + // assertion cannot see this defect. + it('mounts /mcp/skill for the same verbs as /mcp so a method mismatch reaches the dispatcher 405', async () => { + const { server, routes } = makeFakeServer(); + const plugin = createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false }); + await plugin.start?.(makeCtx(server)); + + expect(routes).toContain('GET /api/v1/mcp/skill'); + expect(routes).toContain('POST /api/v1/mcp/skill'); + expect(routes).toContain('DELETE /api/v1/mcp/skill'); + }); + // Regression (framework #2217 seam #2): /ready shipped with a dispatch() // branch but NO server.() registration, so it 404'd over HTTP before // reaching the handler — the same class of bug as /mcp and /keys. /health and diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index c035d8cc2f..ba48314c13 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -916,14 +916,38 @@ export function createDispatcherPlugin(config: DispatcherPluginConfig = {}): Plu // Public SKILL.md download (env-customized portable Agent Skill). // Separate registration: `/mcp` above is an exact-path mount, so // the sub-path needs its own route to be reachable over HTTP. - server.get(`${prefix}/mcp/skill`, async (req: any, res: any) => { - try { - const result = await dispatcher.dispatch('GET', '/mcp/skill', req.body, req.query, { request: req }); - sendResult(result, res); - } catch (err: any) { - errorResponse(err, res); - } - }); + // + // [#7649] Mounted for the SAME method set as `/mcp` above rather + // than GET alone, even though GET is the only method this route + // SERVES. The domain owns a 405 branch for the rest + // (`handleMcpSkillRequest`: "Method not allowed — use GET", body + // built through `buildApiError` per #3842) — but a branch can only + // answer a mismatch that REACHES the dispatcher. With GET as the + // sole registration, Hono routed `POST /api/v1/mcp/skill` to + // `notFound`, where the adapter's `unmatchedResponse()` answered + // with its own `{error, code, message, method, path, allowed}` + // shape: a second, non-standard 405 envelope on the wire, and the + // domain branch dead code on this adapter. Registering the verbs + // hands the mismatch to the branch that already exists. + // + // The method set tracks `/mcp`'s deliberately: `server.get/post/ + // delete` are also the three verbs the observability Proxy above + // instruments, so a PUT/PATCH mount here would be both wider than + // the sibling route and silently un-instrumented. + const mountMcpSkill = (method: 'GET' | 'POST' | 'DELETE') => { + const register = method === 'GET' ? server.get : method === 'DELETE' ? server.delete : server.post; + register.call(server, `${prefix}/mcp/skill`, async (req: any, res: any) => { + try { + const result = await dispatcher.dispatch(method, '/mcp/skill', req.body, req.query, { request: req }); + sendResult(result, res); + } catch (err: any) { + errorResponse(err, res); + } + }); + }; + mountMcpSkill('GET'); + mountMcpSkill('POST'); + mountMcpSkill('DELETE'); server.post(`${prefix}/keys`, async (req: any, res: any) => { try { 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 new file mode 100644 index 0000000000..ed69043651 --- /dev/null +++ b/packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts @@ -0,0 +1,185 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { LiteKernel, Plugin, PluginContext } from '@objectstack/core'; +import { HonoServerPlugin } from '@objectstack/plugin-hono-server'; +import type { IHttpServer } from '@objectstack/spec/contracts'; + +import { createDispatcherPlugin } from './dispatcher-plugin.js'; + +/** + * End-to-end regression for #7649 — `POST /api/v1/mcp/skill` answered 405 with + * the WRONG envelope. + * + * ## What was measured (QA run #7627) + * + * ``` + * POST /api/v1/mcp/skill + * → HTTP 405 + * {"error":"Method Not Allowed","code":"METHOD_NOT_ALLOWED", + * "message":"POST is not supported for /api/v1/mcp/skill. Allowed: GET.", + * "method":"POST","path":"/api/v1/mcp/skill","allowed":["GET"]} + * ``` + * + * …instead of the standard dispatcher envelope + * `{success:false, error:{code, message, httpStatus}}` carrying the documented + * message "Method not allowed — use GET". + * + * ## Why the defect was invisible to the existing tests + * + * The 405 branch is NOT missing. `handleMcpSkillRequest` has had one since + * #3842 routed it through `buildApiError`, and + * `http-dispatcher.mcp.test.ts` covers it — by calling + * `dispatcher.handleMcpSkill('POST', …)` DIRECTLY. That call cannot observe the + * defect, because the defect is one layer above the dispatcher: the plugin + * mounted `${prefix}/mcp/skill` for GET only, so a POST matched no route at + * all, Hono routed it to `notFound`, and the hono adapter's + * `unmatchedResponse()` — which re-matches the path across verbs and answers + * 405 with its own hand-rolled body — replied first. The domain's branch was + * dead code on this adapter. + * + * That is exactly the class of bug `dispatcher-plugin.routes.test.ts` opens by + * naming ("unit tests called the handlers directly, hiding it"), with one extra + * turn of the screw: here the status was already RIGHT. Only the body differed, + * so a test asserting `res.status === 405` passes in both worlds. Hence this + * suite drives a REAL Hono server over real `fetch` and asserts the BODY. + * + * ## Shape of the suite + * + * `LiteKernel` (as in `auth-unknown-subpath.hono.integration.test.ts`): this is + * about the HTTP mount seam, and a full `ObjectKernel` would demand a `data` + * service no assertion here reads. The fake `mcp` service implements only + * `renderSkill`, which is all `GET /mcp/skill` calls — enough for the happy-path + * control that proves the fix did not disturb the method the route serves. + */ + +/** The standard envelope's message for this branch — contract, not prose. */ +const EXPECTED_MESSAGE = 'Method not allowed — use GET'; +const SKILL_PATH = '/api/v1/mcp/skill'; +const SKILL_MARKER = 'OBJECTSTACK_SKILL_FIXTURE'; + +/** An `mcp` service that can render the skill and nothing else. */ +function fakeMcpPlugin(): Plugin { + return { + name: 'com.objectstack.test.fake-mcp-skill', + version: '1.0.0', + init: async (ctx: PluginContext) => { + ctx.registerService('mcp', { + renderSkill: (o: any) => + `---\nname: objectstack\n---\n\n# ${SKILL_MARKER}\n\nMCP: ${o?.mcpUrl ?? ''}\n`, + }); + }, + }; +} + +describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, #7649)', () => { + let kernel: LiteKernel; + let baseUrl: string; + const prevEnabled = process.env.OS_MCP_SERVER_ENABLED; + + beforeAll(async () => { + // Default-on; set explicitly so a stray env var in the runner cannot + // turn every assertion below into a 404 that still "passes" a laxer read. + delete process.env.OS_MCP_SERVER_ENABLED; + + kernel = new LiteKernel(); + kernel.use(fakeMcpPlugin()); + // port 0 → OS-assigned free port; resolved via getPort() after listening. + kernel.use(new HonoServerPlugin({ port: 0, cors: false })); + kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false, requireAuth: false })); + + await kernel.bootstrap(); + + const httpServer = kernel.getService('http.server'); + baseUrl = `http://127.0.0.1:${httpServer.getPort!()}`; + }, 30_000); + + afterAll(async () => { + if (prevEnabled === undefined) delete process.env.OS_MCP_SERVER_ENABLED; + else process.env.OS_MCP_SERVER_ENABLED = prevEnabled; + if (kernel) { + await Promise.race([ + kernel.shutdown(), + new Promise((resolve) => setTimeout(resolve, 10_000)), + ]); + } + }, 30_000); + + // ── ① the defect ──────────────────────────────────────────────────────── + it('POST returns {success:false, error:{code, message, httpStatus}} — not the adapter\'s hand-rolled body', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'POST' }); + const body = await res.json(); + + expect(res.status).toBe(405); + // The envelope, field by field — the whole defect is that these differ, + // so the status assertion above proves nothing on its own. + expect(body.success).toBe(false); + expect(body.error).toBeTypeOf('object'); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.message).toBe(EXPECTED_MESSAGE); + expect(body.error.httpStatus).toBe(405); + }); + + it('POST does not answer with `unmatchedResponse()`\'s shape', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'POST' }); + const body = await res.json(); + + // 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 `Allow` header CHANGES with this fix, which is worth stating exactly + // rather than filing under "unchanged". Before, the adapter derived it from + // its own route table and Hono registers HEAD implicitly alongside every + // GET, so the hint read `GET, HEAD`. Now the domain branch's own literal + // answers, and it says `GET` — matching the message next to it ("use GET") + // and the one verb this route actually serves. HEAD is still served; the + // hint just no longer enumerates it. + it('answers Allow: GET — the domain branch\'s literal, not the adapter\'s derived `GET, HEAD`', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'POST' }); + expect(res.status).toBe(405); + expect(res.headers.get('allow')).toBe('GET'); + }); + + // DELETE is mounted for the same reason POST is — `/mcp` carries all three + // verbs, and one of them answering a different 405 envelope than the other + // is the drift this issue closes. + it('DELETE gets the same standard envelope', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'DELETE' }); + const body = await res.json(); + + expect(res.status).toBe(405); + expect(body.success).toBe(false); + expect(body.error.code).toBe('METHOD_NOT_ALLOWED'); + expect(body.error.message).toBe(EXPECTED_MESSAGE); + expect(body.error.httpStatus).toBe(405); + }); + + // ── ② positive control: the happy path is untouched ───────────────────── + it('GET still serves the SKILL.md as text/markdown, anonymously', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'GET' }); + const text = await res.text(); + + expect(res.status).toBe(200); + expect(res.headers.get('content-type')).toContain('text/markdown'); + expect(res.headers.get('cache-control')).toBe('no-store'); + expect(text).toContain(SKILL_MARKER); + // Derived from the request host — the auth service is absent here. + expect(text).toContain(`${baseUrl.replace('http://', 'http://')}/api/v1/mcp`); + }); + + // A verb with no mount at all still falls to the adapter, and should: + // `unmatchedResponse()` is the correct owner of a route that does not + // exist under that verb. This pins the BOUNDARY of the fix rather than + // claiming the adapter answer is wrong everywhere. + it('PUT — unmounted — still falls through to the adapter (boundary, not a regression)', async () => { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'PUT' }); + expect(res.status).toBe(405); + expect(await res.json()).toHaveProperty('allowed'); + }); +}); From 5f069e6cdf46c2c772e6c11f3042da71aad99a04 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:33:26 +0000 Subject: [PATCH 2/2] fix(runtime): make the #7649 regression test type-clean for check:type-check-debt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new test file added +11 raw tsc errors to `@objectstack/runtime`'s TEST_DEBT measurement (227 -> 238). `packages/runtime/tsconfig.json` excludes `**/*.test.ts`, so `pnpm --filter @objectstack/runtime typecheck` never compiled the file; only `check:type-check-debt --re-measure`, which re-runs tsc with that exclusion dropped, can see the test layer. The ledger is a shrink-only ratchet (#5278), so the fix is the file, not the number. The 11 split two ways: - 10x TS18046 `'body' is of type 'unknown'` — `Response.json()` returns `unknown`. Reads now go through one `call()` helper that casts once to an open record. Deliberately NOT a narrow interface: this suite exists because two different body shapes can arrive on this path, and one case asserts keys that must NOT exist, so a type admitting only the correct envelope would encode the conclusion under test. - 1x TS2353 `'requireAuth' does not exist in type 'DispatcherPluginConfig'` — copied from a sibling suite. The key is dead: nothing reads it (the deployment-wide gate was removed), it was silently ignored, and the route under test is public anyway. Dropped rather than cast away. No assertion changed. Every envelope check is still a runtime assertion on the same field, and the reverse verification still goes red in the same places. Also dropped a no-op `.replace('http://', 'http://')` in the GET control. Measured after: 227, equal to the recorded ledger entry, 0 from these files. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_0158ZQo7LiHSxGWpYKuPq1wu --- ...ethod-not-allowed.hono.integration.test.ts | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) 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 ed69043651..82f57fd4f8 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 @@ -86,7 +86,14 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, kernel.use(fakeMcpPlugin()); // port 0 → OS-assigned free port; resolved via getPort() after listening. kernel.use(new HonoServerPlugin({ port: 0, cors: false })); - kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false, requireAuth: false })); + // No `requireAuth: false` here, though the sibling integration suites in + // this package still pass one: `DispatcherPluginConfig` has no such field + // (the deployment-wide gate was removed — see the comment at + // `dispatcher-plugin.ts:209` and `http-dispatcher.requireauth.test.ts:56`, + // "There is no `requireAuth: false` any more"). It was silently ignored, + // and copying it here bought nothing but a type error. The route under + // test is public by design, so nothing needs relaxing. + kernel.use(createDispatcherPlugin({ prefix: '/api/v1', securityHeaders: false })); await kernel.bootstrap(); @@ -105,10 +112,26 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, } }, 30_000); + /** + * Drive the route and parse the body. + * + * `Response.json()` is typed `unknown`, and there is no honest interface to + * narrow it to HERE: this suite exists precisely because **two different + * body shapes** can arrive on this path, and one of the cases reads keys + * that must NOT exist. A type admitting only the correct envelope would + * encode the very conclusion the suite is meant to prove, and would make + * the negative case unwritable. So the cast is to an open record and every + * assertion below stays a RUNTIME assertion — nothing is checked by the + * compiler here that the wire is not also checked for. + */ + async function call(method: string): Promise<{ res: Response; body: Record }> { + const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method }); + return { res, body: (await res.json()) as Record }; + } + // ── ① the defect ──────────────────────────────────────────────────────── it('POST returns {success:false, error:{code, message, httpStatus}} — not the adapter\'s hand-rolled body', async () => { - const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'POST' }); - const body = await res.json(); + const { res, body } = await call('POST'); expect(res.status).toBe(405); // The envelope, field by field — the whole defect is that these differ, @@ -121,8 +144,7 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, }); it('POST does not answer with `unmatchedResponse()`\'s shape', async () => { - const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'POST' }); - const body = await res.json(); + 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 @@ -150,8 +172,7 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, // verbs, and one of them answering a different 405 envelope than the other // is the drift this issue closes. it('DELETE gets the same standard envelope', async () => { - const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'DELETE' }); - const body = await res.json(); + const { res, body } = await call('DELETE'); expect(res.status).toBe(405); expect(body.success).toBe(false); @@ -170,7 +191,7 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, expect(res.headers.get('cache-control')).toBe('no-store'); expect(text).toContain(SKILL_MARKER); // Derived from the request host — the auth service is absent here. - expect(text).toContain(`${baseUrl.replace('http://', 'http://')}/api/v1/mcp`); + expect(text).toContain(`${baseUrl}/api/v1/mcp`); }); // A verb with no mount at all still falls to the adapter, and should: @@ -178,8 +199,8 @@ describe('POST /api/v1/mcp/skill answers the standard 405 envelope (integration, // exist under that verb. This pins the BOUNDARY of the fix rather than // claiming the adapter answer is wrong everywhere. it('PUT — unmounted — still falls through to the adapter (boundary, not a regression)', async () => { - const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method: 'PUT' }); + const { res, body } = await call('PUT'); expect(res.status).toBe(405); - expect(await res.json()).toHaveProperty('allowed'); + expect(body).toHaveProperty('allowed'); }); });