Skip to content

Commit 5ed7ad0

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 0e8ac6a commit 5ed7ad0

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
@@ -315,14 +315,9 @@ export abstract class Protocol<SendRequestT extends Request, SendNotificationT e
315315

316316
this.setRequestHandler('tasks/result', async (request, extra) => {
317317
const capturedTransport = this._transport;
318-
const result = await tm.handleGetTaskPayload(
319-
request.params.taskId,
320-
extra.sessionId,
321-
extra.signal,
322-
async message => {
323-
await capturedTransport?.send(message, { relatedRequestId: extra.requestId });
324-
}
325-
);
318+
const result = await tm.handleGetTaskPayload(request.params.taskId, extra.sessionId, extra.signal, async message => {
319+
await capturedTransport?.send(message, { relatedRequestId: extra.requestId });
320+
});
326321
return result as unknown as SendResultT;
327322
});
328323

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
@@ -37,8 +37,6 @@ describe('Protocol transport handling bug', () => {
3737
protected assertCapabilityForMethod(): void {}
3838
protected assertNotificationCapability(): void {}
3939
protected assertRequestHandlerCapability(): void {}
40-
protected assertTaskCapability(): void {}
41-
protected assertTaskHandlerCapability(): void {}
4240
})();
4341

4442
transportA = new MockTransport('A');

0 commit comments

Comments
 (0)