remove tasks#2097
Conversation
|
|
@claude review |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| LATEST_PROTOCOL_VERSION, | ||
| METHOD_NOT_FOUND, | ||
| PARSE_ERROR, | ||
| RELATED_TASK_META_KEY, | ||
| SUPPORTED_PROTOCOL_VERSIONS | ||
| } from '../../types/constants.js'; |
There was a problem hiding this comment.
🔴 This PR removes a large set of public exports from @modelcontextprotocol/core, /client, and /server (e.g. InMemoryTaskStore, RELATED_TASK_META_KEY, isTerminal, ExperimentalClientTasks, ExperimentalServerTasks, all Task* types, the tasks capability field) and also deletes three pending changesets (extract-task-manager.md, fix-failed-task-result-retrieval.md, fix-task-session-isolation.md) without adding a replacement — changeset-bot is already flagging "No Changeset found". Please add a .changeset/*.md covering @modelcontextprotocol/core, /client, and /server so the next release gets a version bump and the CHANGELOG documents the removal of the experimental Tasks API.
Extended reasoning...
What's missing
This PR strips the entire experimental Tasks subsystem from three published packages, but ships no changeset. Worse, it deletes three pending changesets that were queued for the next release:
.changeset/extract-task-manager.md(minor bump for core/client/server).changeset/fix-failed-task-result-retrieval.md(patch for core).changeset/fix-task-session-isolation.md(patch for core)
After this merge, .changeset/ contains nothing for these packages, and the changeset-bot comment on the PR confirms: "
Why a changeset is required here
The PR description self-identifies as "Breaking Changes: Yes", and the diff bears that out. The following symbols are removed from the published public barrels:
@modelcontextprotocol/core(packages/core/src/exports/public/index.ts):RELATED_TASK_META_KEY,isTaskAugmentedRequestParams,isTerminal,InMemoryTaskStore,InMemoryTaskMessageQueue,assertClientRequestTaskCapability,assertToolsCallTaskCapability, allTask*/Queued*types,TaskCreatedMessage,TaskStatusMessage,RequestTaskStore,TaskContext,TaskManagerOptions,TaskRequestOptions. Thetasksfield is also removed fromClientCapabilities/ServerCapabilities, andtask/relatedTaskfromRequestOptions/NotificationOptions.@modelcontextprotocol/client(packages/client/src/index.ts):ExperimentalClientTasks, plus theclient.experimental.tasksaccessor.@modelcontextprotocol/server(packages/server/src/index.ts):ExperimentalServerTasks,ExperimentalMcpServerTasks,ToolTaskHandler,CreateTaskRequestHandler,TaskRequestHandler, plusserver.experimental.tasksandmcpServer.experimental.tasks.
Even though these were tagged @experimental, they were exported from the package root barrels and consumers may be importing them. Per REVIEW.md § Documentation & Changesets and § Backwards compat, removing public exports requires a changeset so the version is bumped and the CHANGELOG records the removal.
Step-by-step: what happens without a fix
- This PR merges with
.changeset/empty for core/client/server. - The next
changesets versionrun finds nothing for these packages → no version bump, no CHANGELOG entry. - If any other PR later adds an unrelated patch changeset, core/client/server publish a new version whose CHANGELOG says nothing about ~18k lines of removed Tasks API.
- A consumer with
import { InMemoryTaskStore, isTerminal } from '@modelcontextprotocol/core'upgrades and gets a compile error with no release-notes guidance. - Additionally, the three deleted changesets described fixes that did ship in earlier commits on this branch — deleting them silently drops those entries from the upcoming CHANGELOG too.
Fix
Add a single replacement changeset, e.g.:
---
'@modelcontextprotocol/core': minor
'@modelcontextprotocol/client': minor
'@modelcontextprotocol/server': minor
---
Remove the experimental Tasks API (TaskManager, InMemoryTaskStore, ExperimentalClientTasks,
ExperimentalServerTasks, Task* types/schemas, `tasks` capability, `task`/`relatedTask`
on RequestOptions/NotificationOptions, `ctx.task` handler context).(Use major instead of minor if the team treats removal of @experimental exports as semver-breaking; either way, some changeset is required.) Deleting the three obsolete task-related changesets is fine once this replacement is in place, since their content is superseded by the removal.
| // Response message types | ||
| export type { | ||
| BaseResponseMessage, | ||
| ErrorMessage, | ||
| ResponseMessage, | ||
| ResultMessage, | ||
| TaskCreatedMessage, | ||
| TaskStatusMessage | ||
| } from '../../shared/responseMessage.js'; | ||
| export type { BaseResponseMessage, ErrorMessage, ResponseMessage, ResultMessage } from '../../shared/responseMessage.js'; | ||
| export { takeResult, toArrayAsync } from '../../shared/responseMessage.js'; |
There was a problem hiding this comment.
🔴 With all Tasks-API stream producers (requestStream, callToolStream, createMessageStream, elicitInputStream) deleted, nothing in the SDK yields AsyncGenerator<ResponseMessage<T>> anymore — yet responseMessage.ts (ResponseMessage, ResultMessage, ErrorMessage, BaseResponseMessage, takeResult, toArrayAsync) is left in place and still re-exported here. The PR partially edited that file (stripped TaskCreatedMessage/TaskStatusMessage, rewrote docstrings) but left a now-pointless two-arm union and helpers whose only producers are gone. Since this PR is already a breaking removal, responseMessage.ts and its public re-exports should be deleted as part of it rather than shipped as orphaned public API.
Extended reasoning...
What this is
This PR removes the experimental Tasks API. Among the deletions are the four streaming methods that produced AsyncGenerator<ResponseMessage<T>>: TaskManager.requestStream (core), ExperimentalClientTasks.requestStream/callToolStream (client), and ExperimentalServerTasks.requestStream/createMessageStream/elicitInputStream (server). Those were the only producers of the ResponseMessage stream type in the entire SDK.
However, packages/core/src/shared/responseMessage.ts is not deleted. The PR touches it — removing TaskCreatedMessage and TaskStatusMessage from the union and rewriting the docstrings to drop task references — but leaves BaseResponseMessage, ResultMessage<T>, ErrorMessage, ResponseMessage<T>, takeResult, and toArrayAsync in place. All six remain re-exported from packages/core/src/exports/public/index.ts:54-56 and from packages/core/src/index.ts.
Why it's orphaned
A repo-wide grep for ResponseMessage, ResultMessage, ErrorMessage, BaseResponseMessage, takeResult, and toArrayAsync after this PR returns only three locations: responseMessage.ts itself, packages/core/src/index.ts (export * from './shared/responseMessage.js'), and the explicit re-exports in exports/public/index.ts. There are zero call sites, zero return-type references, and zero tests. The two lowercase responseMessage hits in packages/client/src/client/streamableHttp.ts:669 and its test are unrelated local variable names, not imports of this module.
Step-by-step proof
- Before this PR,
ResponseMessage<T>wasTaskStatusMessage | TaskCreatedMessage | ResultMessage<T> | ErrorMessage— yielded byTaskManager.requestStream()intaskManager.tsand re-yielded by the client/server experimental wrappers. - This PR deletes
taskManager.ts(915 lines),packages/client/src/experimental/tasks/client.ts, andpackages/server/src/experimental/tasks/server.ts. EveryAsyncGenerator<ResponseMessage<...>>signature in the codebase is removed. - The PR then edits
responseMessage.tsto collapse the union toResultMessage<T> | ErrorMessageand rewrites the JSDoc from "Union of all message types yielded by task-aware streaming APIs such as callToolStream()…" to the generic "Union of all message types yielded by streaming APIs." But no such streaming APIs remain. takeResult()'s docstring still says "Consumes a ResponseMessage stream and returns the final result" — describing a stream type that nothing in the SDK now emits.- Result: six public exports with zero concrete producers or consumers anywhere in
packages/,examples/, ortest/.
Why it matters for this PR specifically
REVIEW.md's "Completeness" section says: "grep the package for surviving instances of the old form — partial migrations leave sibling code paths with the very bug the PR claims to fix." This PR's stated purpose is reducing surface area by ~17,800 lines; leaving responseMessage.ts half-edited is exactly the kind of partial migration that section warns about. REVIEW.md's "API surface" section adds: "every new export is intentional… abstractions have at least one concrete callsite in the SDK." After this PR, ResponseMessage and friends have none.
The fact that the author did edit responseMessage.ts (stripping the task-specific arms) shows they were aware it was task-coupled — the cleanup just stopped one step short. And because this PR is already a breaking change (it removes dozens of public types), this is the right place to drop these exports; leaving them in means a second breaking PR later just to remove dead types, which is harder to justify once they've shipped in a release.
Fix
Delete packages/core/src/shared/responseMessage.ts, remove export * from './shared/responseMessage.js' from packages/core/src/index.ts, and remove the two re-export lines from packages/core/src/exports/public/index.ts:54-56. Alternatively, if there's a concrete near-term plan to reintroduce a non-task streaming API that yields this type, note that in the PR description so the retention is intentional rather than accidental.
|
|
||
| Located in `packages/*/src/experimental/`: | ||
|
|
There was a problem hiding this comment.
🟡 The ### Experimental Features section in CLAUDE.md is left dangling — its only bullet (Tasks) was removed, but the heading and the Located in packages/*/src/experimental/: lead-in remain, now pointing at directories this PR deletes. Suggest removing the section entirely (lines 105–108).
Extended reasoning...
What the issue is
This PR removes the experimental Tasks API in its entirety, including deleting packages/core/src/experimental/, packages/client/src/experimental/, and packages/server/src/experimental/. In CLAUDE.md, the diff removes the single bullet under Experimental Features:
### Experimental Features
Located in `packages/*/src/experimental/`:
-- **Tasks**: Long-running task support with polling/resumption (`packages/core/src/experimental/tasks/`)
-
### Zod SchemasAfter this change, lines 105–108 of CLAUDE.md read:
### Experimental Features
Located in `packages/*/src/experimental/`:
### Zod Schemas
— a heading followed by a lead-in sentence ending in a colon, with no list items, immediately followed by the next heading.
Why this is a problem
There are two issues with the leftover prose:
- Empty section: The lead-in
Located in packages/*/src/experimental/:syntactically introduces a list, but there are no items. This reads as broken/incomplete documentation. - Stale path reference: The PR deletes
packages/core/src/experimental/index.ts,packages/client/src/experimental/index.ts, andpackages/server/src/experimental/index.ts(and everything under them). The globpackages/*/src/experimental/therefore points at directories that no longer exist, which will mislead anyone using CLAUDE.md to navigate the codebase.
Step-by-step proof
- Before this PR, CLAUDE.md §Experimental Features had exactly one bullet:
- **Tasks**: Long-running task support…. - The diff hunk for CLAUDE.md (
@@ -106,8 +106,6 @@) removes that bullet and the trailing blank line, but leaves the### Experimental Featuresheading and theLocated in packages/*/src/experimental/:line untouched. - The same PR deletes
packages/{core,client,server}/src/experimental/index.tsand all files under those directories (visible in the changed-files list and the diff). - Result: CLAUDE.md now contains a section header + path reference with zero content, pointing at paths that were removed in the same PR.
Impact
This is purely a documentation-consistency issue with no runtime effect. CLAUDE.md is the repo's onboarding/architecture doc, so leaving it referencing deleted directories degrades its accuracy but doesn't break anything.
Suggested fix
Delete the ### Experimental Features heading and the Located in packages/*/src/experimental/: line so the doc goes straight from the middleware paragraph to ### Zod Schemas. If experimental features are reintroduced later, the section can be re-added at that time.
| const jsonrpcNotification: JSONRPCNotification = { | ||
| jsonrpc: '2.0', | ||
| method: notification.method, | ||
| ...(notification.params && { params: notification.params }) | ||
| }; |
There was a problem hiding this comment.
🟡 nit: jsonrpcNotification is now an unconditionally-assigned const at line 833, but the two send sites at lines 866 and 873 still use jsonrpcNotification!. The non-null assertions are dead leftovers from when this value came from taskResult.queued ? undefined : taskResult.jsonrpcNotification — drop the !.
Extended reasoning...
What
In Protocol.notification() (packages/core/src/shared/protocol.ts), this PR replaced the previous task-aware construction of jsonrpcNotification:
// before
const taskResult = await this._taskManager.processOutboundNotification(notification, options);
const queued = taskResult.queued;
const jsonrpcNotification = taskResult.queued ? undefined : taskResult.jsonrpcNotification;with an unconditional declaration:
// after (line 833)
const jsonrpcNotification: JSONRPCNotification = {
jsonrpc: '2.0',
method: notification.method,
...(notification.params && { params: notification.params })
};However, the two consumers further down were not updated:
- line 866:
this._transport?.send(jsonrpcNotification!, options).catch(...) - line 873:
await this._transport.send(jsonrpcNotification!, options);
Why the ! is now dead
Previously jsonrpcNotification was typed JSONRPCNotification | undefined because the ternary could produce undefined when the notification was queued through the task message queue. The early-return on queued guaranteed it was defined at the send sites, but TypeScript couldn't narrow that, so ! was used to silence the compiler.
After this PR the variable is a plain const jsonrpcNotification: JSONRPCNotification — never undefined. The non-null assertion now asserts away nothing.
Step-by-step
notification()is called.- Line 833 assigns a concrete
JSONRPCNotificationliteral tojsonrpcNotification. - Whether or not
canDebounceis true, control reaches either line 866 or line 873. - At both points
jsonrpcNotificationis statically and dynamically a non-undefinedJSONRPCNotification;!has no effect on type or runtime.
Impact
No runtime impact — ! is erased at compile time. But it misleads readers into thinking jsonrpcNotification can be undefined here, and falls under the repo's REVIEW.md correctness checklist ("no unsafe assertions") and "Completeness" (partial migration leaves leftover artifacts of the old form). The PR's own "clean up" commit (8d651ab) touched this file but missed these two.
Fix
Drop the ! at both sites:
this._transport?.send(jsonrpcNotification, options).catch(error => this._onerror(error));
...
await this._transport.send(jsonrpcNotification, options);| /** | ||
| * Supertype that can handle both regular tools (simple callback) and task-based tools (task handler object). | ||
| * Tool handler type — either a regular callback or (for compatibility) itself. | ||
| */ | ||
| export type AnyToolHandler<Args extends StandardSchemaWithJSON | undefined = undefined> = ToolCallback<Args> | ToolTaskHandler<Args>; | ||
| export type AnyToolHandler<Args extends StandardSchemaWithJSON | undefined = undefined> = ToolCallback<Args>; |
There was a problem hiding this comment.
🟡 After removing ToolTaskHandler from the union, AnyToolHandler<Args> is now a bare alias of ToolCallback<Args>, and the rewritten JSDoc — "either a regular callback or (for compatibility) itself" — is incoherent. Since this PR is already a breaking removal, consider dropping the alias (and its public re-export at packages/server/src/index.ts:12) and using ToolCallback directly; at minimum, fix the JSDoc to something coherent like @deprecated Use {@linkcode ToolCallback}.
Extended reasoning...
What changed
Before this PR, AnyToolHandler was a meaningful supertype:
/**
* Supertype that can handle both regular tools (simple callback) and task-based tools (task handler object).
*/
export type AnyToolHandler<Args ...> = ToolCallback<Args> | ToolTaskHandler<Args>;After removing ToolTaskHandler, the PR collapsed it to:
/**
* Tool handler type — either a regular callback or (for compatibility) itself.
*/
export type AnyToolHandler<Args ...> = ToolCallback<Args>;The new JSDoc is a botched hand-edit of the original — "either a regular callback or itself" is nonsensical prose on a public export.
Why it matters
AnyToolHandler is part of the public API surface: it's re-exported from packages/server/src/index.ts:12 and appears in the public RegisteredTool.handler field type (mcp.ts:1069). Users hovering this type in their IDE will see a docstring that doesn't parse as English. Internally it's also used at mcp.ts:682, :722, and :1096 — but in every case it now means exactly ToolCallback<Args>, so the indirection adds nothing.
Per the repo's REVIEW.md guidance ("one way to do things — improving an existing API beats adding a parallel one") and the general "avoid backwards-compatibility hacks like re-exporting types" rule, a vacuous public alias is the kind of partial-migration leftover the Completeness check is meant to catch.
Step-by-step
- A consumer imports
AnyToolHandlerfrom@modelcontextprotocol/server(index.ts:12). - TypeScript resolves it to
ToolCallback<Args>— structurally identical, no extra members. - IDE/typedoc renders the JSDoc: "Tool handler type — either a regular callback or (for compatibility) itself." — which describes a union that no longer exists and reads as "X or X".
- There is no scenario in which choosing
AnyToolHandleroverToolCallbackchanges type-checking behavior; the alias is pure noise.
On the refutation
One verifier marked bug_003 as REFUTED, but only because it duplicates bug_002 (same file, same lines, same finding). That's a dedup refutation, not a substantive disagreement — all seven verifier passes that examined the content confirmed the JSDoc is broken and the alias is vacuous.
Suggested fix
Preferred (since this PR is already breaking and the alias was tied to the experimental Tasks API being removed):
- Replace the four internal
AnyToolHandler<...>usages withToolCallback<...>. - Delete the
AnyToolHandlertype and remove it frompackages/server/src/index.ts:12.
Minimal (if you'd rather keep the export for one release):
/** @deprecated Use {@linkcode ToolCallback}. */
export type AnyToolHandler<Args extends StandardSchemaWithJSON | undefined = undefined> = ToolCallback<Args>;Either way, the current "or (for compatibility) itself" docstring needs to go.
1 participant
Remove experimental Tasks API
Motivation and Context
The experimental Tasks API (task creation, polling, listing, cancellation) was added as an experimental feature but is being removed before stabilization. This PR strips the entire Tasks subsystem from the SDK — schemas, types, runtime logic, client/server integration, examples, and tests — reducing the codebase by ~17,800 lines.
This simplifies the protocol layer (
Protocol,Client,Server,McpServer), removes task-related capability negotiation, and cleans up the request/notification options that carried task metadata (task,relatedTask).How Has This Been Tested?
pnpm test:all)pnpm typecheck:all)pnpm lint:all)packages/core/test/experimental/inMemory.test.tstest/integration/test/experimental/tasks/task.test.tstest/integration/test/experimental/tasks/taskListing.test.tstest/integration/test/taskLifecycle.test.tsBreaking Changes
Yes — users of the experimental Tasks API will need to remove all task-related code:
TaskSchema,TaskStatusSchema,TaskCreationParamsSchema,TaskMetadataSchema,RelatedTaskMetadataSchema,CreateTaskResultSchema,TaskAugmentedRequestParamsSchema,ClientTasksCapabilitySchema,ServerTasksCapabilitySchema, and all task request/notification schemas (tasks/get,tasks/list,tasks/cancel,tasks/result)tasksfield removed from bothClientCapabilitiesandServerCapabilitiestaskandrelatedTaskfields removed fromRequestOptionsandNotificationOptionstaskgroup (id,store,requestedTtl) removed from handler context typesTaskClientexperimental extension and its methodsTaskServer,TaskMcpServerexperimental extensions andtaskManageroption fromProtocolOptionsTaskManager,NullTaskManager,InMemoryTaskStore, and all task store interfacessimpleTaskInteractiveclient and server examplesmigration.mdandmigration-SKILL.mdTypes of changes
Checklist
Additional context
All removed code was under
experimental/directories and marked@experimental. The Tasks API had not been stabilized or included in a released version of the MCP spec. The migration docs have been updated to remove task-related migration guidance since there is nothing to migrate to.Files deleted (major):
packages/core/src/shared/taskManager.ts(915 lines)packages/core/src/experimental/tasks/(helpers, interfaces, stores)packages/client/src/experimental/tasks/(TaskClient)packages/server/src/experimental/tasks/(TaskServer, TaskMcpServer)examples/server/src/simpleTaskInteractive.ts(758 lines)examples/client/src/simpleTaskInteractiveClient.ts(204 lines)