Skip to content

Commit b783fcd

Browse files
fix: address review feedback on TaskManager extraction
- Move assertTaskCapability/assertTaskHandlerCapability from mutable properties on TaskManager to TaskManagerOptions constructor params, eliminating fragile post-construction wiring - Add _requireHost getter with clear error message instead of _host! non-null assertions throughout TaskManager - Extract duplicated relatedTask metadata injection in Protocol.notification() into _buildJsonRpcNotification() helper - Add @internal JSDoc to TaskManagerHost (will be removed from public exports in PR #1447) - Add changeset for the breaking changes - Revert unrelated pnpm-workspace.yaml change
1 parent 5ed7ad0 commit b783fcd

File tree

5 files changed

+79
-66
lines changed

5 files changed

+79
-66
lines changed

.changeset/extract-task-manager.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
"@modelcontextprotocol/core": minor
3+
"@modelcontextprotocol/client": minor
4+
"@modelcontextprotocol/server": minor
5+
---
6+
7+
refactor: extract task orchestration from Protocol into TaskManager
8+
9+
**Breaking changes:**
10+
- `extra.taskId``extra.task?.taskId`
11+
- `extra.taskStore``extra.task?.taskStore`
12+
- `extra.taskRequestedTtl``extra.task?.requestedTtl`
13+
- `ProtocolOptions` no longer accepts `taskStore`/`taskMessageQueue` — pass via `TaskManagerOptions` in `ClientOptions`/`ServerOptions`
14+
- Abstract methods `assertTaskCapability`/`assertTaskHandlerCapability` removed from Protocol

packages/client/src/client/client.ts

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -265,23 +265,21 @@ export class Client<
265265
taskStore: options?.taskStore,
266266
taskMessageQueue: options?.taskMessageQueue,
267267
defaultTaskPollInterval: options?.defaultTaskPollInterval,
268-
maxTaskQueueSize: options?.maxTaskQueueSize
268+
maxTaskQueueSize: options?.maxTaskQueueSize,
269+
assertTaskCapability: (method: string) => {
270+
assertToolsCallTaskCapability(this._serverCapabilities?.tasks?.requests, method, 'Server');
271+
},
272+
assertTaskHandlerCapability: (method: string) => {
273+
if (!this._capabilities) return;
274+
assertClientRequestTaskCapability(this._capabilities.tasks?.requests, method, 'Client');
275+
}
269276
});
270277

271278
super(options, taskManager);
272279
this._capabilities = options?.capabilities ?? {};
273280
this._jsonSchemaValidator = options?.jsonSchemaValidator ?? new AjvJsonSchemaValidator();
274281
this._enforceStrictCapabilities = options?.enforceStrictCapabilities ?? false;
275282

276-
// Wire task capability assertions
277-
taskManager.assertTaskCapability = (method: string) => {
278-
assertToolsCallTaskCapability(this._serverCapabilities?.tasks?.requests, method, 'Server');
279-
};
280-
taskManager.assertTaskHandlerCapability = (method: string) => {
281-
if (!this._capabilities) return;
282-
assertClientRequestTaskCapability(this._capabilities.tasks?.requests, method, 'Client');
283-
};
284-
285283
// Store list changed config for setup after connection (when we know server capabilities)
286284
if (options?.listChanged) {
287285
this._pendingListChangedConfig = options.listChanged;

packages/core/src/shared/protocol.ts

Lines changed: 19 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -837,49 +837,37 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
837837
return;
838838
}
839839

840-
let jsonrpcNotification: JSONRPCNotification = {
841-
...notification,
842-
jsonrpc: '2.0'
843-
};
844-
845-
if (options?.relatedTask) {
846-
jsonrpcNotification = {
847-
...jsonrpcNotification,
848-
params: {
849-
...jsonrpcNotification.params,
850-
_meta: {
851-
...jsonrpcNotification.params?._meta,
852-
[RELATED_TASK_META_KEY]: options.relatedTask
853-
}
854-
}
855-
};
856-
}
857-
840+
const jsonrpcNotification = this._buildJsonRpcNotification(notification, options);
858841
this._transport?.send(jsonrpcNotification, options).catch(error => this._onerror(error));
859842
});
860843

