mcp-http-surface (a): POST /api/v1/mcp/skill returns a hand-rolled 405 envelope, not the standard API error shape

[huangyiirene]

Symptom

POST /api/v1/mcp/skill (a method mismatch — the route is GET-only) answers 405 with the wrong envelope.

• Observed: a hand-rolled body {error, code, message, method, path, allowed}.
• Expected: the standard {success:false, error:{code, message, httpStatus}} shape with the documented message "Method not allowed — use GET" (the 405 branch routed through buildApiError by The dispatcher puts the HTTP status in error.code and parks the real code in details — pinned in #3687, still unfixed #3842).

Root cause

dispatcher-plugin.ts (packages/runtime/src/dispatcher-plugin.ts:919) registers the skill route with server.get(${prefix}/mcp/skill, …) only — unlike /mcp, which is mounted for POST+GET+DELETE (the method-keyed register at line ~902). Because no POST handler is registered for /mcp/skill, a method mismatch never reaches the dispatcher: Hono routes it to notFound and the hono adapter's unmatchedResponse() answers with its own envelope. The domain's 405 branch (via buildApiError, #3842) is dead code on this adapter.

Still present on origin/main (dispatcher-plugin.ts:919 is server.get(...) only).

Reproduction

Boot a showcase with the MCP server available, then:

curl -X POST http://<host>/api/v1/mcp/skill

Expected {success:false, error:{code, message, httpStatus:405}} / "Method not allowed — use GET". Actual {error, code, message, method, path, allowed} from unmatchedResponse().

Source

Extracted from the QA run #7627 (framework 92f26f7, console 6314e87f).

[os-help]

🔒 Claimed by the domain:cli PM seat (seat post #6024), round 7.

• Session: session_0158ZQo7LiHSxGWpYKuPq1wu
• Branch: claude/issue-7649-mcp-skill-405-envelope

Dispatching to a developer agent now. If you are another agent reading this: the assignee field cannot tell you whose claim this is — this comment can. A claim under a different session ID means the card is taken regardless of what the assignee says; please pick another.

Scope as dispatched: register the POST (and any other non-GET method the /mcp mount already handles) for ${prefix}/mcp/skill in packages/runtime/src/dispatcher-plugin.ts:919 so a method mismatch reaches the dispatcher's existing 405 branch via buildApiError (#3842) instead of falling through Hono's notFound → unmatchedResponse(). The domain-side 405 branch already exists and is currently dead code on this adapter; the fix is to stop bypassing it, not to write a second one. Wants a regression test that pins the standard {success:false, error:{code, message, httpStatus:405}} envelope on POST /api/v1/mcp/skill, since the whole defect is that the envelope silently differs.

---

Generated by Claude Code

[os-help]

{
  "issue": 7649,
  "status": "done",
  "branch": "claude/issue-7649-mcp-skill-405-envelope",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/7790",
  "pr_url": "https://github.com/objectstack-ai/objectstack/pull/7790",
  "premise_still_valid": true,
  "summary": "Premise confirmed on origin/main exactly as filed: dispatcher-plugin.ts:919 mounted `${prefix}/mcp/skill` with server.get only, so a non-GET request matched no route, Hono routed it to notFound, and the hono adapter's unmatchedResponse() answered 405 with its own {error, code, message, method, path, allowed} body — leaving handleMcpSkillRequest's buildApiError 405 branch (#3842) dead code on this adapter. Fixed by mounting /mcp/skill for the same verb set as its sibling /mcp (GET+POST+DELETE) via a mountMcpSkill helper that mirrors mountMcp at ~902; no second 405 implementation was written. POST and DELETE now return {success:false, error:{code:'METHOD_NOT_ALLOWED', message:'Method not allowed — use GET', httpStatus:405}}. GET is untouched (200, text/markdown, cache-control: no-store), verified by a positive control. One behaviour change worth PM attention: the Allow header now reads 'GET' (the domain branch's literal) where the adapter previously derived 'GET, HEAD' — HEAD is still served, only the hint no longer enumerates it. PUT/PATCH deliberately stay adapter-owned and are pinned as a boundary test; see scope_deviations.",
  "tests": "NEW packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts — a REAL Hono server over real fetch (6 cases), because the pre-existing coverage (http-dispatcher.mcp.test.ts '405s non-GET with an Allow header') calls dispatcher.handleMcpSkill('POST', …) directly and asserts status+Allow only; it was green throughout the defect, since the status was ALWAYS 405 and only the body differed. Asserts success/error.code/error.message/error.httpStatus field by field, plus the absence of unmatchedResponse()'s method/path/allowed keys and that error is not a string. ALSO added a registration assertion in dispatcher-plugin.routes.test.ts beside the existing /mcp one. RESULTS: `npx vitest run --reporter=verbose src/mcp-skill-method-not-allowed.hono.integration.test.ts src/dispatcher-plugin.routes.test.ts` -> 'Test Files 2 passed (2) / Tests 21 passed (21)'. Full affected package: `pnpm --filter @objectstack/runtime test` -> 'Test Files 130 passed (130) / Tests 2036 passed (2036)'. Cross-adapter contract: `pnpm --filter @objectstack/http-conformance test` -> 'Test Files 4 passed (4) / Tests 72 passed (72)' (first attempt failed to LOAD with 'Failed to resolve entry for package @objectstack/runtime' — a missing dist, not an assertion; green after `pnpm --filter @objectstack/runtime build`). Typecheck: `pnpm --filter @objectstack/runtime typecheck` -> tsc --noEmit, clean. Lint: `npx eslint --no-inline-config` on the 3 changed files -> exit 0. `node scripts/check-nul-bytes.mjs` -> OK (7131 files); `--self-test` -> 75 assertions pass. REVERSE VERIFICATION (reverted only the source file via `git checkout HEAD~1 -- dispatcher-plugin.ts`, kept the tests; NO git stash used): 5 failed / 16 passed, in the predicted direction — POST envelope red ('expected undefined to be false', i.e. body.success absent), unmatched-shape red ('expected string not to be string'), DELETE envelope red, registration red ('expected [ …(54) ] to include POST /api/v1/mcp/skill'); GET happy path GREEN and PUT-boundary GREEN as predicted. ONE PREDICTION WAS WRONG and is reported rather than smoothed over: the Allow-header case also went red, with 'expected GET, HEAD to be GET' — the header does not merely survive the fix, it changes value, because pre-fix the adapter derived Allow from its route table and Hono registers HEAD implicitly beside every GET. I corrected the test name and the changeset to state that transition instead of claiming the header is unchanged.",
  "checks": {
    "lint": "PASS — npx eslint --no-inline-config packages/runtime/src/{dispatcher-plugin.ts,dispatcher-plugin.routes.test.ts,mcp-skill-method-not-allowed.hono.integration.test.ts}, exit 0, no output. NOTE: @objectstack/runtime has no per-package `lint` script (ERR_PNPM_RECURSIVE_RUN_NO_SCRIPT); root lint is `eslint . --no-inline-config`, scoped here to the changed files per the local-verification-scope rule.",
    "typecheck": "PASS — @objectstack/runtime@17.0.0-rc.6 typecheck, `tsc --noEmit`, no diagnostics.",
    "test": "PASS — runtime: Test Files 130 passed (130), Tests 2036 passed (2036). http-conformance: Test Files 4 passed (4), Tests 72 passed (72). Targeted: 21/21.",
    "nul_bytes": "PASS — check-nul-bytes: OK (scanned 7131 text files); --self-test: 75 assertions.",
    "ci": "not_waited — reported at draft-PR time per #6644 L2; PM owns CI convergence and the ready-flip."
  },
  "files_changed": [
    "packages/runtime/src/dispatcher-plugin.ts — replaced the GET-only server.get mount with a mountMcpSkill('GET'|'POST'|'DELETE') helper mirroring mountMcp at ~902",
    "packages/runtime/src/mcp-skill-method-not-allowed.hono.integration.test.ts — NEW, 6 real-Hono end-to-end cases",
    "packages/runtime/src/dispatcher-plugin.routes.test.ts — added a /mcp/skill verb-registration assertion",
    ".changeset/mcp-skill-method-not-allowed-envelope.md — NEW, @objectstack/runtime patch"
  ],
  "tests_added": 7,
  "open_questions": [],
  "scope_deviations": [
    "Verb set limited to GET+POST+DELETE (mirroring /mcp) rather than also PUT/PATCH. Two reasons: the issue and claim comment both scope it to 'any other non-GET method the /mcp mount already handles', and server.get/post/delete are exactly the three verbs the plugin's own observability Proxy instruments — a PUT/PATCH mount would be wider than the sibling route AND silently un-instrumented. PUT/PATCH therefore still fall to the adapter's unmatchedResponse(), which is correct behaviour for a verb the route does not serve. Pinned explicitly as a boundary test rather than left implicit. Not filed as a finding: nothing a user hits, and it is a pre-existing property of the /mcp family, not introduced here.",
    "Added a registration assertion to dispatcher-plugin.routes.test.ts beyond the requested envelope regression test — that file exists for exactly this bug class and already carries the /mcp row; a sibling row is cheap and makes the mount itself, not only its downstream effect, hard to regress.",
    "Also ran @objectstack/http-conformance (not named in the dispatch) because it validates the cross-adapter IHttpServer unmatched-request contract that unmatchedResponse() implements — the surface this change moves work away from.",
    "Did NOT enumerate the full check:* farm locally, per the local-verification-scope rule; ran the affected packages' suites plus check:nul-bytes and eslint on the changed files.",
    "Reverse verification used `git checkout HEAD~1 -- <path>`; git stash was not used anywhere (shared stash stack)."
  ],
  "out_of_scope_findings": [],
  "blockers": []
}

Notes for the PM.

1. Report schema conflict, surfaced rather than silently resolved. The dispatch prompt asked for issue / branch / pr_url / files_changed / tests_added / checks / scope_deviations / blockers; the standing os-dev contract specifies issue / status / branch / pr / premise_still_valid / summary / tests / open_questions / out_of_scope_findings. I emitted the union so either parser resolves, rather than picking a side.
2. One wire-visible change beyond the envelope: Allow goes from GET, HEAD to GET on this route's 405. Documented in the changeset and the PR body; flagging it here so it is a decision you have seen rather than one you inherit.
3. Branch is based on 7a8476f09; main has since advanced to 098b6291. No conflicts expected (one file, one hunk), but the merge is yours.

---

Generated by Claude Code

---

Generated by Claude Code