Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/middleware/express/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"test:watch": "vitest"
},
"dependencies": {
"@hono/node-server": "catalog:runtimeServerOnly",
"cors": "catalog:runtimeServerOnly"
},
Comment on lines 45 to 48
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 @hono/node-server declares hono as a required peer dependency, so consumers of @modelcontextprotocol/express will now get an unmet-peer warning (or a hard error under strict-peer-deps) for hono even though an Express app has no reason to install it. The sibling @modelcontextprotocol/node adapter handles this exact case by declaring hono as an optional peer (packages/middleware/node/package.json:56-64) — this package should add the same peerDependencies / peerDependenciesMeta entries.

Extended reasoning...

What the issue is

This PR adds @hono/node-server to @modelcontextprotocol/express's dependencies (to use getRequestListener for the Node IncomingMessage ↔ Web Request bridge). However, @hono/node-server declares hono as a required peer dependency — confirmed by pnpm-lock.yaml resolving it as 1.19.11(hono@4.12.9), and explicitly noted in this repo's own .changeset/hono-peer-optional.md ('@hono/node-server itself still declares hono as a hard peer'). The express package.json adds the dependency but does not forward/suppress that transitive peer requirement.

How it manifests

When a downstream user runs npm i @modelcontextprotocol/express (or pnpm/yarn equivalents):

  1. The package manager installs @hono/node-server as a transitive dependency of @modelcontextprotocol/express.
  2. It checks @hono/node-server's peer requirements and finds hono: ^4.
  3. hono is not in the consumer's tree (an Express user has no reason to install Hono), so the manager emits an unmet-peer-dependency warning.
  4. Under pnpm with strict-peer-dependencies: true, or npm install --strict-peer-deps, this becomes a hard install failure.

There is no runtime impact — getRequestListener doesn't actually need hono — but the install-time noise/failure is real and is directly introduced by this PR's new dependency.

Why existing code doesn't prevent it

The repo has already solved this exact problem once. packages/middleware/node/package.json:56-64 wraps the same @hono/node-server dependency for the same getRequestListener usage and declares:

"peerDependencies": { "hono": "catalog:runtimeServerOnly" },
"peerDependenciesMeta": { "hono": { "optional": true } }

with the rationale documented in .changeset/hono-peer-optional.md. This PR copies the @hono/node-server dependency from the node adapter but omits the matching peer-dep handling — a partial application of an established sibling-package pattern.

Step-by-step proof

  1. packages/middleware/express/package.json after this PR: dependencies contains @hono/node-server; no hono entry anywhere.
  2. pnpm-lock.yaml resolves @hono/node-server as 1.19.11(hono@4.12.9) — the parenthesized peer means @hono/node-server's manifest has peerDependencies: { hono: ^4 } with no peerDependenciesMeta marking it optional.
  3. A consumer project containing only express + @modelcontextprotocol/express + @modelcontextprotocol/server therefore has no hono in its tree.
  4. npm install --strict-peer-deps (or pnpm strict mode) → ERESOLVE / unmet peer hono@^4; default mode → warning.

Fix

Add to packages/middleware/express/package.json, mirroring the node adapter:

"peerDependencies": {
    "@modelcontextprotocol/server": "workspace:^",
    "express": "^4.18.0 || ^5.0.0",
    "hono": "catalog:runtimeServerOnly"
},
"peerDependenciesMeta": {
    "hono": { "optional": true }
}

Note (per the existing changeset): even with this, some package managers may still surface the upstream @hono/node-server warning, so this is primarily about consistency with the node adapter and signaling intent to consumers — hence filed as a nit rather than a blocking bug.

