Skip to content

feat: add injectable SDK logger#2370

Open
mattzcarey wants to merge 8 commits into
mainfrom
feat/injectable-logger
Open

feat: add injectable SDK logger#2370
mattzcarey wants to merge 8 commits into
mainfrom
feat/injectable-logger

Conversation

@mattzcarey

@mattzcarey mattzcarey commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a logger protocol option shared by Client, Server, and McpServer
  • expose the same diagnostics sink on createMcpHandler
  • route client capability diagnostics, tool-name warnings, x-mcp-header warnings, and Standard Schema fallback warnings through the configured logger
  • export the SdkLogger type and add focused coverage and documentation

Fixes #1262

API decision

SdkLogger is intentionally a minimal, console-compatible diagnostics sink with optional debug, info, warn, and error methods. It does not adopt RFC 5424 levels or filtering: these are local SDK diagnostics, not MCP protocol logging, and level policy belongs to the application's logger. Console, Pino, and Winston can be passed directly or through a small adapter.

Omitting a method discards diagnostics at that level. Omitting logger preserves the current console behavior. For stdio servers, the documentation shows routing every level to stderr so diagnostics cannot enter the JSON-RPC channel.

Tests

  • pnpm sync:snippets --check
  • pnpm --filter @modelcontextprotocol/examples typecheck
  • pnpm --filter @modelcontextprotocol/client test -- test/client/logger.test.ts
  • pnpm --filter @modelcontextprotocol/core-internal test -- test/util/standardSchema.zodFallback.test.ts
  • pnpm --filter @modelcontextprotocol/server test -- test/server/createMcpHandler.test.ts test/server/mcp.compat.test.ts
  • pnpm --filter @modelcontextprotocol/core-internal --filter @modelcontextprotocol/client --filter @modelcontextprotocol/server typecheck
  • pnpm --filter @modelcontextprotocol/client --filter @modelcontextprotocol/server build
  • node scripts/smoke-dist-types.mjs
  • pre-push typecheck:all, build:all, and lint:all

@mattzcarey mattzcarey requested a review from a team as a code owner June 25, 2026 13:02
@changeset-bot

changeset-bot Bot commented Jun 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 453b520

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@modelcontextprotocol/core-internal Minor
@modelcontextprotocol/client Minor
@modelcontextprotocol/server Minor
@modelcontextprotocol/express Major
@modelcontextprotocol/fastify Major
@modelcontextprotocol/hono Major
@modelcontextprotocol/node Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2370

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2370

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2370

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2370

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2370

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2370

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2370

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2370

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2370

commit: 453b520

Comment thread packages/core-internal/src/util/standardSchema.ts
Comment thread packages/core-internal/src/shared/protocol.ts
@mattzcarey mattzcarey force-pushed the feat/injectable-logger branch from 63bb2b4 to f2d0beb Compare June 25, 2026 13:38

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Both points from my earlier review are now addressed in this revision — the prompt argsSchema conversion path forwards the configured logger, and prose docs plus a sourced example were added for the new option. I didn't find any new issues, but since this introduces a new public API surface (the logger ProtocolOptions field and the exported SdkLogger type), a maintainer should sign off on the API shape.

Extended reasoning...

Overview

This PR adds an injectable SdkLogger to ProtocolOptions, threads it through Protocol, Client, and McpServer, and routes existing console.* SDK diagnostics (capability-gating debug messages, tool-name validation warnings, zod-fallback warnings) through it. It exports the SdkLogger type from the core public exports, adds a changeset, prose documentation in docs/client.md and docs/server.md, a sourced example in examples/client/src/clientGuide.examples.ts, and focused tests for the client, tool-name, prompt-conversion, and zod-fallback paths. The latest commit also addresses both items raised in my prior review: promptArgumentsFromStandardSchema now accepts and forwards a logger (with the prompts/list call site passing this._logger), and prose documentation for the option was added.

Security risks

None identified. The logger is a user-supplied callback object invoked only with diagnostic strings via optional chaining; it does not handle credentials, network input, or auth flows. The default remains console, preserving existing behavior for callers that don't opt in.

Level of scrutiny

Moderate. The mechanical changes (replacing console.warn/console.debug with this._logger.*?.() and threading a defaulted parameter) are low risk and well covered by the new tests, which also assert that console is no longer hit when a custom logger is configured. However, the PR introduces a new public API surface — a logger field on ProtocolOptions and a newly exported SdkLogger type — and this repository's conventions place the burden of proof on additions to the public API. Whether the shape (partial logger with all-optional methods, no level filtering, named SdkLogger, attached to ProtocolOptions rather than a narrower options type) is the right long-term design is a maintainer judgment call, so I'm not shadow-approving.