861844
return;
862845
}
863846

864-
let jsonrpcNotification: JSONRPCNotification = {
847+
const jsonrpcNotification = this._buildJsonRpcNotification(notification, options);
848+
await this._transport.send(jsonrpcNotification, options);
849+
}
850+
851+
private _buildJsonRpcNotification(notification: SendNotificationT, options?: NotificationOptions): JSONRPCNotification {
852+
const jsonrpcNotification: JSONRPCNotification = {
865853
...notification,
866854
jsonrpc: '2.0'
867855
};
868856

869-
if (options?.relatedTask) {
870-
jsonrpcNotification = {
871-
...jsonrpcNotification,
872-
params: {
873-
...jsonrpcNotification.params,
874-
_meta: {
875-
...jsonrpcNotification.params?._meta,
876-
[RELATED_TASK_META_KEY]: options.relatedTask
877-
}
878-
}
879-
};
857+
if (!options?.relatedTask) {
858+
return jsonrpcNotification;
880859
}
881860

882-
await this._transport.send(jsonrpcNotification, options);
861+
return {
862+
...jsonrpcNotification,
863+
params: {
864+
...jsonrpcNotification.params,
865+
_meta: {
866+
...jsonrpcNotification.params?._meta,
867+
[RELATED_TASK_META_KEY]: options.relatedTask
868+
}
869+
}
870+
};
883871
}
884872

885873
/**

packages/core/src/shared/taskManager.ts

Lines changed: 30 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,16 @@ export type TaskManagerOptions = {
6363
* If undefined, the queue size is unbounded.
6464
*/
6565
maxTaskQueueSize?: number;
66+
/**
67+
* Checks if task creation is supported for a given request method.
68+
* Wired by Client/Server to enforce capability negotiation.
69+
*/
70+
assertTaskCapability?: (method: string) => void;
71+
/**
72+
* Checks if task handling is supported for a given request method.
73+
* Wired by Client/Server to enforce capability negotiation.
74+
*/
75+
assertTaskHandlerCapability?: (method: string) => void;
6676
};
6777

