-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: add injectable SDK logger #2370
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: main
Are you sure you want to change the base?
Changes from all commits
dde8b21
c4ef4ed
4b96a94
66c1fa5
013345e
6c05a11
7944587
453b520
1b92074
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 |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@modelcontextprotocol/core-internal': minor | ||
| '@modelcontextprotocol/client': minor | ||
| '@modelcontextprotocol/server': minor | ||
| --- | ||
|
|
||
| Add a `logger` protocol option so client and server SDK diagnostics can be routed through user-provided logging. | ||
|
mattzcarey marked this conversation as resolved.
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,25 @@ Only the result comes back: | |
|
|
||
| `progress` must increase on every notification for the same token; `total` and `message` are optional. | ||
|
|
||
| ## Route SDK diagnostics | ||
|
|
||
| Pass `logger` to route local warnings and debug messages from SDK internals. On stdio, send every level to stderr so diagnostics never enter the JSON-RPC channel. | ||
|
Comment on lines
+74
to
+76
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. 🟡 The docs section "Route SDK diagnostics" and the changeset make an unqualified claim that SDK warnings can be routed through the new Extended reasoning...What the gap is This PR introduces the injectable
Why they cannot be routed today The OAuth helpers are standalone functions that take an Why this is worth noting on this PR The tension is not with the auth code (which is pre-existing and untouched by this diff) but with the prose this diff adds:
A repo-wide grep of Concrete walk-through
Impact and how to fix Impact is bounded: a couple of warn lines during OAuth flows, only for legacy or incomplete providers; nothing breaks functionally. Two acceptable resolutions: (a) soften the changeset/docs wording to note that OAuth-flow diagnostics are not yet routable (e.g. "most local warnings and debug messages"), or (b) thread an optional |
||
|
|
||
| ```ts source="../../examples/guides/servers/logging-progress-cancellation.examples.ts#sdkLogger_stderr" | ||
| const sdkLogger = { | ||
| debug: (...args: unknown[]) => console.error('[debug]', ...args), | ||
| info: (...args: unknown[]) => console.error('[info]', ...args), | ||
| warn: (...args: unknown[]) => console.error('[warn]', ...args), | ||
| error: (...args: unknown[]) => console.error('[error]', ...args) | ||
| }; | ||
|
|
||
| const diagnosticServer = new McpServer({ name: 'file-processor', version: '1.0.0' }, { logger: sdkLogger }); | ||
| ``` | ||
|
|
||
| The same option works on `Client` and low-level `Server`; `createMcpHandler` also accepts it for handler diagnostics. Every method is optional, and an omitted method discards diagnostics at that level. The default is `console`. | ||
|
|
||
| This logger stays local to the process. It does not send MCP `notifications/message`; use `ctx.mcpReq.log` for that deprecated protocol feature. | ||
|
|
||
| ## Log to the client | ||
|
|
||
| ::: warning Deprecated — SEP-2577 | ||
|
|
@@ -201,5 +220,6 @@ Resolve an identifier against a fixed list, as `fetch-source` does. A tool that | |
|
|
||
| - Every handler receives a context as its second argument; the request-scoped helpers live on `ctx.mcpReq`. | ||
| - `ctx.mcpReq.notify` sends `notifications/progress` when the request carried a `progressToken`; `progress` must increase on each one. | ||
| - The `logger` option routes local SDK diagnostics; it is separate from MCP protocol logging. | ||
| - `ctx.mcpReq.log(level, data)` sends `notifications/message` once the `logging` capability is declared; MCP logging is deprecated (SEP-2577). | ||
| - `ctx.mcpReq.signal` aborts on cancellation and disconnect — check it in long loops and forward it to your own I/O. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import type { JSONRPCRequest, Tool } from '@modelcontextprotocol/core-internal'; | ||
| import { InMemoryTransport, LATEST_PROTOCOL_VERSION } from '@modelcontextprotocol/core-internal'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { Client } from '../../src/client/client'; | ||
|
|
||
| const MODERN = '2026-07-28'; | ||
|
|
||
| describe('Client logger option', () => { | ||
| it('routes SDK diagnostics to the configured logger', async () => { | ||
| const debug = vi.fn(); | ||
| const consoleDebug = vi.spyOn(console, 'debug').mockImplementation(() => {}); | ||
| const client = new Client({ name: 'test-client', version: '1.0.0' }, { logger: { debug } }); | ||
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | ||
|
|
||
| serverTransport.onmessage = async message => { | ||
| if ('method' in message && 'id' in message && message.method === 'initialize') { | ||
| await serverTransport.send({ | ||
| jsonrpc: '2.0', | ||
| id: message.id, | ||
| result: { | ||
| protocolVersion: LATEST_PROTOCOL_VERSION, | ||
| capabilities: {}, | ||
| serverInfo: { name: 'test-server', version: '1.0.0' } | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| await Promise.all([client.connect(clientTransport), serverTransport.start()]); | ||
|
|
||
| await expect(client.listTools()).resolves.toEqual({ tools: [] }); | ||
| expect(debug).toHaveBeenCalledWith( | ||
| 'Client.listTools() called but server does not advertise tools capability - returning empty list' | ||
| ); | ||
| expect(consoleDebug).not.toHaveBeenCalled(); | ||
|
|
||
| consoleDebug.mockRestore(); | ||
| await client.close(); | ||
| await clientTransport.close(); | ||
| await serverTransport.close(); | ||
| }); | ||
|
|
||
| it('routes invalid x-mcp-header tool exclusion warnings to the configured logger', async () => { | ||
| const warn = vi.fn(); | ||
| const consoleWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}); | ||
| const client = new Client( | ||
| { name: 'test-client', version: '1.0.0' }, | ||
| { logger: { warn }, versionNegotiation: { mode: { pin: MODERN } } } | ||
| ); | ||
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | ||
| const invalidTool: Tool = { | ||
| name: 'bad-tool', | ||
| inputSchema: { type: 'object', properties: { data: { type: 'object', 'x-mcp-header': 'Data' } } } | ||
| }; | ||
|
|
||
| serverTransport.onmessage = async message => { | ||
| const request = message as JSONRPCRequest; | ||
| if (request.id === undefined) return; | ||
|
|
||
| if (request.method === 'server/discover') { | ||
| await serverTransport.send({ | ||
| jsonrpc: '2.0', | ||
| id: request.id, | ||
| result: { | ||
| resultType: 'complete', | ||
| supportedVersions: [MODERN], | ||
| capabilities: { tools: {} }, | ||
| serverInfo: { name: 'test-server', version: '1.0.0' } | ||
| } | ||
| }); | ||
| } else if (request.method === 'tools/list') { | ||
| await serverTransport.send({ | ||
| jsonrpc: '2.0', | ||
| id: request.id, | ||
| result: { | ||
| resultType: 'complete', | ||
| ttlMs: 60_000, | ||
| cacheScope: 'public', | ||
| tools: [invalidTool] | ||
| } | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| await Promise.all([client.connect(clientTransport), serverTransport.start()]); | ||
|
|
||
| const result = await client.listTools(); | ||
| expect(result.tools).toEqual([]); | ||
| expect(warn).toHaveBeenCalledWith(expect.stringContaining("excluding tool 'bad-tool'")); | ||
| expect(consoleWarn).not.toHaveBeenCalled(); | ||
|
|
||
| consoleWarn.mockRestore(); | ||
| await client.close(); | ||
| await clientTransport.close(); | ||
| await serverTransport.close(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| /** | ||
| * Console-compatible sink for local SDK diagnostics. | ||
| * | ||
| * This is separate from MCP protocol logging (`notifications/message`). Every method is | ||
| * optional; diagnostics at an omitted level are discarded. Pass an adapter around a | ||
| * structured logger when its methods do not already accept console-style arguments. | ||
| */ | ||
| export type SdkLogger = { | ||
| debug?: (...args: unknown[]) => void; | ||
| info?: (...args: unknown[]) => void; | ||
| warn?: (...args: unknown[]) => void; | ||
| error?: (...args: unknown[]) => void; | ||
| }; |
Uh oh!
There was an error while loading. Please reload this page.