From e72a3613958299adcb656da76c0d916e9e99393b Mon Sep 17 00:00:00 2001 From: poshinchen Date: Tue, 6 Oct 2026 11:16:08 -0400 Subject: [PATCH 1/2] fix(mcp/ts): forward the call signal to connect and fetch task metadata for unlisted tools Two follow-ups from the #4711 review: - Without tasksConfig, callTool now forwards the caller's signal to connect(), so aborting works during a stalled transport handshake instead of only after the connection completes. - Legacy task support is read from tools/list metadata. A tool invoked through a client that has not listed tools yet now fetches that metadata once, so taskSupport-required tools execute as tasks instead of failing with a plain tools/call. A failed listing logs a warning and keeps the plain-call behavior. --- strands-ts/src/mcp/__tests__/client.test.ts | 14 ++++++++++++ .../src/mcp/__tests__/task-client.test.ts | 18 +++++++++++++++ strands-ts/src/mcp/client.ts | 22 ++++++++++++++++++- 3 files changed, 53 insertions(+), 1 deletion(-) diff --git a/strands-ts/src/mcp/__tests__/client.test.ts b/strands-ts/src/mcp/__tests__/client.test.ts index cc588be75f..be8a21d68a 100644 --- a/strands-ts/src/mcp/__tests__/client.test.ts +++ b/strands-ts/src/mcp/__tests__/client.test.ts @@ -281,6 +281,20 @@ describe('MCP Integration', () => { expect(sdkClientMock.connect).not.toHaveBeenCalled() }) + it('stops waiting for the connection when callTool is aborted without tasksConfig', async () => { + sdkClientMock.connect.mockReturnValue(new Promise(() => {})) + const tool = new McpTool({ name: 'calc', description: '', inputSchema: {}, client }) + const controller = new AbortController() + const reason = new Error('caller aborted during connect') + + const pending = client.callTool(tool, {}, { signal: controller.signal }) + const rejection = expect(pending).rejects.toBe(reason) + controller.abort(reason) + + await rejection + expect(sdkClientMock.callTool).not.toHaveBeenCalled() + }) + it('converts SDK tool specs to McpTool instances', async () => { const longName = 'a'.repeat(65) const outputSchema = { diff --git a/strands-ts/src/mcp/__tests__/task-client.test.ts b/strands-ts/src/mcp/__tests__/task-client.test.ts index 93c8b71cf7..288323cfd8 100644 --- a/strands-ts/src/mcp/__tests__/task-client.test.ts +++ b/strands-ts/src/mcp/__tests__/task-client.test.ts @@ -266,6 +266,24 @@ describe('McpClient legacy task execution', () => { expect(server.requests('tasks/result')).toHaveLength(0) }) + it('fetches task metadata for a tool invoked before any listTools call', async () => { + const { client, server, tool } = await createHarness({ + era: 'legacy', + capabilities: { tools: {}, tasks: { requests: { tools: { call: {} } }, cancel: {} } }, + }) + server.handle('tools/list', () => ({ + tools: [{ name: 'task_tool', inputSchema: { type: 'object' }, execution: { taskSupport: 'required' } }], + })) + server.handle('tools/call', () => ({ task: legacyTask('completed') })) + server.handle('tasks/result', () => ({ content: [{ type: 'text', text: 'unlisted tool result' }] })) + + await expect(client.callTool(tool, {})).resolves.toEqual({ + content: [{ type: 'text', text: 'unlisted tool result' }], + }) + expect(server.requests('tools/list')).toHaveLength(1) + expect(requestParams(server.requests('tools/call')[0]!)).toMatchObject({ task: {} }) + }) + it('waits for queued input on tasks/result beyond the request timeout without progress', async () => { vi.useFakeTimers() const { client, server, tool } = await legacyHarness({ requestTimeout: 100, pollTimeout: 5_000 }) diff --git a/strands-ts/src/mcp/client.ts b/strands-ts/src/mcp/client.ts index 34a61f8724..b5b96eb88e 100644 --- a/strands-ts/src/mcp/client.ts +++ b/strands-ts/src/mcp/client.ts @@ -843,11 +843,18 @@ export class McpClient { operation?: TaskOperation, completeLegacyTask = false ): Promise { - await this.connect(false, operation ? { signal: operation.signal } : undefined) + const connectSignal = operation?.signal ?? options.signal + await this.connect(false, connectSignal ? { signal: connectSignal } : undefined) if (this._state === 'failed') throw new Error('MCP server failed to connect. Call connect(true) to retry.') const params = this._prepareToolCall(tool, args) + // Task support is read from tools/list metadata, so a tool invoked before any listing + // fetches it once. + if (completeLegacyTask && this._client.getNegotiatedProtocolVersion() === '2025-11-25') { + await this._ensureServerToolDefinitions(operation?.signal) + } + // The upstream codec rejects extension result types before custom result schemas run. if (completeLegacyTask && this._supportsLegacyTask(params.name)) { const outputSchema = compileToolOutputSchema( @@ -1107,6 +1114,19 @@ export class McpClient { } } + private async _ensureServerToolDefinitions(signal?: AbortSignal): Promise { + if (this._serverToolDefinitions.size > 0) return + try { + const result = await this._client.listTools(undefined, { + timeout: this._tasksConfig!.requestTimeoutMs, + ...(signal && { signal }), + }) + this._serverToolDefinitions = new Map(result.tools.map((tool) => [tool.name, toMcpServerToolDefinition(tool)])) + } catch (error) { + logger.warn(`error=<${error}> | failed to list tools for legacy task support detection`) + } + } + private async _callLegacyTask(params: CallToolRequest['params'], operation: TaskOperation): Promise { const requestOptions = (): CallToolRequestOptions => ({ signal: operation.signal, From 7ef2966537038b9f6b23f8e4da77e81673cd4af6 Mon Sep 17 00:00:00 2001 From: poshinchen Date: Mon, 12 Oct 2026 10:02:46 -0400 Subject: [PATCH 2/2] fix(mcp/ts): make the task metadata fetch once-per-client and capability-gated Review follow-ups on the unlisted-tool metadata fetch: - Track listing with a flag instead of a non-empty cache, so an empty tools/list result or a failed listing no longer refetches and rewarns on every call; listTools() sets the same flag. - Gate the fetch on the server's legacy tasks capability in addition to the protocol version, so ordinary 2025-11-25 servers pay no extra round-trip. - Bound the fetch by the remaining operation deadline like every other legacy task request, and rethrow when the operation was aborted so a user cancel surfaces as cancellation instead of a listing warning followed by a plain call. --- .../src/mcp/__tests__/task-client.test.ts | 45 +++++++++++++++++++ strands-ts/src/mcp/client.ts | 22 ++++++--- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/strands-ts/src/mcp/__tests__/task-client.test.ts b/strands-ts/src/mcp/__tests__/task-client.test.ts index 288323cfd8..4630e032fb 100644 --- a/strands-ts/src/mcp/__tests__/task-client.test.ts +++ b/strands-ts/src/mcp/__tests__/task-client.test.ts @@ -284,6 +284,51 @@ describe('McpClient legacy task execution', () => { expect(requestParams(server.requests('tools/call')[0]!)).toMatchObject({ task: {} }) }) + it('lists tools once even when the listing is empty or the tool is unknown', async () => { + const { client, server, tool } = await createHarness({ + era: 'legacy', + capabilities: { tools: {}, tasks: { requests: { tools: { call: {} } }, cancel: {} } }, + }) + server.handle('tools/list', () => ({ tools: [] })) + server.handle('tools/call', () => ({ content: [{ type: 'text', text: 'plain' }] })) + + await client.callTool(tool, {}) + await client.callTool(tool, {}) + await client.callTool(tool, {}) + + expect(server.requests('tools/list')).toHaveLength(1) + expect(server.requests('tools/call')).toHaveLength(3) + }) + + it('skips the metadata fetch when the server does not advertise legacy tasks', async () => { + const { client, server, tool } = await createHarness({ + era: 'legacy', + capabilities: { tools: {} }, + }) + server.handle('tools/call', () => ({ content: [{ type: 'text', text: 'plain' }] })) + + await expect(client.callTool(tool, {})).resolves.toEqual({ content: [{ type: 'text', text: 'plain' }] }) + expect(server.requests('tools/list')).toHaveLength(0) + }) + + it('surfaces an abort during the metadata fetch instead of calling the tool', async () => { + const { client, server, tool } = await createHarness({ + era: 'legacy', + capabilities: { tools: {}, tasks: { requests: { tools: { call: {} } }, cancel: {} } }, + }) + server.handle('tools/list', () => NO_RESPONSE) + const controller = new AbortController() + const reason = new Error('aborted during metadata fetch') + + const pending = client.callTool(tool, {}, { signal: controller.signal }) + const rejection = expect(pending).rejects.toBe(reason) + await vi.waitFor(() => expect(server.requests('tools/list')).toHaveLength(1)) + controller.abort(reason) + + await rejection + expect(server.requests('tools/call')).toHaveLength(0) + }) + it('waits for queued input on tasks/result beyond the request timeout without progress', async () => { vi.useFakeTimers() const { client, server, tool } = await legacyHarness({ requestTimeout: 100, pollTimeout: 5_000 }) diff --git a/strands-ts/src/mcp/client.ts b/strands-ts/src/mcp/client.ts index b5b96eb88e..f349a59082 100644 --- a/strands-ts/src/mcp/client.ts +++ b/strands-ts/src/mcp/client.ts @@ -341,6 +341,8 @@ export class McpClient { /** Server-side name of each listed tool, which differs from `tool.name` when a prefix is set. */ private _serverToolNames = new WeakMap() private _serverToolDefinitions = new Map() + /** Whether tools/list ran for this client, independent of how many tools it returned. */ + private _serverToolsListed = false private _registeredToolNames = new Set() private _onToolsChanged: ((oldTools: string[], newTools: McpTool[]) => void) | undefined private _refreshingTools = false @@ -633,6 +635,7 @@ export class McpClient { } this._serverToolDefinitions = toolDefinitions + this._serverToolsListed = true // Per-call overrides are transient, so they must not become the baseline that a later // tools-changed refresh reports as the previously registered names. @@ -851,8 +854,12 @@ export class McpClient { // Task support is read from tools/list metadata, so a tool invoked before any listing // fetches it once. - if (completeLegacyTask && this._client.getNegotiatedProtocolVersion() === '2025-11-25') { - await this._ensureServerToolDefinitions(operation?.signal) + if ( + completeLegacyTask && + this._client.getNegotiatedProtocolVersion() === '2025-11-25' && + this._client.getServerCapabilities()?.tasks?.requests?.tools?.call !== undefined + ) { + await this._ensureServerToolDefinitions(operation!) } // The upstream codec rejects extension result types before custom result schemas run. @@ -1114,15 +1121,18 @@ export class McpClient { } } - private async _ensureServerToolDefinitions(signal?: AbortSignal): Promise { - if (this._serverToolDefinitions.size > 0) return + private async _ensureServerToolDefinitions(operation: TaskOperation): Promise { + if (this._serverToolsListed) return + this._serverToolsListed = true try { const result = await this._client.listTools(undefined, { - timeout: this._tasksConfig!.requestTimeoutMs, - ...(signal && { signal }), + timeout: remainingTime(operation.deadline, this._tasksConfig!.requestTimeoutMs), + signal: operation.signal, }) this._serverToolDefinitions = new Map(result.tools.map((tool) => [tool.name, toMcpServerToolDefinition(tool)])) } catch (error) { + // A cancelled or expired operation must surface as such, not as a listing failure. + if (operation.signal.aborted) throw error logger.warn(`error=<${error}> | failed to list tools for legacy task support detection`) } }