6878
/**
@@ -94,6 +104,8 @@ export interface TaskContext {
94104
/**
95105
* Internal interface that TaskManager needs from the Protocol host
96106
* for sending outbound requests and notifications.
107+
*
108+
* @internal Not intended for external use. Will be removed from public exports.
97109
*/
98110
export interface TaskManagerHost {
99111
request<T extends AnySchema>(request: Request, resultSchema: T, options?: RequestOptions): Promise<SchemaOutput<T>>;
@@ -136,6 +148,13 @@ export class TaskManager {
136148
this._host = host;
137149
}
138150

151+
private get _requireHost(): TaskManagerHost {
152+
if (!this._host) {
153+
throw new McpError(ErrorCode.InternalError, 'TaskManager is not bound to a Protocol host — call bind() first');
154+
}
155+
return this._host;
156+
}
157+
139158
get taskStore(): TaskStore | undefined {
140159
return this._taskStore;
141160
}
@@ -151,17 +170,13 @@ export class TaskManager {
151170
return this._taskMessageQueue;
152171
}
153172

154-
/**
155-
* Checks if task creation is supported for a given request method.
156-
* Set by Client/Server to wire capability checking.
157-
*/
158-
assertTaskCapability?: (method: string) => void;
173+
get assertTaskCapability(): ((method: string) => void) | undefined {
174+
return this._options.assertTaskCapability;
175+
}
159176

160-
/**
161-
* Checks if task handling is supported for a given request method.
162-
* Set by Client/Server to wire capability checking.
163-
*/
164-
assertTaskHandlerCapability?: (method: string) => void;
177+
get assertTaskHandlerCapability(): ((method: string) => void) | undefined {
178+
return this._options.assertTaskHandlerCapability;
179+
}
165180

166181
// =========================================================================
167182
// Public API methods (previously protected on Protocol)
@@ -176,7 +191,7 @@ export class TaskManager {
176191
resultSchema: T,
177192
options?: RequestOptions
178193
): AsyncGenerator<ResponseMessage<SchemaOutput<T>>, void, void> {
179-
const host = this._host!;
194+
const host = this._requireHost;
180195
const { task } = options ?? {};
181196

182197
// For non-task requests, just yield the result
@@ -247,23 +262,23 @@ export class TaskManager {
247262
}
248263

249264
async getTask(params: GetTaskRequest['params'], options?: RequestOptions): Promise<GetTaskResult> {
250-
return this._host!.request({ method: 'tasks/get', params }, GetTaskResultSchema, options);
265+
return this._requireHost.request({ method: 'tasks/get', params }, GetTaskResultSchema, options);
251266
}
252267

253268
async getTaskResult<T extends AnySchema>(
254269
params: GetTaskPayloadRequest['params'],
255270
resultSchema: T,
256271
options?: RequestOptions
257272
): Promise<SchemaOutput<T>> {
258-
return this._host!.request({ method: 'tasks/result', params }, resultSchema, options);
273+
return this._requireHost.request({ method: 'tasks/result', params }, resultSchema, options);
259274
}
260275

261276
async listTasks(params?: { cursor?: string }, options?: RequestOptions): Promise<SchemaOutput<typeof ListTasksResultSchema>> {
262-
return this._host!.request({ method: 'tasks/list', params }, ListTasksResultSchema, options);
277+
return this._requireHost.request({ method: 'tasks/list', params }, ListTasksResultSchema, options);
263278
}
264279

265280
async cancelTask(params: { taskId: string }, options?: RequestOptions): Promise<SchemaOutput<typeof CancelTaskResultSchema>> {
266-
return this._host!.request({ method: 'tasks/cancel', params }, CancelTaskResultSchema, options);
281+
return this._requireHost.request({ method: 'tasks/cancel', params }, CancelTaskResultSchema, options);
267282
}
268283

269284
// =========================================================================

packages/server/src/server/server.ts

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -159,23 +159,21 @@ export class Server<
159159
taskStore: options?.taskStore,
160160
taskMessageQueue: options?.taskMessageQueue,
161161
defaultTaskPollInterval: options?.defaultTaskPollInterval,
162-
maxTaskQueueSize: options?.maxTaskQueueSize
162+
maxTaskQueueSize: options?.maxTaskQueueSize,
163+
assertTaskCapability: (method: string) => {
164+
assertClientRequestTaskCapability(this._clientCapabilities?.tasks?.requests, method, 'Client');
165+
},
166+
assertTaskHandlerCapability: (method: string) => {
167+
if (!this._capabilities) return;
168+
assertToolsCallTaskCapability(this._capabilities.tasks?.requests, method, 'Server');
169+
}
163170
});
164171

165172
super(options, taskManager);
166173
this._capabilities = options?.capabilities ?? {};
167174
this._instructions = options?.instructions;
168175
this._jsonSchemaValidator = options?.jsonSchemaValidator ?? new AjvJsonSchemaValidator();
169176

170-
// Wire task capability assertions
171-
taskManager.assertTaskCapability = (method: string) => {
172-
assertClientRequestTaskCapability(this._clientCapabilities?.tasks?.requests, method, 'Client');
173-
};
174-
taskManager.assertTaskHandlerCapability = (method: string) => {
175-
if (!this._capabilities) return;
176-
assertToolsCallTaskCapability(this._capabilities.tasks?.requests, method, 'Server');
177-
};
178-
179177
this.setRequestHandler('initialize', request => this._oninitialize(request));
180178
this.setNotificationHandler('notifications/initialized', () => this.oninitialized?.());
181179

0 commit comments

Comments
 (0)