Skip to content

Commit 34423d9

Browse files
fix: address code review findings
- Add await to _clearTaskQueue calls in handleGetTaskPayload and handleCancelTask (was fire-and-forget, errors became unhandled rejections) - Remove unused relatedRequestId parameter from sendOnResponseStream callback (Protocol already captures extra.requestId in closure) - Remove dead assertTaskCapability/assertTaskHandlerCapability overrides from protocolTransportHandling.test.ts
1 parent 45d7e10 commit 34423d9

File tree

3 files changed

+7
-17
lines changed

3 files changed

+7
-17
lines changed

packages/core/src/shared/protocol.ts

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -317,14 +317,9 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
317317

318318
this.setRequestHandler(GetTaskPayloadRequestSchema, async (request, extra) => {
319319
const capturedTransport = this._transport;
320-
const result = await tm.handleGetTaskPayload(
321-
request.params.taskId,
322-
extra.sessionId,
323-
extra.signal,
324-
async (message, _relatedRequestId) => {
325-
await capturedTransport?.send(message, { relatedRequestId: extra.requestId });
326-
}
327-
);
320+
const result = await tm.handleGetTaskPayload(request.params.taskId, extra.sessionId, extra.signal, async message => {
321+
await capturedTransport?.send(message, { relatedRequestId: extra.requestId });
322+
});
328323
return result as unknown as SendResultT;
329324
});
330325

packages/core/src/shared/taskManager.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ export class TaskManager {
282282
taskId: string,
283283
sessionId: string | undefined,
284284
signal: AbortSignal,
285-
sendOnResponseStream: (message: JSONRPCNotification | JSONRPCRequest, relatedRequestId: RequestId) => Promise<void>
285+
sendOnResponseStream: (message: JSONRPCNotification | JSONRPCRequest) => Promise<void>
286286
): Promise<Result> {
287287
const handleTaskResult = async (): Promise<Result> => {
288288
// Deliver queued messages
@@ -310,10 +310,7 @@ export class TaskManager {
310310
}
311311

312312
// Send non-response messages on the response stream
313-
await sendOnResponseStream(
314-
queuedMessage.message as JSONRPCNotification | JSONRPCRequest,
315-
0 as RequestId // placeholder, Protocol will use the actual requestId
316-
);
313+
await sendOnResponseStream(queuedMessage.message as JSONRPCNotification | JSONRPCRequest);
317314
}
318315
}
319316

@@ -328,7 +325,7 @@ export class TaskManager {
328325
}
329326

330327
const result = await this._requireTaskStore.getTaskResult(taskId, sessionId);
331-
this._clearTaskQueue(taskId);
328+
await this._clearTaskQueue(taskId);
332329

333330
return {
334331
...result,
@@ -366,7 +363,7 @@ export class TaskManager {
366363
}
367364

368365
await this._requireTaskStore.updateTaskStatus(taskId, 'cancelled', 'Client cancelled task execution.', sessionId);
369-
this._clearTaskQueue(taskId);
366+
await this._clearTaskQueue(taskId);
370367

371368
const cancelledTask = await this._requireTaskStore.getTask(taskId, sessionId);
372369
if (!cancelledTask) {

packages/core/test/shared/protocolTransportHandling.test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ describe('Protocol transport handling bug', () => {
3838
protected assertCapabilityForMethod(): void {}
3939
protected assertNotificationCapability(): void {}
4040
protected assertRequestHandlerCapability(): void {}
41-
protected assertTaskCapability(): void {}
42-
protected assertTaskHandlerCapability(): void {}
4341
})();
4442

4543
transportA = new MockTransport('A');

0 commit comments

Comments
 (0)