"peerDependencies": {
Expand Down
36 changes: 35 additions & 1 deletion packages/middleware/express/src/express.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,42 @@
import type { Express } from 'express';
import { getRequestListener } from '@hono/node-server';
import type { AuthInfo, Dispatchable, HandleHttpOptions } from '@modelcontextprotocol/server';
import { handleHttp } from '@modelcontextprotocol/server';
import type { Express, RequestHandler } from 'express';
import express from 'express';

import { hostHeaderValidation, localhostHostValidation } from './middleware/hostHeaderValidation.js';

/**
* Mounts an `McpServer` (or any `Protocol` subclass) as an Express `RequestHandler`.
* Each request flows through `mcp.dispatch()` directly via {@linkcode handleHttp};
* `mcp.connect()` and a transport instance are not used.
*
* Reads `req.auth` (set by {@linkcode index.requireBearerAuth | requireBearerAuth}) and `req.body` (set by
* `express.json()`, which {@linkcode createMcpExpressApp} installs by default).
*
* ```ts
* import { McpServer, SessionCompat } from '@modelcontextprotocol/server';
* import { createMcpExpressApp, mcpExpressHandler } from '@modelcontextprotocol/express';
*
* const mcp = new McpServer({ name: 's', version: '1.0.0' });
* const app = createMcpExpressApp();
* app.all('/mcp', mcpExpressHandler(mcp, { session: new SessionCompat() }));
* ```

Check warning on line 24 in packages/middleware/express/src/express.ts

View check run for this annotation

Claude / Claude Code Review

JSDoc example not sourced from express.examples.ts (sync:snippets convention)

nit: the sibling `createMcpExpressApp` in this file sources its `@example` blocks from `express.examples.ts` (via `` ```ts source="./express.examples.ts#…" `` per CLAUDE.md § JSDoc `@example` Code Snippets), but this snippet is a freeform inline fence with no `source=`, so it is not type-checked by `pnpm sync:snippets`. Since `express.examples.ts` already exists for this module, consider adding a `#region mcpExpressHandler_basic` there and converting this to `@example` with `source="./express.ex
Comment on lines +17 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: the sibling createMcpExpressApp in this file sources its @example blocks from express.examples.ts (via ```ts source="./express.examples.ts#…" per CLAUDE.md § JSDoc @example Code Snippets), but this snippet is a freeform inline fence with no source=, so it is not type-checked by pnpm sync:snippets. Since express.examples.ts already exists for this module, consider adding a #region mcpExpressHandler_basic there and converting this to @example with source="./express.examples.ts#mcpExpressHandler_basic" — that also gives the README update a single type-checked source of truth. (Acknowledged that toNodeHttpHandler/handleHttp use plain fences too, so this is a within-file consistency suggestion only.)

Extended reasoning...

What this is

CLAUDE.md:44-48 documents the repo convention for JSDoc examples:

JSDoc @example tags should pull type-checked code from companion .examples.ts files (e.g., client.tsclient.examples.ts). Use ```ts source="./file.examples.ts#regionName" fences referencing //#region regionName blocks … Run pnpm sync:snippets to sync example content into JSDoc comments and markdown files.

The new mcpExpressHandler JSDoc (express.ts:17-24) embeds its usage example as a plain ```ts fence with no @example tag and no source= attribute. Meanwhile, the only other exported function in the same file, createMcpExpressApp (express.ts:80-94), follows the convention with three @example blocks all sourced from ./express.examples.ts#…, and packages/middleware/express/src/express.examples.ts exists and contains exactly those three #region blocks (createMcpExpressApp_default, _customHost, _allowedHosts) — but no region for mcpExpressHandler.

Why it matters (mildly)

The practical consequence is that the mcpExpressHandler snippet is not compiled by tsc and not refreshed by pnpm sync:snippets. If HandleHttpOptions, SessionCompat, or the import paths change, createMcpExpressApp's examples will fail the build while mcpExpressHandler's will silently drift. Given that another review comment on this PR already asks for the same snippet to be added to README.md, putting it in express.examples.ts once means JSDoc + README share a single type-checked source rather than two hand-maintained copies.

Addressing the counter-argument

It is true that the CLAUDE.md sentence is phrased around "@example tags", and that several other files in the repo — notably toNodeHttpHandler (packages/middleware/node/src/streamableHttp.ts:34) and handleHttp (packages/server/src/server/handleHttp.ts:24), the two functions this helper composes — use bare ```ts fences with no source=. So the convention is not universally enforced and this is not a guideline violation.

The reason it's still worth a one-line nit here, where it would not be on toNodeHttpHandler, is local context: this specific module already has a companion .examples.ts file set up and in use by its other export. Adding one more #region to an existing file is much lower friction than creating the companion from scratch, and leaving the two exports in the same file using two different example mechanisms is the kind of within-file inconsistency that tends to confuse the next contributor about which pattern to copy. That said, it's docs-only with zero runtime impact, the PR already carries several nits, and the author may reasonably defer this — hence nit, take or leave.

Step-by-step

  1. packages/middleware/express/src/express.ts:17-24 — new JSDoc uses ```ts with no @example and no source=.
  2. packages/middleware/express/src/express.ts:80-94 — sibling createMcpExpressApp uses three @example blocks with source="./express.examples.ts#createMcpExpressApp_…".
  3. packages/middleware/express/src/express.examples.ts — exists; contains #region blocks only for createMcpExpressApp_*; nothing for mcpExpressHandler.
  4. CLAUDE.md:44-48 — documents the source= / .examples.ts / pnpm sync:snippets convention.
  5. ⇒ the new snippet is the only example in this file that is not type-checked or synced.

Suggested fix

In express.examples.ts:

import { McpServer, SessionCompat } from '@modelcontextprotocol/server';
import { createMcpExpressApp, mcpExpressHandler } from './express.js';

function mcpExpressHandler_basic() {
    //#region mcpExpressHandler_basic
    const mcp = new McpServer({ name: 's', version: '1.0.0' });
    const app = createMcpExpressApp();
    app.all('/mcp', mcpExpressHandler(mcp, { session: new SessionCompat() }));
    //#endregion mcpExpressHandler_basic
    return app;
}

and in express.ts replace the bare fence with:

 * @example
 * ```ts source="./express.examples.ts#mcpExpressHandler_basic"
 * …
 * ```

then run pnpm sync:snippets.

*/
export function mcpExpressHandler(mcp: Dispatchable, options?: HandleHttpOptions): RequestHandler {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: mcpExpressHandler is a new public export (re-exported via packages/middleware/express/src/index.ts), so the PR description's "Public API unchanged" line is inaccurate and a changeset is needed — changeset-bot already flagged it. Please add a minor changeset for @modelcontextprotocol/express so the new helper shows up in the changelog.

Extended reasoning...

What's missing

This PR introduces mcpExpressHandler in packages/middleware/express/src/express.ts:26, and packages/middleware/express/src/index.ts:1 re-exports everything from that module via export * from './express.js'. That makes mcpExpressHandler a new public symbol on @modelcontextprotocol/express. However, the diff touches only four files (package.json, express.ts, express.test.ts, pnpm-lock.yaml) and does not include anything under .changeset/.

Why it matters

The PR description states "Public API unchanged" and ticks only "Documentation update" in the change-type checklist, but the diff (and the PR title itself — "mcpExpressHandler mount helper") clearly add a new exported function. Per CLAUDE.md ("Adding a symbol to a package index.ts makes it public API — do so intentionally") and REVIEW.md § Documentation & Changesets, prose that the diff doesn't back should be flagged. Without a changeset, merging will not produce a version bump or changelog entry, so consumers of @modelcontextprotocol/express won't see that mcpExpressHandler was added (and the new @hono/node-server runtime dependency also goes unannounced).

Step-by-step proof

  1. packages/middleware/express/src/express.ts:26export function mcpExpressHandler(...) is added in this diff.
  2. packages/middleware/express/src/index.ts:1export * from './express.js' re-exports it from the package entry point (exports['.']dist/index.mjs).
  3. git log 1916e69..28a2550 -- .changeset/ shows no commits in the stack touching .changeset/.
  4. changeset-bot's PR comment confirms: "⚠️ No Changeset found … Merging this PR will not cause a version bump for any packages."
  5. Therefore a new public API symbol ships with no version bump and no changelog entry.

How to fix

Add a changeset (the bot's link defaults to patch, but a new export warrants minor):

---
"@modelcontextprotocol/express": minor
---

Add `mcpExpressHandler(mcp, options?)` mount helper backed by `handleHttp`, allowing an `McpServer` to be mounted directly as an Express `RequestHandler` without a transport instance.

Also worth updating the PR description to drop "Public API unchanged" and tick "New feature".

Severity

This is release-hygiene/process, not a runtime bug, and changeset-bot has already surfaced the missing file with a one-click link — so nit. The add-on beyond the bot is (a) the bump should be minor rather than the bot's suggested patch, and (b) the PR description contradicts the diff.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: packages/middleware/express/README.md hasn't been updated for the new export — the ## Exports list (lines 23-31) omits mcpExpressHandler, and the ### Streamable HTTP endpoint (Express) example (lines 43-60) still tells users to install @modelcontextprotocol/node and wire NodeStreamableHTTPServerTransport per request, which is exactly the boilerplate this helper replaces. Per REVIEW.md § Tests & docs ("New feature: verify prose documentation is added (not just JSDoc)"), please add mcpExpressHandler to the Exports bullets and swap the Streamable HTTP snippet for the app.all('/mcp', mcpExpressHandler(mcp, ...)) form from the JSDoc.

Extended reasoning...

What's missing

mcpExpressHandler is a new public export of @modelcontextprotocol/express (added at packages/middleware/express/src/express.ts:26 and re-exported via src/index.ts's export * from './express.js'), but the package README — the primary discoverability surface for npm users — was not updated. Two sections are now stale:

  1. ## Exports (README.md:23-31) enumerates seven symbols (createMcpExpressApp, hostHeaderValidation, localhostHostValidation, requireBearerAuth, mcpAuthMetadataRouter, getOAuthProtectedResourceMetadataUrl, OAuthTokenVerifier) and does not list mcpExpressHandler.
  2. ### Streamable HTTP endpoint (Express) (README.md:43-60) still instructs users to npm install @modelcontextprotocol/node, import NodeStreamableHTTPServerTransport, and write a per-request new Transport → server.connect → transport.handleRequest callback — precisely the boilerplate mcpExpressHandler exists to eliminate.

Why it matters

REVIEW.md § Tests & docs explicitly says: "New feature: verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example." This PR adds good JSDoc with a usage snippet (express.ts:9-25), but no prose documentation. A user landing on the README — which is what renders on the npm package page — will see a complete-looking Exports list that omits the new helper, and a recommended wiring pattern that is now the legacy approach. The whole point of M2 is to make handleHttp-backed mounting the ergonomic default for Express; leaving the README pointing at the transport-class pattern undercuts that.

Why this isn't covered by the existing PR comments

The three prior review comments address (a) inlining vs. calling toNodeHttpHandler, (b) the missing hono optional-peer entry in package.json, and (c) the missing changeset / inaccurate "Public API unchanged" claim. None of them mention README.md, the stale Exports list, or the outdated usage example — this is a distinct documentation gap.

Step-by-step proof

  1. packages/middleware/express/src/express.ts:26export function mcpExpressHandler(...) is added in this diff.
  2. packages/middleware/express/src/index.ts:1export * from './express.js' re-exports it from the package entry point ⇒ public API.
  3. packages/middleware/express/README.md:23-31 — the Exports bullet list does not contain mcpExpressHandler.
  4. packages/middleware/express/README.md:45-59 — the Streamable HTTP example imports NodeStreamableHTTPServerTransport from @modelcontextprotocol/node and shows await server.connect(transport); await transport.handleRequest(req, res, req.body); inside the route handler.
  5. README.md:19-20 — the Install section still says "For MCP Streamable HTTP over Node.js … npm install @modelcontextprotocol/node", which the new helper makes unnecessary for Express users.
  6. PR diff touches four files (package.json, express.ts, express.test.ts, pnpm-lock.yaml); README.md is not among them.

Suggested fix

Add to the Exports list:

- `mcpExpressHandler(mcp, options?)`

and replace the Streamable HTTP example with the JSDoc form:

import { McpServer, SessionCompat } from '@modelcontextprotocol/server';
import { createMcpExpressApp, mcpExpressHandler } from '@modelcontextprotocol/express';

const mcp = new McpServer({ name: 'my-server', version: '1.0.0' });
const app = createMcpExpressApp();
app.all('/mcp', mcpExpressHandler(mcp, { session: new SessionCompat() }));

Optionally drop (or demote to a footnote) the npm install @modelcontextprotocol/node line at README.md:19-20, since the new helper removes that dependency for the common case.

Severity

Documentation gap only — no runtime impact — so nit.

const handler = handleHttp(mcp, options);
return (req, res, _next) => {
void _next;
const authInfo = (req as { auth?: AuthInfo }).auth;
const parsedBody: unknown = req.body;
const extra = authInfo !== undefined || parsedBody !== undefined ? { authInfo, parsedBody } : undefined;
// overrideGlobalObjects: false keeps the native Response prototype intact for frameworks
// that subclass it (e.g. Next.js).
const listener = getRequestListener(webReq => handler(webReq, extra), { overrideGlobalObjects: false });
void listener(req, res);
};
}
Comment on lines +26 to +38
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 nit: the PR description says this "switches internals to handleHttp(server) + toNodeHttpHandler", but the body of toNodeHttpHandler is inlined here rather than called (compare packages/middleware/node/src/streamableHttp.ts:41-51). If keeping the adapters dependency-free of one another is the intent, that's reasonable — just update the description; otherwise this could collapse to return toNodeHttpHandler(handleHttp(mcp, options)) and avoid a second copy of the Node↔Web bridge (the overrideGlobalObjects: false fix is already duplicated).

Extended reasoning...

What this is

The PR description states the express adapter "switches internals to handleHttp(server) + toNodeHttpHandler". The diff, however, never imports or calls toNodeHttpHandler from @modelcontextprotocol/node. Instead, mcpExpressHandler (express.ts:26-38) imports getRequestListener directly from @hono/node-server and inlines a body that is line-for-line equivalent to toNodeHttpHandler at packages/middleware/node/src/streamableHttp.ts:41-51:

step toNodeHttpHandler (node) mcpExpressHandler (express)
1 void _next void _next
2 read req.auth / req.body read (req as {auth?}).auth / req.body
3 build extra iff either defined build extra iff either defined
4 getRequestListener(webReq => handler(webReq, extra), { overrideGlobalObjects: false }) identical
5 invoke listener on (req, res) identical

So the description claims composition with toNodeHttpHandler, but the code ships a copy of it.

Why it's worth a comment (and why it's only a nit)

There are now two copies of the Node-IncomingMessage → Web-Request bridge that must evolve together. The overrideGlobalObjects: false workaround (the "keeps the native Response prototype intact for Next.js" comment) already had to be written in both places, which is exactly the maintenance burden duplication creates. If toNodeHttpHandler later grows error-forwarding to next, content-length handling, etc., this copy silently diverges.

That said, this is not a correctness bug — both implementations behave identically today, and the duplicated logic is ~6 lines. It's a design/description nit, not a blocker.

Addressing the counter-argument (cross-adapter dependency)

A reasonable objection is that the four middleware packages (express, fastify, hono, node) are deliberately independent siblings — none currently depend on each other — and delegating to toNodeHttpHandler would force @modelcontextprotocol/express to take a runtime dependency on @modelcontextprotocol/node, pulling a second adapter package into every Express user's tree just to deduplicate six lines. That's a legitimate architectural reason to inline.

I'm not arguing that's wrong. The point is narrower: the PR description doesn't match the diff. It says "+ toNodeHttpHandler", and a reader cross-referencing the review guide will look for that call and not find it. If the inlining is intentional to keep adapters independent, the fix is simply to drop "+ toNodeHttpHandler" from the description (and optionally leave a one-line "// inlined from toNodeHttpHandler to avoid depending on @mcp/node" so the next person touching either copy knows there's a sibling).

On scope: REVIEW.md § Documentation & Changesets is phrased around .changeset/*.md and inline comments rather than PR descriptions, so this isn't a guideline violation in the strict sense — it's just a description/diff mismatch worth tidying before merge.

Step-by-step

  1. PR description: "switches internals to handleHttp(server) + toNodeHttpHandler".
  2. grep -r toNodeHttpHandler packages/middleware/express/ → no matches.
  3. packages/middleware/express/package.json dependencies → @hono/node-server, cors; no @modelcontextprotocol/node.
  4. Diff express.ts:28-37 against node/src/streamableHttp.ts:44-50 → functionally identical.
  5. Therefore: description claims reuse, code inlines a copy. Either is fine; they should agree.

Suggested fix

Either:

  • (a) add @modelcontextprotocol/node as a dependency and reduce the body to return toNodeHttpHandler(handleHttp(mcp, options));, or
  • (b) drop "+ toNodeHttpHandler" from the PR description and (optionally) add a one-line comment noting the inline is deliberate to avoid a cross-adapter dependency.

Given the existing package structure, (b) is probably the lighter-weight choice.


/**
* Options for creating an MCP Express application.
*/
Expand Down
52 changes: 51 additions & 1 deletion packages/middleware/express/test/express.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,19 @@
import type { NextFunction, Request, Response } from 'express';
import supertest from 'supertest';
import { vi } from 'vitest';

import { createMcpExpressApp } from '../src/express.js';
import { McpServer, SessionCompat } from '@modelcontextprotocol/server';

import { createMcpExpressApp, mcpExpressHandler } from '../src/express.js';
import { hostHeaderValidation, localhostHostValidation } from '../src/middleware/hostHeaderValidation.js';

const INIT_MESSAGE = {
jsonrpc: '2.0',
method: 'initialize',
params: { clientInfo: { name: 'test-client', version: '1.0' }, protocolVersion: '2025-11-25', capabilities: {} },
id: 'init-1'
};

// Helper to create mock Express request/response/next
function createMockReqResNext(host?: string) {
const req = {
Expand Down Expand Up @@ -189,4 +199,44 @@ describe('@modelcontextprotocol/express', () => {
expect(app).toBeDefined();
});
});

describe('mcpExpressHandler', () => {
function makeApp(options?: Parameters<typeof mcpExpressHandler>[1]) {
const mcp = new McpServer({ name: 'test-server', version: '1.0.0' });
const app = createMcpExpressApp({ host: '0.0.0.0', allowedHosts: ['127.0.0.1'] });
app.all('/mcp', mcpExpressHandler(mcp, { enableJsonResponse: true, ...options }));
return app;
}

test('serves initialize via mcp.dispatch() (stateless, no transport class)', async () => {
const res = await supertest(makeApp())
.post('/mcp')
.set('Accept', 'application/json, text/event-stream')
.set('Host', '127.0.0.1')
.send(INIT_MESSAGE);
expect(res.status).toBe(200);
expect(res.body.result).toMatchObject({ serverInfo: { name: 'test-server' } });
expect(res.headers['mcp-session-id']).toBeUndefined();
});

test('serves session lifecycle via SessionCompat', async () => {
const app = makeApp({ session: new SessionCompat() });
const initRes = await supertest(app)
.post('/mcp')
.set('Accept', 'application/json, text/event-stream')
.set('Host', '127.0.0.1')
.send(INIT_MESSAGE);
const sid = initRes.headers['mcp-session-id'] as string;
expect(sid).toBeTruthy();
const pingRes = await supertest(app)
.post('/mcp')
.set('Accept', 'application/json, text/event-stream')
.set('Host', '127.0.0.1')
.set('mcp-session-id', sid)
.set('mcp-protocol-version', '2025-11-25')
.send({ jsonrpc: '2.0', method: 'ping', params: {}, id: 'p-1' });
expect(pingRes.status).toBe(200);
expect(pingRes.body).toMatchObject({ jsonrpc: '2.0', id: 'p-1', result: {} });
});
});
});
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading