-
Notifications
You must be signed in to change notification settings - Fork 1.8k
docs: @deprecated roots/sampling/logging (advisory; SEP-2577) #2071
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -247,6 +247,9 @@ export type ServerContext = BaseContext & { | |
| /** | ||
| * Send a log message notification to the client. | ||
| * Respects the client's log level filter set via logging/setLevel. | ||
| * | ||
| * @deprecated SEP-2577 deprecates the MCP Logging feature (advisory; no wire change). | ||
| * Prefer stderr or OpenTelemetry for server diagnostics. | ||
|
Comment on lines
+250
to
+252
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 prose docs ( Extended reasoning...What's missingThis PR adds
A grep for Why this mattersThe REVIEW.md "Tests & docs" checklist explicitly says: "Bugfix or behavior change: check whether docs/**/*.md describes the old behavior and needs updating; flag prose that now contradicts the implementation." This PR self-identifies as a "Documentation update" in the checklist, yet the actual prose documentation is the one artifact that wasn't updated. Step-by-step user impact
The JSDoc tooltip does contain the replacement hint, so users aren't completely stranded — but the prose docs are now actively misleading by recommending deprecated surface without acknowledgement. Suggested fixAdd a short admonition to each affected section, e.g.:
At minimum this should land in SeverityFiling as a nit rather than blocking: the prose isn't strictly wrong (the features still work; deprecation is advisory-only with no wire change), and the JSDoc tooltips do surface the replacement guidance. It's also plausible this is intentionally scoped for a follow-up PR. But since the PR is labeled as a documentation update and the REVIEW.md checklist explicitly calls for flagging prose that contradicts the implementation, it's worth noting. This is distinct from any concern about |
||
| */ | ||
| log: (level: LoggingLevel, data: unknown, logger?: string) => Promise<void>; | ||
|
|
||
|
|
@@ -257,6 +260,8 @@ export type ServerContext = BaseContext & { | |
|
|
||
| /** | ||
| * Request LLM sampling from the client. | ||
| * | ||
| * @deprecated SEP-2577 deprecates the MCP Sampling feature (advisory; no wire change). | ||
| */ | ||
| requestSampling: ( | ||
| params: CreateMessageRequest['params'], | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1010,6 +1010,7 @@ export class McpServer { | |||||||||||||
| * Sends a logging message to the client, if connected. | ||||||||||||||
| * Note: You only need to send the parameters object, not the entire JSON-RPC message. | ||||||||||||||
| * @see {@linkcode LoggingMessageNotification} | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Logging feature (advisory; no wire change). | ||||||||||||||
|
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. 🟡 nit: The Extended reasoning...What this isThis PR deprecates three server-side surfaces for the MCP Logging feature, all referencing SEP-2577. Two of them include a one-line migration hint; the third does not:
All three deprecate the same feature with the same SEP reference; only Why
|
||||||||||||||
| * @param params | ||||||||||||||
| * @param sessionId Optional for stateless transports and backward compatibility. | ||||||||||||||
| * | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -466,18 +466,24 @@ | |||||||||||||
| /** | ||||||||||||||
| * Request LLM sampling from the client (without tools). | ||||||||||||||
| * Returns single content block for backwards compatibility. | ||||||||||||||
| * | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Sampling feature (advisory; no wire change). | ||||||||||||||
| */ | ||||||||||||||
| async createMessage(params: CreateMessageRequestParamsBase, options?: RequestOptions): Promise<CreateMessageResult>; | ||||||||||||||
|
Check warning on line 472 in packages/server/src/server/server.ts
|
||||||||||||||
|
Comment on lines
+470
to
472
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. 🟡 nit: Extended reasoning...What the gap isThis PR's stated scope is to add advisory
Why
|
||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Request LLM sampling from the client with tool support. | ||||||||||||||
| * Returns content that may be a single block or array (for parallel tool calls). | ||||||||||||||
| * | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Sampling feature (advisory; no wire change). | ||||||||||||||
| */ | ||||||||||||||
| async createMessage(params: CreateMessageRequestParamsWithTools, options?: RequestOptions): Promise<CreateMessageResultWithTools>; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Request LLM sampling from the client. | ||||||||||||||
| * When tools may or may not be present, returns the union type. | ||||||||||||||
| * | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Sampling feature (advisory; no wire change). | ||||||||||||||
| */ | ||||||||||||||
| async createMessage( | ||||||||||||||
| params: CreateMessageRequest['params'], | ||||||||||||||
|
|
@@ -632,6 +638,12 @@ | |||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Request the client's filesystem roots. | ||||||||||||||
| * | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Roots feature (advisory; no wire change). | ||||||||||||||
| * Prefer passing scope via tool parameters or server configuration. | ||||||||||||||
| */ | ||||||||||||||
|
felixweinberger marked this conversation as resolved.
|
||||||||||||||
| async listRoots(params?: ListRootsRequest['params'], options?: RequestOptions) { | ||||||||||||||
| return this._requestWithSchema({ method: 'roots/list', params }, ListRootsResultSchema, options); | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -642,6 +654,9 @@ | |||||||||||||
| * @see {@linkcode LoggingMessageNotification} | ||||||||||||||
| * @param params | ||||||||||||||
| * @param sessionId Optional for stateless transports and backward compatibility. | ||||||||||||||
| * | ||||||||||||||
| * @deprecated SEP-2577 deprecates the MCP Logging feature (advisory; no wire change). | ||||||||||||||
| * Prefer stderr or OpenTelemetry for server diagnostics. | ||||||||||||||
| */ | ||||||||||||||
| async sendLoggingMessage(params: LoggingMessageNotification['params'], sessionId?: string) { | ||||||||||||||
| if (this._capabilities.logging && !this.isMessageIgnored(params.level, sessionId)) { | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 nit: Consider adding a patch-level changeset for
@modelcontextprotocol/{core,client,server}noting the SEP-2577 advisory deprecation.@deprecatedJSDoc is emitted into the published.d.ts, so consumers will see IDE strikethrough on these methods after upgrading — that's a user-visible change worth a CHANGELOG entry. (Precedent:.changeset/register-rawshape-compat.mddocumented a prior@deprecatedaddition.) Non-blocking.Extended reasoning...
What this is
This PR adds
@deprecatedJSDoc to ~8 public API members across three published packages:@modelcontextprotocol/client:Client.setLoggingLevel,Client.sendRootsListChanged@modelcontextprotocol/server:Server.createMessage(all overloads),Server.listRoots,Server.sendLoggingMessage,McpServer.sendLoggingMessage@modelcontextprotocol/core:ServerContext.mcpReq.log,ServerContext.mcpReq.requestSamplingThe diff touches only these four source files;
git show --stat 3b256a8confirms no.changeset/*.mdwas added.Why it's user-visible
TypeScript preserves
@deprecatedtags in emitted.d.tsfiles. After upgrading, every consumer who callsserver.createMessage(...),ctx.mcpReq.log(...), etc. will see IDE strikethrough, and projects with"reportDeprecated": truein tsconfig will get a tsc diagnostic. That's a behaviour change in the consuming developer's experience, even though there is no runtime or wire-level change.Step-by-step
@modelcontextprotocol/serverat the current version and code containingawait server.createMessage({...}).node_modules/@modelcontextprotocol/server/dist/server/server.d.tsnow carries@deprecated SEP-2577 ...abovecreateMessage.Repo convention
The repo uses changesets as its only changelog mechanism (
release.ymlrunschangesets/action;.changeset/has 60+ pending entries). Recent merged PRs (#1855 — a type-only fix forexactOptionalPropertyTypes, #1887, #1974) all included changesets. There is also direct precedent for exactly this kind of change:.changeset/register-rawshape-compat.mdwas added specifically to document a prior@deprecatedaddition to public overloads.Caveats / why this is a nit, not a blocker
There is no CI gate enforcing changeset presence, and
CONTRIBUTING.mddoes not mandate one. Some merged commits (e.g. pure refactors) have landed without changesets. So this is a convention/process suggestion, not a hard requirement violation — hence nit, non-blocking.Suggested fix