fix(runtime): route POST /api/v1/mcp/skill to the dispatcher's own 405 branch

.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.

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.<verb>() registration, so it 404'd over HTTP before
   // reaching the handler — the same class of bug as /mcp and /keys. /health and

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 {

packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts

@@ -0,0 +1,206 @@
+// 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 ?? '<YOUR_ENV_MCP_URL>'}\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 }));
+        // 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();
+
+        const httpServer = kernel.getService<IHttpServer>('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<void>((resolve) => setTimeout(resolve, 10_000)),
+            ]);
+        }
+    }, 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<string, any> }> {
+        const res = await fetch(`${baseUrl}${SKILL_PATH}`, { method });
+        return { res, body: (await res.json()) as Record<string, any> };
+    }
+
+    // ── ① the defect ────────────────────────────────────────────────────────
+    it('POST returns {success:false, error:{code, message, httpStatus}} — not the adapter\'s hand-rolled body', async () => {
+        const { res, body } = await call('POST');
+
+        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 { 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 `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, body } = await call('DELETE');
+
+        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}/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, body } = await call('PUT');
+        expect(res.status).toBe(405);
+        expect(body).toHaveProperty('allowed');
+    });
+});