feat(mcp): load MCP servers from JSON#2947
Conversation
github-actions
Bot
added
size/l
area-mcp
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| it('creates McpClient with url when url is present', async () => { | ||
| const clients = await McpClient.loadServers({ | ||
| 'remote-server': { url: 'https://example.com/mcp' }, | ||
| }) |
There was a problem hiding this comment.
Issue: This test asserts only toHaveLength(1) and that other transports weren't called, but never verifies a StreamableHTTPClientTransport was actually constructed with the expected URL — so a regression in URL→streamable-http detection wouldn't be caught here.
Suggestion: Assert the positive case, e.g. expect(StreamableHTTPClientTransport).toHaveBeenCalledWith(new URL('https://example.com/mcp'), ...), mirroring how the SSE tests verify the constructed transport.
| } | ||
|
|
||
| function baseOptions(name: string, server: McpServerConfig, defaults?: McpClientOptions): McpClientOptions { | ||
| const opts: McpClientOptions = { ...defaults, applicationName: defaults?.applicationName ?? name } |
There was a problem hiding this comment.
Issue: When defaults.applicationName is set, every loaded client gets that same name (defaults?.applicationName ?? name), so the per-server key is discarded and all clients share one identity. The test uses defaults applicationName over server name codifies this.
Suggestion: Confirm this is intended. The more useful default for a multi-server load is usually the distinct server key per client; a shared applicationName from defaults collapses that. If intentional, a one-line doc note on loadServers's defaults param explaining the precedence would prevent surprise.
|
|
||
| for (const [name, server] of Object.entries(servers)) { | ||
| if (!server || typeof server !== 'object' || Array.isArray(server)) { | ||
| throw new Error(`Server "${name}" must be an object, got ${Array.isArray(server) ? 'array' : typeof server}`) |
There was a problem hiding this comment.
Issue: This object-shape validation runs before continueOnError is resolved (line 36) and outside the try/catch. A single malformed entry (e.g. a server value that is a string or array) throws and aborts the entire load — even when the caller passed defaults.continueOnError: true or set it on other servers.
Suggestion: Move this check inside the try block (resolving continueOnError first from defaults for the non-object case), so a bad entry is skipped consistently with every other config error. Worth a test covering "malformed entry skipped under continueOnError".
|
Assessment: Comment Solid, focused change: the Review themes
Nice work — the env interpolation, exhaustive transport switch, and error messages are clear and well tested. |
2 participants
Description
Related Issues
Documentation PR
Type of Change
Bug fix
New feature
Breaking change
Documentation update
Other (please describe):
Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
hatch run prepareChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.