feat(core,server,client)!: tasksPlugin() replaces 2025-11 interception; remove Experimental*Tasks classes (SEP-2663)#2066
Conversation
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| | `ToolTaskHandler` / `CreateTaskRequestHandler` / `TaskRequestHandler` | removed | | ||
|
|
||
| `TaskStore` / `InMemoryTaskStore` / `TaskMetadata` / `TaskMessageQueue` (storage interfaces) are unchanged. | ||
| `TaskStore` / `InMemoryTaskStore` / `TaskMetadata` (storage interfaces) are unchanged. |
There was a problem hiding this comment.
🟡 The migration docs drop TaskMessageQueue from the "storage interfaces are unchanged" line, but TaskMessageQueue and InMemoryTaskMessageQueue are still exported from packages/core/src/exports/public/index.ts:131,139 — and this PR deleted their only SDK consumer (TaskManager), so they're now orphaned public surface with undocumented status. Either remove/deprecate the exports and add them to the "removed" table, or restore them to the "unchanged" list with a note that the SDK no longer consumes them.
Extended reasoning...
What changed vs. what shipped
Both docs/migration.md and docs/migration-SKILL.md were edited in this PR to change the storage-interfaces line from:
TaskStore/InMemoryTaskStore/TaskMetadata/TaskMessageQueue(storage interfaces) are unchanged.
to:
TaskStore/InMemoryTaskStore/TaskMetadata(storage interfaces) are unchanged.
That is a deliberate edit — TaskMessageQueue was removed from the unchanged list. But it was not added to the "removed" table above it, and it was not removed from the public export surface. The result is a symbol whose migration status is simply undefined by the docs.
The export is still live
packages/core/src/exports/public/index.ts still exports both the type and the in-memory impl:
- line 131:
TaskMessageQueue(type, fromexperimental/tasks/interfaces.ts:104) - line 139:
InMemoryTaskMessageQueue(class, fromexperimental/tasks/stores/inMemory.ts:245)
These flow through to the @modelcontextprotocol/server and @modelcontextprotocol/client re-export barrels.
Step-by-step: why it's now orphaned
- Before this PR,
TaskMessageQueuewas consumed byTaskManagerviaTaskManagerOptions.taskMessageQueue(packages/core/src/shared/taskManager.ts, deleted in this diff). - This PR deletes
shared/taskManager.tsentirely and replaces it withtasksPlugin().TasksPluginOptionsis{ store: TaskStore; notify?: ... }— no queue parameter. - The example server (
examples/server/src/simpleStreamableHttp.ts) drops itsInMemoryTaskMessageQueueimport and thetaskMessageQueue:capability field. - A grep for remaining references shows only the definitions, the public-export barrel, and unit tests of the store itself — no SDK code path reads or accepts a
TaskMessageQueueanymore.
So after this PR, a user reading the migration guide sees TaskMessageQueue neither under "removed" nor under "unchanged", finds it still importable from @modelcontextprotocol/server, and has no guidance on whether to keep using it or what replaces it.
Why this matters
REVIEW.md § Completeness flags partial migrations that leave surviving instances of the old pattern, and § Documentation & Changesets flags prose that diverges from what the code ships. This is both: the doc edit implies a status change (dropped from "unchanged") that the code doesn't reflect (still exported), and the explicit "removed" list doesn't account for it. The repo also emphasizes that every public export is intentional — an export with no SDK consumer and no doc entry doesn't meet that bar.
How to fix
Pick one and make docs + exports agree:
- (a) Remove it. Drop
TaskMessageQueue/InMemoryTaskMessageQueue(and theQueued*companion types) frompackages/core/src/exports/public/index.ts, and add a row to the migration "removed" table:TaskMessageQueue / InMemoryTaskMessageQueue → removed; SEP-2663 has no message-queue side-channel. - (b) Keep it. Restore
TaskMessageQueueto the "unchanged" line in both migration docs, ideally with a note that the SDK itself no longer consumes it and it's retained only for userland stores.
Given the PR's own explanation that the queue/side-channel mechanism "does not exist anymore" under SEP-2663, (a) is likely the intended outcome.
| /** | ||
| * Value placed at `ctx.ext.task` by {@linkcode tasksPlugin}. Handlers cast | ||
| * `ctx.ext?.task as TaskContext` to read it. | ||
| */ |
There was a problem hiding this comment.
🟡 The JSDoc here says the value is "placed at ctx.ext.task" and that "Handlers cast ctx.ext?.task as TaskContext", but the plugin actually writes to ctx.ext['io.modelcontextprotocol/task'] (TASK_EXT_KEY, line 47). The tasksPlugin() JSDoc at line 62 ("injects env.ext.task"), the PR description, and docs/migration.md ("injects ctx.ext.task") have the same drift — a user following this literally would read undefined. Suggest pointing all of these at the taskContext(ctx) helper (or TASK_EXT_KEY) instead of the bare .task property.
Extended reasoning...
What the bug is
The inline documentation for TaskContext and tasksPlugin() describes a different ext key than the implementation actually uses. At packages/core/src/experimental/tasks/plugin.ts:35-36 the JSDoc reads:
Value placed at
ctx.ext.taskby {@linkcode tasksPlugin}. Handlers castctx.ext?.task as TaskContextto read it.
And at line 62 the tasksPlugin() JSDoc says it "injects env.ext.task". The PR description and docs/migration.md repeat the same phrasing ("injects ctx.ext.task").
But the implementation writes to a reverse-DNS key, not .task:
export const TASK_EXT_KEY = 'io.modelcontextprotocol/task'; // line 47
...
const nextEnv: RequestEnv = { ...env, ext: { ...env?.ext, [TASK_EXT_KEY]: taskCtx } }; // line ~158and the accessor reads from that same key:
export function taskContext(ctx: BaseContext): TaskContext | undefined {
return ctx.ext?.[TASK_EXT_KEY] as TaskContext | undefined;
}Step-by-step proof
- A user reads the
TaskContextJSDoc and writes a tool handler:mcp.tool('long-job', schema, async (args, ctx) => { const task = ctx.ext?.task as TaskContext; // per JSDoc const t = await task.store.createTask({ ttl: 60_000 }); ... });
tasksPluginmiddleware runs and buildsnextEnv.ext = { ...env?.ext, ['io.modelcontextprotocol/task']: taskCtx }. There is no'task'property on that object.- In the handler,
ctx.ext?.taskevaluates toundefined. task.store.createTask(...)throwsTypeError: Cannot read properties of undefined (reading 'store').
So the documented access pattern does not work; only taskContext(ctx) or ctx.ext?.['io.modelcontextprotocol/task'] does.
Why existing code doesn't prevent it
Nothing in the runtime aliases 'io.modelcontextprotocol/task' to 'task'. The ext bag is a plain Record<string, unknown> per the BaseContext.ext convention, and the plugin only ever assigns [TASK_EXT_KEY]. The taskContext() helper is the intended accessor and is used correctly in every code example, test, and the docs/migration.md snippet — it's only the surrounding prose/JSDoc that drifted.
Impact
Low. This is a documentation/JSDoc inaccuracy, not a runtime bug. All shipped examples (docs/migration.md code block, docs/server.md, taskLifecycle.test.ts, plugin.test.ts, migration-SKILL.md) steer users to taskContext(ctx), which works. But a user who reads the type's own JSDoc — the most authoritative-looking source in an IDE hover — and follows it literally will hit a confusing undefined. This matches the repo's recurring "prose that promises behavior the code no longer ships" review catch.
How to fix
Update the three prose locations to reference the helper (or the constant) rather than a bare .task property:
plugin.ts:35-36→ "Value placed atctx.ext[TASK_EXT_KEY]by {@linkcode tasksPlugin}. Read it via {@linkcode taskContext}."plugin.ts:62→ "...and injects a {@linkcode TaskContext} intoenv.ext(read via {@linkcode taskContext})."docs/migration.md→ "...registerstasks/get|list|cancel|resulthandlers and injects a task context readable viataskContext(ctx)."
(docs/migration-SKILL.md already points at taskContext(ctx) in its NEW column, so no change needed there.)
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
df45523 to
6bcdf6c
Compare
felixweinberger
force-pushed
the
fweinberger/f3-tasks-replace
branch
from
4588797 to
108c18d
Compare
|
@claude review |
| // | ||
| // `should respect server task capabilities`: | ||
| // Removed. Tasks is an extension under SEP-2663, not core protocol; there is no | ||
| // client-side `assertCapabilityForMethod` case for `tasks/*`. Capability is advertised; | ||
| // enforcement is the server having `tasksPlugin` registered. |
There was a problem hiding this comment.
🟡 This deletion-rationale comment says "there is no client-side assertCapabilityForMethod case for tasks/*. Capability is advertised; enforcement is the server having tasksPlugin registered" — but commit 3f81ccf in this same PR adds exactly that case to Client.assertCapabilityForMethod (packages/client/src/client/client.ts:566-575), and taskLifecycle.test.ts adds a Task capability gate (server without tasks) suite exercising it. The comment was evidently written before 3f81ccf and is now self-contradictory within the diff; suggest updating it to point at the new capability-gate test instead.
Extended reasoning...
What the bug is
The replacement comment block at test/integration/test/client/client.test.ts:2287-2291 explains why the old should respect server task capabilities test was removed:
should respect server task capabilities:
Removed. Tasks is an extension under SEP-2663, not core protocol; there is no
client-sideassertCapabilityForMethodcase fortasks/*. Capability is advertised;
enforcement is the server havingtasksPluginregistered.
But this same PR — specifically commit 3f81ccf "feat(client): assertCapabilityForMethod gates tasks/* on serverCapabilities.tasks; add capability tests" — adds exactly the case the comment says doesn't exist. packages/client/src/client/client.ts now contains:
case 'tasks/get':
case 'tasks/result':
case 'tasks/list':
case 'tasks/cancel': {
if (!this._serverCapabilities?.tasks) {
throw new SdkError(SdkErrorCode.CapabilityNotSupported, `Server does not support tasks (required for ${method})`);
}
break;
}And test/integration/test/taskLifecycle.test.ts adds a describe('Task capability gate (server without tasks)') suite that connects a client (with enforceStrictCapabilities: true) to a server that does not declare tasks, and asserts client.request({ method: 'tasks/get', ... }) rejects with "does not support tasks".
Step-by-step proof
- Read
client.test.ts:2289-2291→ claim: no client-sideassertCapabilityForMethodcase fortasks/*; enforcement is server-side only. - Read
packages/client/src/client/client.tsin this same diff → theswitch (method)inassertCapabilityForMethodnow has a'tasks/get' | 'tasks/result' | 'tasks/list' | 'tasks/cancel'arm that throwsSdkError(CapabilityNotSupported)whenserverCapabilities.tasksis absent. git log --onelineshows commit3f81ccf feat(client): assertCapabilityForMethod gates tasks/* …landed after2c075cc(which added the comment). The comment was accurate when written and went stale within the same PR.taskLifecycle.test.tsin this diff containsdescribe('Task capability gate (server without tasks)')exercising the new gate end-to-end.
So the comment directly contradicts code shipped two commits later in the same PR.
Why existing code doesn't prevent it
It's a non-executable comment in a test file — nothing in CI checks prose against behavior. This is the REVIEW.md "Documentation & Changesets" recurring catch ("prose that promises behavior the code no longer ships"); here the prose promises the absence of behavior the code now ships.
Impact
Low (no runtime effect), but a future reader looking for where the old capability test went will be told the gate doesn't exist, when in fact it does and is tested elsewhere. That's the kind of misdirection these deletion-rationale comments are specifically meant to avoid.
How to fix
Replace the paragraph with something like:
should respect server task capabilities:
Replaced by thetasks/*arm inClient.assertCapabilityForMethodand the
Task capability gate (server without tasks)suite intaskLifecycle.test.ts.
(or simply drop the paragraph, since the surrounding block already points readers at taskLifecycle.test.ts).
| // SEP-2663: client calls the tool normally; if the server returns a task, | ||
| // poll it to completion via `pollTask` (tasks/get + tasks/result). The | ||
| // 2025-11 `callToolStream` variant is removed. | ||
| async function callToolAndAwaitTask(name: string, args: Record<string, unknown>): Promise<CallToolResult> { | ||
| const initial = await client.callTool({ name, arguments: args }); | ||
| const asTask = initial as { resultType?: string; task?: { taskId: string } }; | ||
| if (asTask.resultType !== 'task' || !asTask.task) return initial as CallToolResult; | ||
| console.log(`Task created: ${asTask.task.taskId}; polling...`); | ||
| return (await pollTask(client, asTask.task.taskId)) as CallToolResult; | ||
| } | ||
|
|
||
| // Demo 1: Elicitation (confirm_delete) | ||
| console.log('\n--- Demo 1: Elicitation ---'); | ||
| console.log('Calling confirm_delete tool...'); | ||
| { | ||
| const result = await callToolAndAwaitTask('confirm_delete', { filename: 'important.txt' }); | ||
| console.log(`Result: ${getTextContent(result)}`); | ||
| } | ||
|
|
||
| // Demo 2: Sampling (write_haiku) | ||
| console.log('\n--- Demo 2: Sampling ---'); | ||
| console.log('Calling write_haiku tool...'); | ||
| { | ||
| const result = await callToolAndAwaitTask('write_haiku', { topic: 'autumn leaves' }); | ||
| console.log(`Result:\n${getTextContent(result)}`); | ||
| } |
There was a problem hiding this comment.
🟡 Follow-up to the docs/server.md fix (108c18d): the docs link was dropped, but this client file's own header (line 4) still says it "connects to simpleTaskInteractive.ts server", and the demo body now actively calls callTool({name:'confirm_delete',...}) without params.task — while examples/server/src/simpleTaskInteractive.ts (untouched) still throws "Tool … requires task mode" when params.task is absent and returns {task} without resultType. Before this PR the client was a harmless disabled stub; now it crashes on the first call against its documented companion. Suggest adding the same "server is being rewritten" banner here (or reverting to the disabled stub) until simpleTaskInteractive.ts is migrated.
Extended reasoning...
What the bug is
This PR rewrites examples/client/src/simpleTaskInteractiveClient.ts from a safe disabled stub into an active SEP-2663 demo, but its documented companion server (examples/server/src/simpleTaskInteractive.ts) was not migrated and still implements the 2025-11 client-directed model. The two files are explicitly paired — the client's header comment at line 4 says "This client connects to simpleTaskInteractive.ts server" — and that pairing is now broken at runtime.
The previous review round flagged the docs side of this (inline comment 3226615378 on docs/server.md), and commit 108c18d resolved it by dropping the docs link and adding a note that simpleTaskInteractive.ts "is being rewritten for the new model and is not yet a working reference". But that fix did not touch the client example itself: its header still points users at the unmigrated server, and its body now contains live demo code that cannot work against that server.
The specific code path that fails
The new callToolAndAwaitTask helper (lines ~121–127) calls:
const initial = await client.callTool({ name, arguments: args });
const asTask = initial as { resultType?: string; task?: { taskId: string } };
if (asTask.resultType !== 'task' || !asTask.task) return initial as CallToolResult;and is invoked at line ~133 with callToolAndAwaitTask('confirm_delete', { filename: 'important.txt' }). No params.task is sent — correct under SEP-2663, where the server decides to return a task pointer.
But examples/server/src/simpleTaskInteractive.ts:512–516 still reads request.params.task and throws Error("Tool ${name} requires task mode") when it is absent. A grep confirms that file has zero references to tasksPlugin, taskContext, or resultType; it still uses TaskMessageQueue and returns { task } (no resultType) at line ~606.
Why existing code doesn't prevent it
Nothing in CI runs the examples/ client/server pair end-to-end, and the unmigrated server still type-checks (the TaskMessageQueue types are still exported — see the separate comment on docs/migration-SKILL.md). The mismatch only surfaces when a user actually follows the file's own instructions and runs the migrated client against simpleTaskInteractive.ts.
Step-by-step proof
- Start
examples/server/src/simpleTaskInteractive.ts. - Run
examples/client/src/simpleTaskInteractiveClient.ts(whose header tells you to do exactly this). - The client reaches Demo 1 and sends
tools/callwith{ name: 'confirm_delete', arguments: { filename: 'important.txt' } }— notaskfield. - The server's
tools/callhandler at line 512 evaluatesrequest.params.task→undefined, hits line 516, and throwsError("Tool confirm_delete requires task mode"). - Even if step 4 were patched out, the server returns
{ task }withoutresultType, so the client'sasTask.resultType !== 'task'check passes and it treats the response as a directCallToolResult, never callingpollTask.
Before this PR the client printed "Interactive task demo disabled pending tasksPlugin (SEP-2663)" and exited cleanly. After this PR it actively crashes against its documented companion — a regression in the example's runnability.
Impact and fix
This is example code, not SDK surface, and the author has already acknowledged in docs/server.md that the server example is being rewritten — so filing as a nit. The actionable fix is small and matches what 108c18d did for the docs:
- Add a banner comment (and/or
console.log) tosimpleTaskInteractiveClient.tsnoting thatsimpleTaskInteractive.tsis not yet rewritten for SEP-2663 and pointing attest/integration/test/taskLifecycle.test.tsfor a working reference; or - Revert the client to the disabled stub until the server example is migrated in the same PR; or
- Migrate
simpleTaskInteractive.ts(and its README) totasksPlugin/taskContext/{resultType:'task', task}here.
Either way, the line-4 header should not advertise a pairing that fails on the first call.
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
6bcdf6c to
1ccef83
Compare
felixweinberger
force-pushed
the
fweinberger/f3-tasks-replace
branch
from
108c18d to
e28cbb3
Compare
|
@claude review |
| - `RequestOptions.task` / `RequestOptions.relatedTask`, `NotificationOptions.relatedTask` | ||
| - `protocol.taskManager` getter, `TaskManager` / `NullTaskManager` classes, `TaskManagerOptions` | ||
| - `RequestOptions.task` / `RequestOptions.relatedTask`, `NotificationOptions.relatedTask`, `TaskRequestOptions` | ||
| - `BaseContext.task` (`ctx.task?.store` / `ctx.task?.id` / `ctx.task?.requestedTtl`) |
There was a problem hiding this comment.
🟡 Pre-existing nit: CLAUDE.md:204 still documents task?: Task context ({ id?, store, requestedTtl? }) when task storage is configured as a field on the request handler context, but BaseContext.task was removed in R0 and this PR ships its replacement (taskContext(ctx) / ctx.ext[TASK_EXT_KEY]). Since this PR already updates docs/migration.md, docs/migration-SKILL.md, docs/server.md, and docs/client.md for the new task model, it's a natural place to also update (or drop) that line in CLAUDE.md so the architecture overview matches the shipped surface.
Extended reasoning...
What the issue is
CLAUDE.md:204 (the repo-level architecture overview) documents the request-handler context shape as including:
task?: Task context ({ id?, store, requestedTtl? }) when task storage is configured
But BaseContext (packages/core/src/shared/context.ts) no longer has a .task field. The replacement, shipped in this PR, is taskContext(ctx) reading from ctx.ext['io.modelcontextprotocol/task'] after mcp.use(tasksPlugin({store})) is registered.
Provenance / why "pre-existing"
To be precise about scope: BaseContext.task was removed in R0 (commit 39e10ab), not in this PR. The docs/migration.md line listing BaseContext.task as "gone" is unchanged context in this diff, not an addition. So CLAUDE.md was already stale before this PR landed.
However, this PR is the natural place to fix it because it:
- introduces the replacement (
taskContext(ctx),TASK_EXT_KEY,tasksPlugin), - updates
docs/migration.mdfrom "gone with no in-place replacement" → "replaced by tasksPlugin/taskContext", - updates
docs/migration-SKILL.mdfrom "future: ctx.ext.task via tasksPlugin()" → "taskContext(ctx) (after mcp.use(tasksPlugin({store})))", - updates
docs/server.mdanddocs/client.mdfor the new pattern,
…but skips CLAUDE.md. Per REVIEW.md § Completeness ("when a PR replaces a pattern, grep for surviving instances of the old form"), CLAUDE.md:204 is a surviving instance of the old ctx.task form.
Step-by-step proof
- Read
CLAUDE.md:204→- task?: Task context ({ id?, store, requestedTtl? }) when task storage is configured. - Grep
BaseContextdefinition inpackages/core/src/shared/context.ts→ notaskproperty; onlysessionId,mcpReq,http?,ext?. - Read
packages/core/src/experimental/tasks/plugin.ts:47-52(added in this PR) → task context lives atctx.ext['io.modelcontextprotocol/task'], accessed viataskContext(ctx). - A contributor following CLAUDE.md and writing
ctx.task?.storein a handler gets a TypeScript error (Property 'task' does not exist on type 'ServerContext') — or, if they cast through it,undefinedat runtime.
Impact
Low. CLAUDE.md is internal contributor/architecture documentation, not published API docs, and has no runtime effect. The user-facing docs (docs/server.md, docs/migration.md, docs/migration-SKILL.md) already point at taskContext(ctx). This is purely "prose contradicts implementation" in the repo overview — hence nit, and flagged as pre-existing since the staleness predates this PR.
How to fix
Replace CLAUDE.md:204 with something like:
ext?: Extension bag (Record<string, unknown>). WithtasksPluginregistered,taskContext(ctx)reads{ id?, store, requestedTtl? }from it.
…or simply delete the task? bullet, since ext? is the generic mechanism and taskContext is documented in docs/server.md.
| private _cachedKnownTaskTools: Set<string> = new Set(); | ||
| private _cachedRequiredTaskTools: Set<string> = new Set(); |
There was a problem hiding this comment.
🟡 This PR deletes the only callers of Client.isToolTask() (via the removed ExperimentalClientTasks / ClientInternal) and Client.isToolTaskRequired() (the if (this.isToolTaskRequired(params.name)) throw … guard at the top of callTool), leaving both private methods and their backing _cachedKnownTaskTools / _cachedRequiredTaskTools sets as dead code — cacheToolMetadata still populates the sets at lines 857-873 but nothing reads them. Suggest removing the two methods and the two Set fields (and the corresponding taskSupport population in cacheToolMetadata) as part of this migration.
Extended reasoning...
What the bug is
packages/client/src/client/client.ts retains four pieces of private surface that this PR orphans:
_cachedKnownTaskTools: Set<string>(line 218)_cachedRequiredTaskTools: Set<string>(line 219)private isToolTask(toolName)(line 835) — reads_cachedKnownTaskToolsprivate isToolTaskRequired(toolName)(line 847) — reads_cachedRequiredTaskTools
A repo-wide grep confirms the only references to any of these symbols are inside client.ts itself: the field declarations, the method definitions, a JSDoc cross-link at line 845, and cacheToolMetadata (lines 857-858, 870, 873) which clears and populates the two sets. Nothing else reads them.
How this PR created the dead code
Before this PR there were exactly two consumers:
-
isToolTaskRequiredwas called by the guard at the top ofcallTool:if (this.isToolTaskRequired(params.name)) { throw new ProtocolError(... 'requires task-based execution' ...); }
This PR deletes that guard (diff hunk at
callTool, line ~794). -
isToolTaskwas called byExperimentalClientTasksvia theClientInternalinterface inpackages/client/src/experimental/tasks/client.ts:interface ClientInternal { isToolTask(toolName: string): boolean; ... }
This PR deletes that entire file.
With both callers gone, the two private methods are unreachable, and the two Set fields they read become write-only state.
Step-by-step proof
- Grep
isToolTaskRequiredacrosspackages/→ only hit is the definition atclient.ts:847. The former call site incallToolis removed in this diff. - Grep
isToolTask\bacrosspackages/→ hits are the definition atclient.ts:835and the JSDoc{@linkcode isToolTask}atclient.ts:845. The formerClientInternal.isToolTaskconsumer is in a deleted file. - Grep
_cachedKnownTaskTools/_cachedRequiredTaskTools→ only the field declarations (218-219), the reads inside the two dead methods (840, 848), and the writes insidecacheToolMetadata(857-858, 870, 873). cacheToolMetadatais still called fromlistTools(line 915), so on everytools/listthe client iterates the tool list and populates two sets that no code path will ever consult.
Why existing code doesn't prevent it
Both methods are private, so TypeScript's noUnusedLocals does not flag unused private class members, and there is no external surface to break. The sets are written via .add() / .clear() so they don't trip unused-variable lint either. Nothing in CI catches dead private methods.
Impact
No runtime or correctness effect — this is purely dead private surface left behind by a partial migration (REVIEW.md § Completeness: "grep the package for surviving instances of the old form"). The only cost is a small amount of wasted work in cacheToolMetadata per tools/list call and reader confusion about why these caches exist.
How to fix
Delete:
_cachedKnownTaskToolsand_cachedRequiredTaskToolsfield declarations (lines 218-219)isToolTaskandisToolTaskRequiredmethod definitions (lines 835-849)- The
taskSupportbranch incacheToolMetadatathat populates them (lines 857-858 clears + theif (tool.execution?.taskSupport ...)block at ~868-874)
cacheToolMetadata should keep populating _cachedToolOutputValidators (still used by callTool's output-schema validation), so don't remove the method itself.
…n; remove Experimental*Tasks classes (SEP-2663)
tasksPlugin() is a DispatchMiddleware handling tasks/get|list|cancel|result and injecting taskContext(ctx). mcp.use(tasksPlugin({store})) replaces ProtocolOptions.tasks.
ExperimentalServerTasks/ExperimentalClientTasks/ExperimentalMcpServerTasks classes and *.experimental.tasks getters removed (every method threw; SEP-2663 client doesn't host tasks). Integration tests for task behavior rewritten against tasksPlugin; streaming-method tests removed (no SEP-2663 equivalent).
…lities.tasks; add capability tests
…point to taskLifecycle tests
felixweinberger
force-pushed
the
fweinberger/s2-ext-slot
branch
from
1ccef83 to
06673f2
Compare
felixweinberger
force-pushed
the
fweinberger/f3-tasks-replace
branch
from
e28cbb3 to
3bd4d2e
Compare
|
@claude review |
| // maxTotalTimeout bounds this loop, not each tasks/get; pass through other RequestOptions. | ||
| const { maxTotalTimeout: _drop, defaultPollInterval: _drop2, ...perCallOptions } = options ?? {}; | ||
| void _drop; | ||
| void _drop2; | ||
| while (true) { | ||
| options?.signal?.throwIfAborted(); | ||
| if (Date.now() >= deadline) { | ||
| throw new ProtocolError( | ||
| ProtocolErrorCode.InternalError, | ||
| `pollTask: task ${taskId} did not reach a terminal state within maxTotalTimeout (${maxTotal}ms)` | ||
| ); | ||
| } | ||
| const task = await host.request({ method: 'tasks/get', params: { taskId } }, TASK_SCHEMA, perCallOptions); | ||
| if (isTerminal(task.status)) { | ||
| if (task.status === 'cancelled') { | ||
| throw new ProtocolError(ProtocolErrorCode.InternalError, `Task ${taskId} was cancelled`); | ||
| } | ||
| const raw = await host.request({ method: 'tasks/result', params: { taskId } }, TASK_SCHEMA, perCallOptions); | ||
| if (resultSchema === undefined) return raw as Result; | ||
| const parsed = await validateStandardSchema(resultSchema, raw); | ||
| if (!parsed.success) throw new ProtocolError(ProtocolErrorCode.InternalError, parsed.error); |
There was a problem hiding this comment.
🟡 pollTask reads options.maxTotalTimeout — whose JSDoc (context.ts:93) promises "an SdkError with code SdkErrorCode.RequestTimeout will be raised" — but throws ProtocolError(ProtocolErrorCode.InternalError) when the deadline elapses, and likewise uses ProtocolError(InternalError) for the local result-schema parse failure where SdkErrorCode.InvalidResult exists for exactly that purpose. These are local SDK conditions, not JSON-RPC errors from the peer; suggest SdkError(RequestTimeout) for the deadline branch and SdkError(InvalidResult) for the !parsed.success branch (the cancelled branch is more debatable since it mirrors the deleted TaskManager.requestStream).
Extended reasoning...
What the issue is
pollTask (packages/core/src/experimental/tasks/plugin.ts:246-268) throws ProtocolError(ProtocolErrorCode.InternalError, ...) for three locally-detected conditions:
- line 254-258: the
maxTotalTimeoutdeadline elapsed, - line 263: the polled task's status is
'cancelled', - line 268:
validateStandardSchema(resultSchema, raw)returned!parsed.success.
None of these is a JSON-RPC error response from the peer. The SDK draws an explicit line between the two error classes:
ProtocolError— "JSON-RPC errors that cross the wire as error responses" (types/errors.ts JSDoc).SdkError— "local errors that never cross the wire … never serialized as JSON-RPC error responses" (sdkErrors.ts:1-8).
Why the maxTotalTimeout case is a contract violation
pollTask reuses RequestOptions.maxTotalTimeout by name (line 246: const maxTotal = options?.maxTotalTimeout ?? POLL_TASK_DEFAULT_MAX_TOTAL_MS). That option's own JSDoc at packages/core/src/shared/context.ts:93 states:
If exceeded, an {@linkcode SdkError} with code {@linkcode SdkErrorCode.RequestTimeout} will be raised, regardless of progress notifications.
streamDriver.ts:393 honours this exactly: throw new SdkError(SdkErrorCode.RequestTimeout, 'Maximum total timeout exceeded', { maxTotalTimeout, totalElapsed }). pollTask consumes the same option but throws ProtocolError(InternalError) instead — directly contradicting the documented contract of the option it reads.
Why the schema-validation case has a purpose-built code
SdkErrorCode.InvalidResult is defined as "Response result failed local schema validation" (sdkErrors.ts:30), and is what dispatcher.ts:182 and streamDriver.ts:205 throw for the identical !parsed.success condition. pollTask's if (!parsed.success) throw new ProtocolError(InternalError, parsed.error) at line 268 is the same local check with the wrong class.
Step-by-step proof (caller-visible effect)
- A caller follows the
maxTotalTimeoutJSDoc and writes:try { const result = await pollTask(client, taskId, CallToolResultSchema, { maxTotalTimeout: 30_000 }); } catch (e) { if (e instanceof SdkError && e.code === SdkErrorCode.RequestTimeout) { // retry / surface as transient } throw e; }
- The task does not reach a terminal state within 30s.
pollTask's loop hitsDate.now() >= deadlineand throwsnew ProtocolError(ProtocolErrorCode.InternalError, 'pollTask: task ... did not reach a terminal state within maxTotalTimeout (30000ms)').e instanceof SdkErrorisfalse; the timeout-handling branch is skipped and the error re-throws as if it were a server-side -32603.
A caller using e instanceof ProtocolError to mean "the server sent a JSON-RPC error" (which is what callTool's own JSDoc at client.ts:760 documents — "ProtocolError for protocol-level failures or SdkError for SDK-level issues (timeouts, missing capabilities)") will likewise misclassify all three.
Caveats / why this is a nit
- The
'cancelled'branch at line 263 mirrors what the deletedTaskManager.requestStreamdid (alsoProtocolError(InternalError, 'Task X was cancelled')), so that one is arguably carried-over precedent rather than a new inconsistency. Client.callToolitself (pre-existing, lines ~811/824) usesProtocolErrorfor local output-schema validation, so the SDK's convention is already imperfect.pollTaskis@experimental, the error messages are descriptive, and the error is thrown — only the class/code is off.
How to fix
if (Date.now() >= deadline) {
throw new SdkError(
SdkErrorCode.RequestTimeout,
`pollTask: task ${taskId} did not reach a terminal state within maxTotalTimeout (${maxTotal}ms)`,
{ maxTotalTimeout: maxTotal }
);
}
...
if (!parsed.success) throw new SdkError(SdkErrorCode.InvalidResult, parsed.error);The cancelled branch could stay as-is for parity with prior art, or move to SdkError as well for consistency — either is defensible.
| TaskToolExecution | ||
| } from '../../experimental/tasks/interfaces.js'; |
There was a problem hiding this comment.
🟡 Adjacent to (but not covered by) the TaskMessageQueue comment above: this barrel still exports CreateTaskServerContext, TaskServerContext, and TaskToolExecution (lines 128/135/137), whose only consumers — ToolTaskHandler / CreateTaskRequestHandler / TaskRequestHandler / registerToolTask — are deleted in this PR. Unlike the queue types these are defined as ServerContext & { task: {...} }, i.e. they describe the ctx.task shape this PR replaces with taskContext(ctx)/ctx.ext[TASK_EXT_KEY], so they're actively misleading rather than merely unused. Suggest dropping all three alongside the TaskMessageQueue/Queued* sweep and adding them to the migration 'removed' table next to the ToolTaskHandler row that already references them.
Extended reasoning...
What the issue is
packages/core/src/exports/public/index.ts:128,135,137 still re-exports CreateTaskServerContext, TaskServerContext, and TaskToolExecution from experimental/tasks/interfaces.ts. The file's own section header for these (interfaces.ts:21) reads "Task Handler Types (for registerToolTask)", and this PR deletes registerToolTask (packages/server/src/experimental/tasks/mcpServer.ts), ToolTaskHandler / CreateTaskRequestHandler / TaskRequestHandler (packages/server/src/experimental/tasks/interfaces.ts), and ExperimentalMcpServerTasks — every consumer of these three types. A repo-wide grep confirms each symbol now has exactly two references: its definition and the public-export barrel.
Why these are worse than the queue types
Unlike TaskMessageQueue (which is merely orphaned), CreateTaskServerContext and TaskServerContext are actively misleading. They are defined as:
export type CreateTaskServerContext = ServerContext & { task: { store: RequestTaskStore; requestedTtl?: number } };
export type TaskServerContext = ServerContext & { task: { id: string; store: RequestTaskStore; ... } };— i.e. they describe a ctx.task field on ServerContext. But BaseContext.task was removed in R0, and this PR ships its replacement: tasksPlugin writes to ctx.ext['io.modelcontextprotocol/task'] and handlers read it via taskContext(ctx). A user who finds these types in the public surface and writes a handler typed as (args, ctx: TaskServerContext) => ... will read ctx.task.store and get undefined at runtime. TaskToolExecution similarly only existed to constrain registerToolTask's config ("taskSupport cannot be 'forbidden' for task-based tools"), a constraint that no longer applies anywhere.
Why this is not a duplicate of the existing TaskMessageQueue comment
The earlier inline comment (#3226615390 on docs/migration-SKILL.md) scopes to TaskMessageQueue / InMemoryTaskMessageQueue and mentions "the Queued* companion types" in its fix suggestion. It does not name CreateTaskServerContext, TaskServerContext, or TaskToolExecution. These are a separate family — handler-context types under the registerToolTask section header, not message-queue types — and an author who acts on the earlier comment by dropping TaskMessageQueue + Queued* from the barrel would leave these three behind. The migration docs in this PR add explicit "removed" rows for ToolTaskHandler / CreateTaskRequestHandler / TaskRequestHandler (docs/migration-SKILL.md, docs/migration.md) but leave the backing context types those handlers consumed both exported and undocumented. So while the fix location overlaps (same export block), the symbols and rationale are distinct enough that the existing comment does not cover them.
Step-by-step proof
grep -rn CreateTaskServerContext packages/→ two hits:interfaces.ts:28(definition) andexports/public/index.ts:128(re-export). Same forTaskServerContext(interfaces.ts:36, index.ts:135) andTaskToolExecution(interfaces.ts:45, index.ts:137).- Their pre-PR consumers:
packages/server/src/experimental/tasks/interfaces.tsimportedCreateTaskServerContext/TaskServerContextto defineCreateTaskRequestHandler/TaskRequestHandler;mcpServer.tsimportedTaskToolExecutionforregisterToolTask's config. All three files are deleted in this diff. docs/migration-SKILL.mdadds| ToolTaskHandler / CreateTaskRequestHandler / TaskRequestHandler | removed |— the consumers are documented as gone, but the context types they were built on are neither removed nor listed.- A user imports
TaskServerContextfrom@modelcontextprotocol/server, types a tool callback asasync (args, ctx: TaskServerContext) => { await ctx.task.store.createTask(...) }, and getsTypeError: Cannot read properties of undefined (reading 'store')at runtime because nothing populatesctx.task.
Impact
Nit — experimental dead public surface, no runtime effect on the SDK itself. But per REVIEW.md § Completeness ("grep for surviving instances of the old form") and § API surface ("every public export is intentional"), exported types that describe a context shape this very PR replaces don't meet the bar.
How to fix
In packages/core/src/exports/public/index.ts, drop CreateTaskServerContext, TaskServerContext, and TaskToolExecution from the export list (alongside the TaskMessageQueue/Queued* sweep already flagged). Either delete the definitions from interfaces.ts:21-47 (the whole "Task Handler Types (for registerToolTask)" section) or mark them @deprecated with a pointer to taskContext(ctx). Add them to the migration "removed" table — they fit naturally on the existing ToolTaskHandler / CreateTaskRequestHandler / TaskRequestHandler row.
1 participant
Adds
tasksPlugin({store})— aDispatchMiddlewareregistered viamcp.use(tasksPlugin(...))that handlestasks/get|list|cancel|resultand injectsctx.ext.task. AddstaskContext(ctx)helper. Rewrites the integration tests R0 skipped where the behavior maps to SEP-2663's server-directed model.ExperimentalServerTasks.getTask/listTasks/etc. become@deprecatedthrow-stubs (under SEP-2663 the client does not host tasks, so server→clienttasks/*has no target).Motivation and Context
Implements SEP-2663 (tasks as an extension). Closes the gap R0 opened: tasks works again via the plugin model.
How Has This Been Tested?
1373 tests + 82 skipped; dual conformance both 40/40. New
plugin.test.ts+ rewrittentaskLifecycle/taskListingintegration suites. The remaining ~59 skipped tests have model-change notes (client-directedparams.taskand server-polls-client patterns are MRTR concerns, not tasks).Breaking Changes
Yes (experimental API).
ProtocolOptions.tasks→mcp.use(tasksPlugin({store})).ctx.task→taskContext(ctx). Seedocs/migration.md.Types of changes
Checklist
Additional context
Depends on S2 (and R0, which deleted the prior mechanism). Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.