v2 - RFC Extract Tasks out of protocol.ts into TaskManager#1673
Conversation
…re and task MQ on constructor
🦋 Changeset detectedLatest commit: 0e9dd77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
…lity' of github.com:KKonstantinov/typescript-sdk into feature/extract-tasks-in-task-manager-trigger-on-capability
|
Just leaving this thought process here. I wonder if looping through _modules just to support multiple modules (where we have only one right now) is premature abstraction/generalization here. This could fall back to direct calling of the task module now (while keeping it running only if the task capability is declared), and if additional modules are added in the future, this could probably be done in a 15 minute change/iteration. Wondering if it is worth the indirection today - probably not. Any new "module" I imagine would be a significant feature coming from the protocol spec and not a random SDK-level feature. Thus, it would have to be carefully implemented and reviewed anyway; and the looping of the _modules etc. could be added at that point. The real two benefits would be - tasks code runs runtime only if tasks is declared as a capability; ProtocolModule interface and Protocol Module could still exist if we removed the multi-module speculative abstraction; Still holds value in defining the contract, but Protocol would call the module directly rather than looping. Right now reading through the loop of _modules and seems like added cognitive complexity - but wanted to see how it'd look like. Leaving it as-is right now, but happy to remove that |
![mcp-claude[bot]](https://avatars.githubusercontent.com/in/2690174?s=60&v=4){kind=small}
There was a problem hiding this comment.
The extraction itself looks good - the TaskManager internals are solid. The issue is the ProtocolModule, I'm not sure we have enough justification to extract this. "Tasks" is really three things: server handlers, client polling, and the send-or-queue routing interceptor. The first two extract cleanly; the third is where the bugs @claude found come from.
I think we should drop ProtocolModule and wire TaskManager via direct composition with specific call sites — closer to what #1449 did. A NullTaskManager means requestStream works without tasks capability, and config flows through naturally.
Re: extensions (SEP-2133) — I considered whether ProtocolModule could be the foundation for an extension runtime, but most extensions just need handler registration, not lifecycle interception. We can add a module system later if real patterns demand it, I think it's going to be very hard to predict the shape of potential extensions. I think it's better to avoid further abstraction and build them once we know what the shape should be rather than guess.
…lity' of github.com:KKonstantinov/typescript-sdk into feature/extract-tasks-in-task-manager-trigger-on-capability
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new bugs found and previous issues appear addressed, but this is a 19-file architectural refactoring of core Protocol infrastructure with breaking changes -- worth a human confirming the design direction (NullTaskManager pattern, TaskManagerHost interface, direct composition) aligns with the team's vision.
Extended reasoning...
Overview
This PR extracts ~500 lines of task orchestration logic from the monolithic Protocol class into a dedicated TaskManager class. It touches 19 files across core, client, server, and integration test packages. A new NullTaskManager subclass provides passthrough behavior when tasks capability is not configured. The ProtocolModule abstraction loop from earlier revisions has been replaced with direct composition (Protocol holds a single _taskManager reference), addressing felixweinberger's architectural feedback.
Security risks
No security concerns. The changes are internal refactoring of task lifecycle management. No auth, crypto, or permission-related code is modified. The Zod validation regression (registerHandler bypassing schema validation) has been fixed -- registerHandler now calls schema.parse(request) before delegating to the handler.
Level of scrutiny
This warrants thorough human review. It refactors the core Protocol class that underpins all MCP communication, introduces a new class hierarchy (TaskManager / NullTaskManager), changes the public API surface (taskStore/taskMessageQueue moved into capabilities.tasks), and has documented breaking changes. The previous bug reports (asymmetric sendRequest/sendNotification gating, dropped config options, resource leak on assertTaskCapability throw, streaming regression) all appear to have been addressed in the current revision.
Other factors
- All 5 previously reported bugs appear fixed: (1)
defaultTaskPollInterval/maxTaskQueueSizeare now extracted viaextractTaskManagerOptionsand forwarded correctly, (2)sendRequestwrapping now gates onrelatedTaskIdlikesendNotification, (3) streaming works without tasks viaNullTaskManager, (4)registerHandlerincludes Zod validation, (5)processOutboundRequestis wrapped in try/catch for cleanup on synchronous throw. - The
NullTaskManager.processOutboundNotificationdropsrelatedTaskmetadata from_meta, but this is acceptable since tasks aren't configured in that path. - Tests were updated extensively (many
capabilities: {}changed tocapabilities: { tasks: {...} }) which is expected given the new requirement for explicit task capability declaration. - The author's own comment about premature abstraction with
_moduleslooping appears resolved -- the code now uses direct composition as felixweinberger suggested.
|
@claude review |
There was a problem hiding this comment.
All five previously reported bugs appear to be fixed, and the architectural feedback (direct composition instead of generic ProtocolModule, NullTaskManager for streaming without tasks capability) has been addressed. However, this is a large architectural refactoring (19 files, ~900-line new TaskManager, breaking API changes) touching the core protocol layer, so it warrants human sign-off before merging.
Extended reasoning...
Overview
This PR extracts ~500 lines of task orchestration logic from the monolithic Protocol class into a dedicated TaskManager class. Key changes:
- New
TaskManager(andNullTaskManagerno-op) inpackages/core/src/shared/taskManager.ts taskStore/taskMessageQueue/defaultTaskPollInterval/maxTaskQueueSizemoved fromProtocolOptionsintocapabilities.tasksonClientOptions/ServerOptionsExperimentalClientTasksandExperimentalServerTasksnow delegate toTaskManagerdirectly instead of unsafe casts to internal Protocol methods- Protocol now holds a single
_taskManagerfield with direct calls (no generic module loop) - 19 files changed across core, client, server, examples, and integration tests
Security risks
No security-sensitive changes. The PR does not touch auth, crypto, permissions, or input sanitization beyond what was already in place. The Zod schema validation for task request handlers was initially dropped but has been restored in registerHandler.
Level of scrutiny
This PR warrants high scrutiny:
- It refactors the core protocol layer that all MCP communication flows through
- It introduces breaking API changes (task config location, handler context shape)
- The
TaskManageris a new ~900-line class that takes over critical request/response routing logic - The interaction between
NullTaskManagerandTaskManageracross inbound/outbound request paths is subtle
Other factors
- All five bugs previously found by the bug hunting system have been addressed: config forwarding, sendRequest gating asymmetry, streaming without tasks capability, schema validation in registerHandler, and handler/timeout leak on assertTaskCapability throw
- felixweinberger's architectural feedback (direct composition instead of generic ProtocolModule, NullTaskManager approach) has been incorporated
- All inline review comments are marked as resolved
- Tests were updated extensively, though some streaming tests now declare tasks capability that may not strictly be needed (minor test quality concern)
- The changeset documents the breaking changes but a human should verify completeness against the actual API surface changes
…lity Restores the assertTaskHandlerCapability check that was inadvertently skipped when the server has no tasks capability configured. On main, this check ran unconditionally in Protocol._onrequest; after extraction it moved behind the validateInbound callback which NullTaskManager wasn't providing.
…r-on-capability
Resolves conflicts in experimental/tasks/{client,server}.ts by
combining main's non-generic getTaskResult signature (hardcoded
GetTaskPayloadResultSchema) with this PR's _module call pattern
(no unsafe casts).
isTaskAugmentedRequestParams returns true for any valid params since the task field is optional in the schema. Check request.params.task presence to correctly detect task-augmented requests.
|
@claude review |
There was a problem hiding this comment.
No new bugs found and all five previously-reported issues appear resolved. However, this is a 19-file architectural refactor extracting ~500 lines from Protocol into TaskManager with breaking API changes — it needs human sign-off on the design decisions (NullTaskManager pattern, direct composition vs modules, capability type structure).
Extended reasoning...
Overview
This PR extracts task orchestration (creation, polling, queuing, routing) from the monolithic Protocol class (~1800 lines) into a dedicated TaskManager class with a NullTaskManager no-op variant. It touches 19 files across core, client, server, and integration test packages. The breaking changes move taskStore, taskMessageQueue, defaultTaskPollInterval, and maxTaskQueueSize from ProtocolOptions to capabilities.tasks.
Security risks
No security concerns identified. The refactor preserves the same trust boundaries — task handlers still validate via Zod schemas (now added to registerHandler), and capability assertions are delegated to the same abstract methods on Protocol subclasses.
Level of scrutiny
This requires high scrutiny. It restructures how Protocol integrates with task management, introduces a new TaskManagerHost interface, and changes the public API surface for both Client and Server constructors. The NullTaskManager pattern (suggested by felixweinberger referencing PR #1449) is a key architectural decision. The ProtocolModule abstraction was dropped in favor of direct composition per reviewer feedback — these are design-level choices that warrant human judgment.
Other factors
All five bugs from the prior mcp-claude[bot] review have been addressed in the current code:
ClientTasksCapabilityWithRuntimenow uses& TaskManagerOptions(full intersection, notPick)sendRequestwrapping inprocessInboundRequestnow gates onrelatedTaskId(matchingsendNotification)_modulegetter no longer throws — returnstaskManagerwhich is always available viaNullTaskManagerregisterHandlernow applies Zod schema validation before delegating to handlersprocessOutboundRequestis wrapped in try/catch with proper handler/timeout cleanup
The test suite updates (adding capabilities: { tasks: {...} } to many tests) reflect the intentional requirement that streaming APIs go through TaskManager, which is valid given the NullTaskManager fallback handles non-task streaming correctly.
|
@claude review |
There was a problem hiding this comment.
All five bugs from my March 17 review have been addressed, and the architecture was updated to direct composition per feedback. No new bugs found. However, this is a 19-file refactoring of core protocol infrastructure with breaking API changes and a new 900-line TaskManager -- it warrants a human sign-off on the architecture and migration surface.
Extended reasoning...
Overview
This PR extracts ~900 lines of task orchestration logic from Protocol into a dedicated TaskManager class. It touches 19 files across core, client, server, and integration tests. The key architectural change is that Protocol now holds a TaskManager (or NullTaskManager) directly, delegating task lifecycle operations through a TaskManagerHost interface. Configuration moved from ProtocolOptions to capabilities.tasks on ClientOptions/ServerOptions.
Bug fix verification
All five issues I identified on March 17 are resolved:
defaultTaskPollInterval/maxTaskQueueSize: Now included inClientTasksCapabilityWithRuntimeandServerTasksCapabilityWithRuntimevia& TaskManagerOptions, and correctly extracted byextractTaskManagerOptions()(taskManager.ts:207-211).sendRequestasymmetry:processInboundRequest(taskManager.ts:724) now gates onrelatedTaskIdfirst, matchingsendNotificationbehavior. Falls back to taskContext wrapping when no relatedTaskId.- Streaming regression:
ExperimentalClientTasks._module(client.ts:55) now returnsthis._client.taskManagerwhich is always available (NullTaskManager when unconfigured).NullTaskManagerinheritsrequestStream()which handles the non-task fallback path. - Zod validation:
registerHandler(protocol.ts:377-382) now callsschema.parse(request)before delegating to the handler. - Handler/timeout leak:
_requestWithSchema(protocol.ts:888-903) now wrapsprocessOutboundRequestin try/catch that cleans up_responseHandlers,_progressHandlers, and timeout on synchronous throw.
Security risks
No new security risks identified. The Zod validation fix actually improves input validation for task handlers. The registerHandler now validates incoming requests the same way setRequestHandler does.
Level of scrutiny
This PR requires high scrutiny:
- Core infrastructure: Changes to
Protocol._requestWithSchema,_onrequest,_onresponse, andnotificationaffect every request/response/notification in the SDK. - Breaking changes: Configuration migration from
ProtocolOptionstocapabilities.tasksaffects all existing task users. - New 900-line file:
taskManager.tscontains the full task lifecycle implementation and needs careful review for correctness of state management, message queuing, and polling logic. - Architecture decision: Direct composition vs module interface was updated per reviewer feedback, but should be confirmed.
Other factors
- The
NullTaskManager.processOutboundNotificationdropsrelatedTaskmetadata fromoptions(line 890-894), but this is benign since no relatedTask metadata should flow through when tasks are not configured. - Extensive test updates (10+ test files) provide good coverage of the migration.
- The changeset correctly documents all four breaking changes.
- This is based on PR #1449 and the reviewer has provided substantive architectural feedback that appears to have been incorporated.
- protocol.ts imports: keep modelcontextprotocol#1673's reduced list, apply this PR's ../types/index.js path - taskManager.ts: update import paths to ../types/index.js - exports/public/index.ts: move RequestTaskStore, TaskContext, TaskRequestOptions from protocol.js to taskManager.js, add TaskManagerOptions export
2 participants
Based on #1449
Extract all task orchestration logic (creation, polling, queuing, routing) from the monolithic
Protocolclass into a dedicatedTaskManagerclass that implements a newProtocolModuleinterface.TaskManageris only instantiated whencapabilities.tasksis declared, keeping Protocol lean for non-task use cases.Motivation and Context
The
Protocolclass had grown to ~1800 lines, with roughly half dedicated to task management. This made it difficult to reason about, test, and extend independently. Extracting tasks intoTaskManagerachieves:ProtocolModuleinterface allows future modules to hook into request/response/notification lifecycle without modifying Protocolextra/ctxtask fields are grouped under a singletaskobject instead of scattered top-level propertiesHow Has This Been Tested?
pnpm test:all)pnpm typecheck:allpasses (except a pre-existing unrelated type error inexamples/client)experimental.tasks.*APIsInMemoryTaskStoreon both client and serverBreaking Changes
ProtocolOptions.taskStorecapabilities.tasks.taskStore(onClientOptions/ServerOptions)ProtocolOptions.taskMessageQueuecapabilities.tasks.taskMessageQueue(onClientOptions/ServerOptions)Protocol.assertTaskCapability()(abstract)TaskManagerOptionsProtocol.assertTaskHandlerCapability()(abstract)TaskManagerOptionsTypes of changes
Checklist
Additional context
New files:
packages/core/src/shared/taskManager.ts—TaskManagerclass implementingProtocolModulepackages/core/src/shared/protocolModule.ts—ProtocolModuleinterface and lifecycle typesArchitecture: Protocol now maintains a
_modulesarray. During request/response/notification processing, it delegates to each registered module in order. TaskManager is the first (and currently only) module. The interface is designed so additional modules can be added without further changes to Protocol.Test fixes: Integration tests for
createMessageStreamandelicitInputStreamwere updated to declaretaskscapability on the server, sinceexperimental.tasks.*methods correctly require it. ThecreateMessageStreamtask-related tests were also restructured with sharedbeforeEach/afterEachto eliminate repeated inline server creation.