feat: add multi-server routing example#1691
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward new example following the existing client-quickstart pattern with only minor nits.
Extended reasoning...
Overview
This PR adds a new examples/client-multi-server/ directory containing a ~170-line CLI chatbot that connects to multiple MCP servers, aggregates tools, and routes tool calls. It also adds the expected config entries (.changeset/config.json ignore list, .prettierignore). No library code is modified — only new example files and the lockfile.
Security risks
None. This is a standalone CLI example that reads a local JSON config and calls the Anthropic API. No auth, crypto, or permissions code is involved. The env passthrough in transport config mirrors the existing quickstart pattern.
Level of scrutiny
Low scrutiny is appropriate. This is self-contained example code that closely mirrors the existing client-quickstart example in structure, dependencies, and patterns. It does not affect any published packages (added to changeset ignore list). The examples/ directory is not covered by CODEOWNERS.
Other factors
All three bug reports are nits on example code with no runtime impact: (1) a type guard narrowing to the wrong SDK type (works due to structural compatibility), (2) no warning on tool name collisions across servers, and (3) a theoretical client leak if listTools fails after connect. These are worth noting as inline comments for the author to consider but do not block merging. The PR fixes an open issue (#740) and the author confirms all tests, typecheck, and lint pass.
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Thanks for this — the mcpServers config parsing and agentic loop are solid. A few changes before merge:
1. Prefix tool names with server name
The flat Map<toolName, serverName> means if two servers both expose a tool with the same name, the last one silently overwrites the first. Prefix tool names to avoid this:
const prefixedName = `${serverName}__${tool.name}`;
this.toolToServer.set(prefixedName, { serverName, originalName: tool.name });
this.tools.push({ name: prefixedName, ... });Then strip the prefix before calling the server.
2. Default servers.json should have 2+ servers
A multi-server example with one server in the default config doesn't demonstrate the routing. Add @modelcontextprotocol/server-time as a second entry — it works via npx with no extra setup.
3. Fix the content type predicate
filter((c): c is Anthropic.TextBlock => c.type === 'text') — MCP CallToolResult content blocks aren't Anthropic.TextBlock. Use TextContent from @modelcontextprotocol/core.
|
@claude review |
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Thanks for the iterations - apologies, my previous review was posted prematurely.
It's probably valid to call out some multi-server example. but this mostly duplicates the quickstart and pins another anthropic SDK version. The novel part is the tool-name prefixing and server routing.
I think this could be slimmed down to a minimal routing example, something that spawns 2 in-repo servers (e.g. server-quickstart + mcpServerOutputSchema) and just shows the routing between them rather than a full chatbot client implementation. That would also drop the Anthropic API key dep so it runs without it.
|
@felixweinberger All feedback from both your reviews and claude[bot]'s review has been addressed in the latest commit (a1bde15):
CI is green across all checks. Ready for re-review when you have a moment. |
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Thanks for the updates - mind updating PR title & body to reflect that this isn't a chatbot anymore? Then we can land this.
|
@claude review |
| for (const client of clients) { | ||
| await client.close(); | ||
| } | ||
| console.log('All servers disconnected.'); | ||
| } | ||
|
|
||
| main().catch((err) => { | ||
| console.error('Fatal error:', err); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
🟡 The cleanup loop at lines 103–105 is never reached on error: the .catch() handler at lines 109–112 calls process.exit(1) synchronously, terminating the process before any already-connected clients are gracefully closed. While OS cleanup makes the practical impact zero for this script, the example explicitly builds a clients array for tracked cleanup but bypasses it entirely on the error path — a misleading pattern for developers who copy this into production code where process.exit() is not the safety net.
Extended reasoning...
What the bug is
main() constructs const clients: Client[] = [] (line 36) and pushes each successfully-connected client into it (line 46). At the bottom of main(), lines 103–105 iterate this array and call await client.close() on each entry — the intended cleanup path. However, main() has no try/finally wrapping its body. If anything throws after a client is pushed (e.g., listTools() fails on the second server, or a routing demo callTool() throws), the error propagates out of main() and into the .catch() handler at lines 109–112, which calls process.exit(1). In Node.js, process.exit() terminates the event loop immediately and synchronously — the cleanup loop on lines 103–105 never executes.
The specific code path that triggers it
client.connect(transport)succeeds forweather-nws— that client is pushed toclients[].client.connect(transport)succeeds forweather-structured— pushed too.alertsRoute.client.callTool(...)throws (e.g., the child process exits unexpectedly).- The exception propagates out of
main(). .catch((err) => { console.error(...); process.exit(1); })runs — the process terminates immediately.- Lines 103–105 (
for (const client of clients) { await client.close(); }) are never reached.
Why existing code does not prevent it
There is no try/finally inside main(). The cleanup code is placed sequentially at the end of the function body, which only executes on the success path. The .catch() at the call site calls process.exit(1) rather than re-throwing, which prevents any finally block that could be added inside main() from running as well.
Addressing the refutation
The refutation is correct that the runtime impact is zero: process.exit(1) terminates the Node.js process, the OS reclaims all file descriptors, and the spawned child processes exit when their stdin pipes close. This is why severity is nit, not normal. The concern is pedagogical: this example explicitly introduces the clients tracking array as the mechanism for cleanup, which is the exact pattern developers will lift into production services (long-running HTTP servers, daemons) where process.exit() is not present. Copying this pattern without the try/finally creates a real resource leak in those contexts.
How to fix it
Wrap the body of main() in a try/finally:
async function main() {
const clients: Client[] = [];
try {
// ... connect, discover, demo ...
} finally {
for (const client of clients) {
await client.close();
}
console.log('All servers disconnected.');
}
}
main().catch((err) => {
console.error('Fatal error:', err);
process.exitCode = 1;
});This ensures the cleanup loop runs on both the success path and the error path, and it makes the example demonstrate correct resource management that scales to production.
| // Call get-alerts from weather-nws (server-quickstart) | ||
| const alertsKey = 'weather-nws__get-alerts'; | ||
| const alertsRoute = toolRouter.get(alertsKey); | ||
| if (alertsRoute) { | ||
| console.log(`Calling ${alertsKey} ...`); | ||
| const result = await alertsRoute.client.callTool({ | ||
| name: alertsRoute.originalName, | ||
| arguments: { state: 'CA' }, | ||
| }); | ||
| const text = result.content | ||
| .filter((c) => c.type === 'text') | ||
| .map((c) => c.text) | ||
| .join('\n'); | ||
| console.log(`Result: ${text.slice(0, 200)}...\n`); | ||
| } | ||
|
|
||
| // Call get_weather from weather-structured (mcpServerOutputSchema) | ||
| const weatherKey = 'weather-structured__get_weather'; | ||
| const weatherRoute = toolRouter.get(weatherKey); | ||
| if (weatherRoute) { | ||
| console.log(`Calling ${weatherKey} ...`); | ||
| const result = await weatherRoute.client.callTool({ | ||
| name: weatherRoute.originalName, | ||
| arguments: { city: 'Seattle', country: 'US' }, | ||
| }); | ||
| const text = result.content | ||
| .filter((c) => c.type === 'text') | ||
| .map((c) => c.text) | ||
| .join('\n'); | ||
| console.log(`Result: ${text.slice(0, 200)}...\n`); |
There was a problem hiding this comment.
🟡 The routing demo guards both tool lookups with silent if-checks, so if either hardcoded key (weather-nws__get-alerts or weather-structured__get_weather) is absent from the routing table, the demo prints only "--- Routing demo ---" then "All servers disconnected." with zero indication that no routing was demonstrated. Since the explicit purpose of this code is to prove routing works, a silent no-op is pedagogically misleading. Replace the if-guards with assertive lookups that throw or console.warn when a key is missing.
Extended reasoning...
What the bug is and how it manifests
The routing demo at lines 70-99 uses two hardcoded prefixed keys ('weather-nws__get-alerts' and 'weather-structured__get_weather') to look up entries in toolRouter. Both lookups are wrapped in if (alertsRoute) / if (weatherRoute) guards. If either key is missing, the corresponding console.log and callTool block is silently skipped. The output in that case would be:
--- Routing demo ---
All servers disconnected.
with nothing in between — giving the user no signal that zero routing was demonstrated.
The specific code path that triggers it
Any of the following would cause a silent skip:
- A server entry in the
serversarray is renamed (e.g.,'weather-nws'→'nws'), changing the prefix so the hardcoded key no longer matches. - Either server renames or removes the target tool (e.g.,
get-alerts→get_alerts). - A future refactor adds a server name separator change (e.g.,
__→--).
In any of these cases, toolRouter.get(alertsKey) returns undefined, the if is false, and the block is skipped.
Why existing code doesn't prevent it
The if-guard pattern is the standard way to handle optional values in TypeScript, but it is the wrong choice here. The lookups are not optional: this is demo code whose sole purpose is to call a tool on each server. There is no fallback behavior — nothing useful happens in the else-branch. The guard silently converts a programming error (wrong key) into a no-op.
What the impact would be
A developer adapting this example who mistypes a server name, or a future contributor who renames a server without updating the demo, would see the example "run successfully" (no error, exit code 0) while demonstrating nothing. This defeats the pedagogical value of the demo and makes it harder to catch regressions when server tool names change.
How to fix it
Replace each silent if-guard with an assertive lookup:
const alertsRoute = toolRouter.get(alertsKey);
if (\!alertsRoute) throw new Error(`Route not found for ${alertsKey}. Is the server running and the tool name correct?`);Or more concisely with a helper:
function requireRoute(key: string) {
const route = toolRouter.get(key);
if (\!route) throw new Error(`No route for ${key}`);
return route;
}Step-by-step proof
- Developer renames the
serversentry from'weather-nws'to'nws'to match their mental model. - All tools are now registered under
nws__get-alerts, etc. toolRouter.get('weather-nws__get-alerts')returnsundefined.if (alertsRoute)is false — the call block is skipped.- Same for the second lookup.
- Output is exactly
--- Routing demo ---\n\nAll servers disconnected.with no errors, no warnings. - Developer concludes the example works, ships code with broken routing.
travisbreaks
changed the title
Adds a new standalone example that connects to multiple MCP servers simultaneously, aggregates their tools, and routes tool calls to the correct server. This is the TypeScript equivalent of the Python SDK's simple-chatbot example, addressing issue modelcontextprotocol#740. The example follows the client-quickstart pattern: minimal dependencies (Anthropic SDK + MCP client), standalone package, simple CLI interface. It uses a JSON config file (same format as Claude Desktop) to define servers and includes an agentic loop for multi-step tool use. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Prefix tool names with serverName__ to prevent silent overwrites when multiple servers expose identically-named tools. Original names are preserved and used for the actual callTool requests. - Fix incorrect type guard that narrowed MCP ContentBlock to Anthropic.TextBlock (wrong type). Now uses plain .filter(). - Move this.servers.set() before listTools() so cleanup can still close the client if tool discovery throws. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace the full chatbot implementation with a minimal script that: - Spawns two in-repo servers (server-quickstart + mcpServerOutputSchema) - Connects a Client to each via StdioClientTransport - Discovers tools and builds a prefixed routing table - Calls one tool on each server to demonstrate routing Removes the Anthropic SDK dependency and servers.json config file. No API key required to run. Co-Authored-By: Tadao <tadao@travisfixes.com>
…te lookups, exitCode pattern Address two Claude bot review concerns posted after the prior approval (2026-04-09 17:21:58): 1. Cleanup pattern: main() body had no try/finally, so the cleanup loop never ran when an error propagated to the .catch() handler that called process.exit(1) synchronously. Wrapped main() body in try/finally; moved cleanup loop into finally; replaced process.exit(1) with process.exitCode = 1 so the finally block actually runs on error. 2. Silent routing failures: hardcoded prefixed keys with if-guards meant a missing key (renamed server, renamed tool) printed only the routing demo header then "All servers disconnected." with zero indication that no routing was demonstrated. Replaced silent if-guards with assertive lookups that throw a clear error identifying the missing key. Renamed result/text variables to alertsResult/alertsText/weatherResult/ weatherText since de-indenting the call blocks would put two const result declarations in the same scope. No semantic change to the demo output. Local typecheck:all, lint:all, and build:all all pass. Co-Authored-By: Tadao <tadao@travisfixes.com>
…ubpath split Upstream main split StdioClientTransport into the './stdio' subpath of @modelcontextprotocol/client to keep the root entry free of process-spawning runtime dependencies (child_process, cross-spawn). Two changes were needed to align this example with the new layout: 1. src/index.ts: import StdioClientTransport from '@modelcontextprotocol/client/stdio' instead of from the root entry. 2. tsconfig.json: add path mappings for the new client/stdio subpath and the core/public subpath, matching the client-quickstart example. Without these mappings, the example's typecheck resolved client/stdio through the package's exports map to dist, which produced a race condition with parallel build steps that clear dist before rebuilding. Local typecheck:all, lint:all, and build:all all pass. Co-Authored-By: Tadao <tadao@travisfixes.com>
travisbreaks
force-pushed
the
feat/multi-server-chatbot
branch
from
0c7cd19 to
ce2d3e4
Compare
|
@felixweinberger heads up: branch rebased on current main.
|
2 participants
Summary
Adds a
client-multi-serverexample that spawns two in-repo MCP servers, discovers tools from both, and demonstrates tool-name-prefixed routing between them. No external API key required.Fixes #740
What it does
server-quickstartandmcpServerOutputSchemaas child processesserverName__to prevent collisions)Implementation
client-quickstartexample pattern (package structure, tsconfig, dependencies)servers.jsonfor easy testingTest plan