-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(@mcp/hono): mcpHonoHandler mount helper (handleHttp-backed) #2074
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fweinberger/r4-backchannel
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,44 @@ | ||
| import type { Context } from 'hono'; | ||
| import type { AuthInfo, Dispatchable, HandleHttpOptions } from '@modelcontextprotocol/server'; | ||
| import { handleHttp } from '@modelcontextprotocol/server'; | ||
| import type { Context, Handler } from 'hono'; | ||
| import { Hono } from 'hono'; | ||
|
|
||
| import { hostHeaderValidation, localhostHostValidation } from './middleware/hostHeaderValidation.js'; | ||
|
|
||
| /** | ||
| * Hono context variables {@linkcode mcpHonoHandler} reads. Upstream middleware | ||
| * may set `parsedBody` ({@linkcode createMcpHonoApp} does this for JSON requests) | ||
| * and `authInfo` (e.g. a bearer-token verifier) before this handler runs. | ||
| */ | ||
| export type McpHonoVariables = { parsedBody?: unknown; authInfo?: AuthInfo }; | ||
|
|
||
| /** | ||
| * Mounts an `McpServer` (or any `Protocol` subclass) as a Hono `Handler`. | ||
| * Each request flows through `mcp.dispatch()` directly via {@linkcode handleHttp}; | ||
| * `mcp.connect()` and a transport instance are not used. | ||
| * | ||
| * Reads `c.get('parsedBody')` (set by {@linkcode createMcpHonoApp}'s JSON middleware) | ||
| * and `c.get('authInfo')` (set by an upstream auth middleware) when present. | ||
| * | ||
| * ```ts | ||
| * import { McpServer, SessionCompat } from '@modelcontextprotocol/server'; | ||
| * import { createMcpHonoApp, mcpHonoHandler } from '@modelcontextprotocol/hono'; | ||
| * | ||
| * const mcp = new McpServer({ name: 's', version: '1.0.0' }); | ||
| * const app = createMcpHonoApp(); | ||
| * app.all('/mcp', mcpHonoHandler(mcp, { session: new SessionCompat() })); | ||
| * ``` | ||
| */ | ||
| export function mcpHonoHandler(mcp: Dispatchable, options?: HandleHttpOptions): Handler<{ Variables: McpHonoVariables }> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 This PR adds two new public exports ( Extended reasoning...What the issue is. Step-by-step proof.
Why nothing currently catches it. The repo's review checklist says "New feature: verify prose documentation is added (not just JSDoc)" and the Documentation & Changesets recurring catch says to flag prose that the diff doesn't back. JSDoc was added (hono.ts:15–31), but neither the package README nor a changeset was, and the PR body actively claims no API change. Tests and typecheck pass because none of this is enforced by CI beyond changeset-bot's non-blocking warning. Impact. Without a changeset, the next How to fix.
|
||
| const handler = handleHttp(mcp, options); | ||
| return c => { | ||
| const parsedBody = c.get('parsedBody'); | ||
| const authInfo = c.get('authInfo'); | ||
| const extra = authInfo !== undefined || parsedBody !== undefined ? { authInfo, parsedBody } : undefined; | ||
| return handler(c.req.raw, extra); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Options for creating an MCP Hono application. | ||
| */ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: per CLAUDE.md § "JSDoc @example Code Snippets", this code block should be an
@examplewith ats source="./hono.examples.ts#…"fence pulling from a type-checked companion file (asexpress.ts,fastify.ts, andnode/streamableHttp.tsall do). Right now there's nopackages/middleware/hono/src/hono.examples.ts, so this inline snippet isn't type-checked and will silently rot ifmcpHonoHandler/SessionCompatsignatures change. Consider adding ahono.examples.tswith e.g. a#region mcpHonoHandler_basicUsageand referencing it viasource=.Extended reasoning...
What the issue is. CLAUDE.md (§ JSDoc @example Code Snippets, lines ~44–48) states that JSDoc example code should pull type-checked code from companion
.examples.tsfiles using fences of the formts source="./file.examples.ts#regionName", kept in sync viapnpm sync:snippets. The newmcpHonoHandlerJSDoc instead embeds a plaintsfence with an inline, hand-written snippet (lines 23–30), with no@exampletag and nosource=attribute.Why the convention exists / why it matters here. The point of the
source=+.examples.tsmechanism is that the snippet is real, compiled TypeScript — ifmcpHonoHandler's signature changes, orSessionCompatis renamed, orcreateMcpHonoAppgains a required option,tscfails onhono.examples.tsand the breakage is caught in CI. An inline snippet has none of that protection: it can drift from the actual API and nothing in the build will notice. This is the first JSDoc code block inhono.ts, so it also sets the precedent for the package.Sibling-package precedent. Every other middleware adapter already follows the convention:
packages/middleware/express/src/express.ts→@example+source="./express.examples.ts#…", backed byexpress.examples.tspackages/middleware/fastify/src/fastify.ts→@example+source="./fastify.examples.ts#…", backed byfastify.examples.tspackages/middleware/node/src/streamableHttp.ts→source="./streamableHttp.examples.ts#…", backed bystreamableHttp.examples.ts@modelcontextprotocol/honois the only middleware package with no*.examples.tsfile at all (verified via glob), so this PR is a natural place to add one rather than diverge further.Step-by-step illustration of the rot risk.
mcpHonoHandler(mcp, { session: new SessionCompat() }).SessionCompat→SessionManager, or changes the option key fromsessiontosessions.pnpm build/pnpm typecheckrun cleanly because nothing compiles this string — it's just a comment.pnpm sync:snippetsalso does nothing, because there's nosource=attribute pointing at a region to re-sync.hono.examples.tsregion, step 3 would instead fail the build.Suggested fix. Add
packages/middleware/hono/src/hono.examples.tscontaining the snippet inside// #region mcpHonoHandler_basicUsage/// #endregion, then change the JSDoc to:and run
pnpm sync:snippets.Severity. This is a docs/style convention nit, not a correctness defect — the handler itself is fine. Filing as nit. (Minor caveat: a couple of pre-existing bare
tsfences exist elsewhere in the repo, e.g.node/streamableHttp.ts:34andexpress/auth/metadataRouter.ts:108, so enforcement isn't absolute — but the dominant pattern across the middleware adapters is clearlysource=+.examples.ts, and CLAUDE.md documents it as the expected approach.)