Other factors

The change has a concrete motivating issue (#1262), the changeset correctly marks core/client/server as minor, and existing behavior is unchanged when no logger is provided. The test coverage is reasonable: client capability-gating debug routing, tool-name warning routing, prompt argsSchema conversion routing, and the zod-fallback path all assert both the custom logger call and the absence of console output. No unresolved reviewer comments remain on the thread.

@KKonstantinov

Copy link
Copy Markdown
Contributor

Hey @mattzcarey , we explored this in #1034

You could get some stuff from there, there are some caveats with Stdio, plus following SysLog - RFC5424

@mattzcarey mattzcarey force-pushed the feat/injectable-logger branch from f2d0beb to 103073d Compare June 25, 2026 21:52
Comment thread .changeset/injectable-sdk-logger.md

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The latest revision addresses my earlier findings (changeset now targets core-internal, prose docs/example added, and the prompts/list path now threads the configured logger with test coverage), and I didn't find new bugs — but this adds a new public API surface (ProtocolOptions.logger and the exported SdkLogger type) and there's an open maintainer comment pointing to prior design discussion in #1034 (stdio caveats, RFC5424 levels), so a human should weigh in on the API shape before merging.

Extended reasoning...

Overview

This PR adds an injectable SDK diagnostics logger: a new logger field on ProtocolOptions (consumed by Protocol, Client, and McpServer), a new SdkLogger type exported from the public surface of core-internal, and routing of existing console.* diagnostics (client capability-gating debug messages, tool-name validation warnings, zod JSON-schema fallback warnings) through that logger. It also adds prose docs in docs/client.md and docs/server.md, a sourced example, focused tests in all three packages, and a changeset bumping core-internal/client/server as minor.

Status of prior feedback

All three issues raised on earlier revisions are resolved in the current diff: the changeset frontmatter now lists @modelcontextprotocol/core-internal instead of the schemas-only core package; promptArgumentsFromStandardSchema now accepts and forwards a logger and McpServer passes this._logger at the prompts/list call site (with a regression test); and prose documentation plus an example were added. A grep of the modified packages shows no remaining un-routed console.* diagnostics in scope (the fetch-logging middleware has its own configurable RequestLogger and is unrelated).

Security risks

None identified. The logger is a passive sink for diagnostic strings; no auth, crypto, or transport behavior changes. The only minor consideration is that user-supplied logger callbacks now run inside SDK code paths, but they are invoked with optional chaining and a throwing logger would surface like any handler error.

Level of scrutiny

The implementation is small and well-tested, but this is a deliberate public API addition on a repo whose review conventions put the burden of proof on additions and ask for design-level sign-off first. There is also an unanswered maintainer comment referencing the earlier exploration in PR #1034 (stdio caveats, RFC5424/syslog level alignment), which bears directly on whether the minimal four-method SdkLogger shape is the right long-term surface. That design call belongs to a human maintainer, so I am deferring rather than approving despite finding no correctness issues.

Other factors

Test coverage is good (client logger routing, tool-name warning routing, prompt argsSchema fallback routing, direct standardSchema fallback test), and the docs additions correctly distinguish SDK-internal diagnostics from MCP protocol logging. The codemod versions.ts regeneration matches the corrected changeset targets.

@mattzcarey mattzcarey force-pushed the feat/injectable-logger branch from b5094b5 to caaff71 Compare July 1, 2026 09:18
Comment thread packages/client/src/client/client.ts
Comment thread .changeset/injectable-sdk-logger.md

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The issues flagged in earlier review rounds (changeset targeting the wrong package, prompt-path and client-side console call sites, createMcpHandler logger option) all appear to be addressed in the current revision, with test coverage added for each. Since this introduces new public API surface (SdkLogger and the logger option across ProtocolOptions and CreateMcpHandlerOptions), it still warrants a maintainer's sign-off on the API design; note prose documentation for the option under docs/ hasn't been added yet.

Extended reasoning...

Overview

This PR adds an injectable SDK logger: a new SdkLogger type in core-internal/src/shared/logger.ts, a logger field on ProtocolOptions (consumed by Protocol/Client/Server/McpServer) and on CreateMcpHandlerOptions, and it routes existing console.debug/console.warn SDK diagnostics (client capability messages, x-mcp-header exclusion warnings, tool-name validation warnings, zod-fallback warnings, the responseMode:'json' warning) through the configured logger with console as the default. Tests are added in the client, core-internal, and server packages covering each migrated call site.

Security risks

None of consequence — the change only affects where diagnostic strings are emitted. The logger is user-supplied and called with SDK-generated messages; there is no injection surface, no auth or transport behavior change, and defaults preserve existing console output.

Level of scrutiny

The implementation is mechanical and well-tested, but it adds new public API surface (the exported SdkLogger type and the logger option on two options interfaces across three published packages). Per this repository's conventions, new public API additions carry a burden of proof and should get maintainer review of the design (naming, whether all-optional-methods partial-logger semantics are the right shape, whether the option belongs on CreateMcpHandlerOptions too), so this is not a candidate for auto-approval despite the code being straightforward.

Other factors

All issues flagged in previous review rounds have been addressed in the current diff: the changeset now targets @modelcontextprotocol/core-internal, promptArgumentsFromStandardSchema accepts and forwards the logger, the client-side x-mcp-header exclusion warning uses this._logger, and createMcpHandler accepts a logger option — each with a corresponding test. The remaining gap is prose documentation: no docs/ page describes the new option or the partial-logger semantics (all methods optional), which the repo checklist asks for on new features. The bug hunting system found no bugs in the current revision.

@mattzcarey mattzcarey force-pushed the feat/injectable-logger branch from a95a283 to 453b520 Compare July 6, 2026 19:02
@mattzcarey mattzcarey enabled auto-merge (squash) July 6, 2026 19:02
Comment on lines +74 to +76
## 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 logger option, but two client-package [mcp-sdk] diagnostics in the OAuth flow (packages/client/src/client/auth.ts:136 and auth.ts:1174) still go straight to console.warn and cannot reach the injected logger, since the auth helpers take an OAuthClientProvider rather than ProtocolOptions. Non-blocking (the auth code is pre-existing and untouched by this PR): either soften the docs/changeset wording to note the OAuth-flow exception, or thread a logger into the auth helpers in a follow-up, mirroring how the createMcpHandler warning was resolved here.

Extended reasoning...

What the gap is

This PR introduces the injectable SdkLogger (ProtocolOptions.logger) and migrates the SDK's raw console diagnostics to it — the client capability console.debug lines, the x-mcp-header exclusion warning at client.ts:2444, the tool-name warnings, the zod-fallback warning (tools and prompts paths), the SEP-2243 registration warning in mcp.ts, and (after an earlier review comment) the createMcpHandler responseMode: 'json' warning via a new CreateMcpHandlerOptions.logger option. Two client-package [mcp-sdk] diagnostics survive on the raw console with no route to the injected logger:

  • packages/client/src/client/auth.ts:136 — the SEP-2352 "stored OAuth credential has no issuer stamp" warning in discardIfIssuerMismatch
  • packages/client/src/client/auth.ts:1174 — the SEP-2352 "OAuthClientProvider does not implement saveDiscoveryState()" warning

Why they cannot be routed today

The OAuth helpers are standalone functions that take an OAuthClientProvider (plus an options bag), not ProtocolOptions, and the transports that invoke auth() are constructed independently of the Client. So a user configuring new Client(info, { logger }) — the exact scenario from #1262 (capture or silence SDK console output) — still gets these two lines on the raw console with no supported redirect. There is structurally no way for the new ProtocolOptions.logger to reach these call sites without an API change to the auth helpers.

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:

  • The changeset says "client and server SDK diagnostics can be routed through user-provided logging" — a blanket claim.
  • The new docs section (docs/servers/logging-progress-cancellation.md, "Route SDK diagnostics") says "Pass logger to route local warnings and debug messages from SDK internals."

A repo-wide grep of packages/client/src shows these two auth.ts sites are the only remaining raw-console SDK diagnostics after this PR (the other console usage is the intentional, user-overridable withLogging middleware default at middleware.ts:181); packages/server/src has zero left. So the claim is almost fully backed — these are the last two exceptions.

Concrete walk-through

  1. A user adopts the new option: const client = new Client({ name: 'c', version: '1.0.0' }, { logger: { warn: myWarn } }).
  2. They connect over Streamable HTTP with an authProvider that predates SEP-2352 (no saveDiscoveryState()/discoveryState(), or credentials stored without an issuer stamp).
  3. During the OAuth flow, discardIfIssuerMismatch runs at auth.ts:136 and/or the discovery-state check runs at auth.ts:1174 — both call console.warn('[mcp-sdk] ...') directly.
  4. myWarn is never called; the warning lands on the raw console despite the configured logger — the exact behavior the docs sentence says the option prevents.

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 logger into the auth helpers in a follow-up, mirroring how the createMcpHandler warning was resolved in this PR by adding CreateMcpHandlerOptions.logger. Routing a logger through the provider-based auth flow is an API-surface decision rather than a mechanical replacement, so it should not block this merge — hence a nit, not a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inject your own logger

2 participants