From d4951ccde7c7c14f7724d7b4aabd4a81236866c7 Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 19:05:01 +0000 Subject: [PATCH 1/9] fix(amazonq): reserve built-in tool names for MCP tool registration MCP tool names came from a server's tools/list and were used in their bare form whenever the name was not already in the collision set. That set was not seeded with the built-in tool names, so a server could register under one and replace the built-in entry. createNamespacedToolName now takes a reservedNames set, never returns a reserved name from any naming branch, and discards a persisted name mapping that points at one. Both call sites seed the set from agent.getBuiltInToolNames() so the reservation tracks tool registration. MCP cleanup paths skip built-in names. --- .../agenticChat/agenticChatController.ts | 9 +++++- .../agenticChat/tools/mcp/mcpUtils.test.ts | 26 +++++++++++++++++ .../agenticChat/tools/mcp/mcpUtils.ts | 28 +++++++++++++++---- .../agenticChat/tools/toolServer.ts | 17 ++++++++++- 4 files changed, 72 insertions(+), 8 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts index a4f42c7d33..44741bd494 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts @@ -4923,13 +4923,20 @@ export class AgenticChatController implements ChatHandlers { // TODO: mcp tool spec name will be server___tool. // TODO: Will also need to handle rare edge cases of long server name + long tool name > 64 char const allNamespacedTools = new Set() + const builtInToolNames = new Set(this.#features.agent.getBuiltInToolNames()) let mcpToolSpecNames: Set try { mcpToolSpecNames = new Set( McpManager.instance .getAllTools() .map(tool => - createNamespacedToolName(tool.serverName, tool.toolName, allNamespacedTools, tempMapping) + createNamespacedToolName( + tool.serverName, + tool.toolName, + allNamespacedTools, + tempMapping, + builtInToolNames + ) ) ) } catch (error) { diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts index 6ae532ed75..98e60e69e4 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts @@ -554,6 +554,32 @@ describe('createNamespacedToolName', () => { toolName: longTool, }) }) + + it('namespaces an MCP tool that collides with a reserved built-in name', () => { + const reserved = new Set(['fsRead', 'fsWrite', 'executeBash']) + const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, reserved) + expect(result).to.equal('evil___fsRead') + expect(tools.has('fsRead')).to.be.false + expect(toolNameMapping.has('fsRead')).to.be.false + expect(toolNameMapping.get('evil___fsRead')).to.deep.equal({ + serverName: 'evil', + toolName: 'fsRead', + }) + }) + + it('does not reuse a stale mapping that points at a reserved built-in name', () => { + const reserved = new Set(['fsRead']) + toolNameMapping.set('fsRead', { serverName: 'evil', toolName: 'fsRead' }) + const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, reserved) + expect(result).to.equal('evil___fsRead') + expect(toolNameMapping.has('fsRead')).to.be.false + }) + + it('still prefers the bare tool name when it is not reserved', () => { + const reserved = new Set(['fsRead']) + const result = createNamespacedToolName('github', 'create_issue', tools, toolNameMapping, reserved) + expect(result).to.equal('create_issue') + }) }) describe('normalizePathFromUri', () => { diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts index 6945542078..4e567ddf2a 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts @@ -1269,17 +1269,28 @@ export function findServerInRegistry(registry: McpRegistryData, serverName: stri * Create a namespaced tool name from server and tool names. * Handles truncation and conflicts according to specific rules. * Also stores the mapping from namespaced name back to original names. + * + * `reservedNames` holds names an MCP tool must never occupy (the built-in tool + * names). Without it an MCP server could advertise e.g. `fsRead` and win the + * bare name, so the request would be routed through the built-in permission + * branch instead of the MCP one. */ export function createNamespacedToolName( serverName: string, toolName: string, allNamespacedTools: Set, - toolNameMapping: Map + toolNameMapping: Map, + reservedNames: Set = new Set() ): string { // First, check if this server/tool combination already has a mapping // If it does, reuse that name to maintain consistency across reinitializations for (const [existingName, mapping] of toolNameMapping.entries()) { if (mapping.serverName === serverName && mapping.toolName === toolName) { + // Never reuse a stale mapping that points at a reserved built-in name. + if (reservedNames.has(existingName)) { + toolNameMapping.delete(existingName) + break + } // If the name is already in the set, it's already registered // If not, add it to the set if (!allNamespacedTools.has(existingName)) { @@ -1292,8 +1303,13 @@ export function createNamespacedToolName( // Sanitize the tool name const sanitizedToolName = sanitizeName(toolName) - // First try to use just the tool name if it's not already in use and fits within length limit - if (sanitizedToolName.length <= MAX_TOOL_NAME_LENGTH && !allNamespacedTools.has(sanitizedToolName)) { + // First try to use just the tool name if it's not already in use, is not a + // reserved built-in name, and fits within length limit + if ( + sanitizedToolName.length <= MAX_TOOL_NAME_LENGTH && + !allNamespacedTools.has(sanitizedToolName) && + !reservedNames.has(sanitizedToolName) + ) { allNamespacedTools.add(sanitizedToolName) toolNameMapping.set(sanitizedToolName, { serverName, toolName }) return sanitizedToolName @@ -1304,7 +1320,7 @@ export function createNamespacedToolName( const fullName = `${serverName}${sep}${sanitizedToolName}` // If the full name fits and is unique, use it - if (fullName.length <= MAX_TOOL_NAME_LENGTH && !allNamespacedTools.has(fullName)) { + if (fullName.length <= MAX_TOOL_NAME_LENGTH && !allNamespacedTools.has(fullName) && !reservedNames.has(fullName)) { allNamespacedTools.add(fullName) toolNameMapping.set(fullName, { serverName, toolName }) return fullName @@ -1317,7 +1333,7 @@ export function createNamespacedToolName( const truncatedServer = serverName.substring(0, maxServerLength) const namespacedName = `${truncatedServer}${sep}${sanitizedToolName}` - if (!allNamespacedTools.has(namespacedName)) { + if (!allNamespacedTools.has(namespacedName) && !reservedNames.has(namespacedName)) { allNamespacedTools.add(namespacedName) toolNameMapping.set(namespacedName, { serverName, toolName }) return namespacedName @@ -1345,7 +1361,7 @@ export function createNamespacedToolName( candidateName = `${truncatedTool}${suffix}` } - if (!allNamespacedTools.has(candidateName)) { + if (!allNamespacedTools.has(candidateName) && !reservedNames.has(candidateName)) { allNamespacedTools.add(candidateName) toolNameMapping.set(candidateName, { serverName, toolName }) return candidateName diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts index bad50794e4..cea43ceeec 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts @@ -282,8 +282,14 @@ export const McpToolsServer: Server = ({ function removeAllMcpTools(): void { logging.info('Removing all MCP tools due to admin configuration') + const builtInToolNames = new Set(agent.getBuiltInToolNames()) for (const [server, toolNames] of Object.entries(registered)) { for (const name of toolNames) { + // Never unregister a built-in tool while cleaning up MCP tools. + if (builtInToolNames.has(name)) { + logging.warn(`MCP: refusing to remove built-in tool name ${name}`) + continue + } agent.removeTool(name) allNamespacedTools.delete(name) logging.info(`MCP: removed tool ${name}`) @@ -315,8 +321,16 @@ export const McpToolsServer: Server = ({ } function registerServerTools(server: string, defs: McpToolDefinition[]) { + // Built-in tool names are reserved: an MCP tool must never register under + // one, and cleanup must never unregister one. + const builtInToolNames = new Set(agent.getBuiltInToolNames()) + // 1) remove old tools for (const name of registered[server] ?? []) { + if (builtInToolNames.has(name)) { + logging.warn(`MCP: refusing to remove built-in tool name ${name}`) + continue + } agent.removeTool(name) allNamespacedTools.delete(name) } @@ -339,7 +353,8 @@ export const McpToolsServer: Server = ({ def.serverName, def.toolName, allNamespacedTools, - toolNameMapping + toolNameMapping, + builtInToolNames ) const tool = new McpTool({ logging, workspace, lsp }, def) From c6c42a77a56c5103f9333a1140b960e22c353e13 Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 19:28:22 +0000 Subject: [PATCH 2/9] fix(amazonq): dispatch tool confirmation on the registered tool name processToolConfirmation resolved its switch subject as toolType || toolUse.name. The MCP branch passes the server's original tool name as toolType for display, so a tool advertised as fsRead selected the built-in fsRead card, which reads input.paths and threw "Paths array cannot be empty." before any approval card was shown. The same name also drove the isStandardTool classification and the executeBash body check. Dispatch and classification now use toolUse.name, the registered name, which is namespaced for MCP tools. toolType stays display-only. --- .../agenticChat/agenticChatController.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts index 44741bd494..04069befd2 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts @@ -3079,6 +3079,11 @@ export class AgenticChatController implements ChatHandlers { builtInPermission?: boolean, acceptanceReason?: CommandValidation['acceptanceReason'] ): ChatResult { + // The name used for dispatch and classification is the registered tool + // name, which is namespaced for MCP tools. `toolType` carries the + // server-supplied original name and is display-only, so it must not be + // able to select built-in rendering by matching a built-in name. + const dispatchName = toolUse.name const toolName = toolType || toolUse.name // A multiply linked path is inside the workspace, so the filesystem // prompts below describe the shared file rather than a location outside @@ -3101,7 +3106,7 @@ export class AgenticChatController implements ChatHandlers { let body: string | undefined // Configure tool-specific UI elements - switch (toolName) { + switch (dispatchName) { case EXECUTE_BASH: { const commandString = (toolUse.input as unknown as ExecuteBashParams).command // get feature flag @@ -3222,7 +3227,7 @@ export class AgenticChatController implements ChatHandlers { buttons, } - if (toolName === FS_READ) { + if (dispatchName === FS_READ) { const paths = (toolUse.input as unknown as FsReadParams).paths // Validate paths using our synchronous utility @@ -3264,7 +3269,7 @@ export class AgenticChatController implements ChatHandlers { } // Determine if this is a built-in tool or MCP tool - const isStandardTool = toolName !== undefined && this.#features.agent.getBuiltInToolNames().includes(toolName) + const isStandardTool = this.#features.agent.getBuiltInToolNames().includes(dispatchName) if (isStandardTool) { return { @@ -3273,7 +3278,7 @@ export class AgenticChatController implements ChatHandlers { header, // The warning explains why acceptance is needed, so show it above // the body rather than using it only as a spacing flag. - body: warning ? (toolName === EXECUTE_BASH ? '' : warning + '\n\n') + body : body, + body: warning ? (dispatchName === EXECUTE_BASH ? '' : warning + '\n\n') + body : body, } } else { return { From 63fd1b159edb027f137ccc7b58c4e82daf33cc6b Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 19:40:36 +0000 Subject: [PATCH 3/9] fix(amazonq): guard optional tool name in confirmation classification ToolUse.name is string | undefined, so the isStandardTool check needs the undefined guard that the previous expression carried. Restores it on the dispatch name. An undefined name classifies as non-built-in. --- .../src/language-server/agenticChat/agenticChatController.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts index 04069befd2..64ff0bdf55 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts @@ -3269,7 +3269,8 @@ export class AgenticChatController implements ChatHandlers { } // Determine if this is a built-in tool or MCP tool - const isStandardTool = this.#features.agent.getBuiltInToolNames().includes(dispatchName) + const isStandardTool = + dispatchName !== undefined && this.#features.agent.getBuiltInToolNames().includes(dispatchName) if (isStandardTool) { return { From d94de06e2e0f6c4013645e359d88ce57150636ce Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 19:48:38 +0000 Subject: [PATCH 4/9] test(amazonq): cover reserved built-in names in MCP tool naming Adds cases for every built-in name, two servers advertising the same built-in name, re-registration reusing the namespaced name, truncation of a long server name, the numeric-suffix fallback skipping a reserved candidate, and the default empty reserved set keeping prior behavior. --- .../agenticChat/tools/mcp/mcpUtils.test.ts | 61 +++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts index 98e60e69e4..649ff9af17 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts @@ -28,6 +28,15 @@ import { convertPersonaToAgent, migrateToAgentConfig, } from './mcpUtils' +import { + EXECUTE_BASH, + FILE_SEARCH, + FS_READ, + FS_REPLACE, + FS_WRITE, + GREP_SEARCH, + LIST_DIRECTORY, +} from '../../constants/toolConstants' import type { MCPServerConfig } from './mcpTypes' import { McpPermissionType } from './mcpTypes' import { pathToFileURL } from 'url' @@ -580,6 +589,58 @@ describe('createNamespacedToolName', () => { const result = createNamespacedToolName('github', 'create_issue', tools, toolNameMapping, reserved) expect(result).to.equal('create_issue') }) + + it('refuses every built-in tool name', () => { + const builtIns = [FS_READ, FS_WRITE, FS_REPLACE, LIST_DIRECTORY, GREP_SEARCH, FILE_SEARCH, EXECUTE_BASH] + const reserved = new Set(builtIns) + + for (const builtIn of builtIns) { + const result = createNamespacedToolName('evil', builtIn, tools, toolNameMapping, reserved) + expect(result, `${builtIn} must not be claimed`).to.equal(`evil___${builtIn}`) + expect(tools.has(builtIn), `${builtIn} must stay unclaimed`).to.be.false + } + }) + + it('gives two servers advertising the same built-in name distinct namespaced names', () => { + const reserved = new Set(['fsRead']) + const first = createNamespacedToolName('alpha', 'fsRead', tools, toolNameMapping, reserved) + const second = createNamespacedToolName('beta', 'fsRead', tools, toolNameMapping, reserved) + expect(first).to.equal('alpha___fsRead') + expect(second).to.equal('beta___fsRead') + expect(tools.has('fsRead')).to.be.false + }) + + it('reuses the namespaced name when the same server tool is seen again', () => { + const reserved = new Set(['fsRead']) + const first = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, reserved) + const second = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, reserved) + expect(second).to.equal(first) + expect(toolNameMapping.size).to.equal(1) + }) + + it('keeps a truncated namespaced name out of the reserved set', () => { + const longServer = 'very_long_server_name_that_definitely_exceeds_the_maximum_allowed_length' + const reserved = new Set(['fsRead']) + const result = createNamespacedToolName(longServer, 'fsRead', tools, toolNameMapping, reserved) + expect(result.length).to.be.lessThanOrEqual(MAX_TOOL_NAME_LENGTH) + expect(result.endsWith('___fsRead')).to.be.true + expect(reserved.has(result)).to.be.false + }) + + it('does not fall back onto a reserved name via the numeric suffix path', () => { + // Force the namespaced form to be unavailable so the suffix path runs. + tools.add('evil___fsRead') + const reserved = new Set(['fsRead', 'fsRead1']) + const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, reserved) + expect(reserved.has(result)).to.be.false + expect(result).to.not.equal('fsRead') + expect(result).to.not.equal('fsRead1') + }) + + it('leaves behavior unchanged when no reserved names are supplied', () => { + const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping) + expect(result).to.equal('fsRead') + }) }) describe('normalizePathFromUri', () => { From b8ddf9ebfeb5e77d6bfac6c52bb657062242cf69 Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 19:56:05 +0000 Subject: [PATCH 5/9] test(amazonq): cover sanitized names that collapse onto a built-in name sanitizeName strips disallowed characters, so an advertised name such as "fs Read" becomes "fsRead". The reserved check runs on the sanitized form; this pins that behavior. --- .../agenticChat/tools/mcp/mcpUtils.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts index 649ff9af17..6806e8f4b3 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts @@ -637,6 +637,20 @@ describe('createNamespacedToolName', () => { expect(result).to.not.equal('fsRead1') }) + it('refuses a name that only becomes a built-in name after sanitization', () => { + const reserved = new Set(['fsRead']) + + // sanitizeName strips disallowed characters, so these all collapse to + // 'fsRead' and must not be allowed to claim it. + for (const advertised of ['fs Read', 'fs/Read', 'fsRead!', 'fs.Read']) { + const localTools = new Set() + const localMapping = new Map() + const result = createNamespacedToolName('evil', advertised, localTools, localMapping, reserved) + expect(result, `${advertised} must not claim fsRead`).to.equal('evil___fsRead') + expect(localTools.has('fsRead'), `${advertised} must leave fsRead unclaimed`).to.be.false + } + }) + it('leaves behavior unchanged when no reserved names are supplied', () => { const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping) expect(result).to.equal('fsRead') From ddd3e7722bd8e258ed414a76a7fdf5278642eefc Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 20:56:01 +0000 Subject: [PATCH 6/9] fix(amazonq): key remaining tool-name branches on the registered name getUpdateToolConfirmResult resolved its subject as originalToolName, which is the server's original tool name on the MCP approval path. A tool advertised as executeBash therefore rendered the shell card, which reads input.command, and one advertised as a filesystem tool selected those cases. Dispatch now uses toolUse.name; originalToolName and toolType remain display-only. The built-in path already passes toolUse.name, so built-in rendering is unchanged. getMessageIdForToolUse no longer branches on toolType, so a server cannot steer a card to the built-in executeBash message id. The parameter is now unused and is removed. The Built-in tool list in mcpEventHandler selected tools by excluding raw MCP tool names, which hid a built-in whose name an MCP server also advertises. It now selects by getBuiltInToolNames(). --- .../agenticChat/agenticChatController.ts | 24 ++++++++++++------- .../agenticChat/tools/mcp/mcpEventHandler.ts | 13 +++++----- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts index 64ff0bdf55..096b4085f1 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts @@ -316,12 +316,13 @@ export class AgenticChatController implements ChatHandlers { * @param toolUse The tool use object * @returns The message ID to use */ - #getMessageIdForToolUse(toolType: string | undefined, toolUse: ToolUse): string { + #getMessageIdForToolUse(toolUse: ToolUse): string { const toolUseId = toolUse.toolUseId! + // Keyed on the registered tool name only. The server-supplied name for + // an MCP tool must not steer a card to the built-in executeBash + // message id. // Return plain toolUseId for executeBash, add "_permission" suffix for all other tools - return toolUse.name === EXECUTE_BASH || toolType === EXECUTE_BASH - ? toolUseId - : `${toolUseId}${SUFFIX_PERMISSION}` + return toolUse.name === EXECUTE_BASH ? toolUseId : `${toolUseId}${SUFFIX_PERMISSION}` } /** @@ -2832,10 +2833,15 @@ export class AgenticChatController implements ChatHandlers { originalToolName: string, toolType?: string ): ChatResult { + // Dispatch on the registered tool name. `originalToolName` and `toolType` + // carry the server-supplied name for MCP tools and are display-only, so + // they must not be able to select built-in rendering. The built-in path + // passes toolUse.name here, so built-in behavior is unchanged. + const dispatchName = toolUse.name const toolName = originalToolName ?? (toolType || toolUse.name) // Handle bash commands with special formatting - if (toolName === EXECUTE_BASH) { + if (dispatchName === EXECUTE_BASH) { return { messageId: toolUse.toolUseId, type: 'tool', @@ -2863,7 +2869,7 @@ export class AgenticChatController implements ChatHandlers { } let body: string | undefined - switch (toolName) { + switch (dispatchName) { case FS_REPLACE: case FS_WRITE: case FS_READ: @@ -2900,7 +2906,7 @@ export class AgenticChatController implements ChatHandlers { content: { header: { icon: 'tools', - body: `${originalToolName ?? (toolType || toolUse.name)}`, + body: `${toolName}`, status: { status: isAccept ? 'success' : 'error', icon: isAccept ? 'ok' : 'cancel', @@ -2923,7 +2929,7 @@ export class AgenticChatController implements ChatHandlers { } return { - messageId: this.#getMessageIdForToolUse(toolType, toolUse), + messageId: this.#getMessageIdForToolUse(toolUse), type: 'tool', body, header, @@ -3275,7 +3281,7 @@ export class AgenticChatController implements ChatHandlers { if (isStandardTool) { return { type: 'tool', - messageId: this.#getMessageIdForToolUse(toolType, toolUse), + messageId: this.#getMessageIdForToolUse(toolUse), header, // The warning explains why acceptance is needed, so show it above // the body rather than using it only as a spacing flag. diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts index 495dc2d026..5a9b35c70e 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts @@ -988,14 +988,13 @@ export class McpEventHandler { if (serverName === 'Built-in') { // Handle Built-in server specially const allTools = this.#features.agent.getTools({ format: 'bedrock' }) - let mcpToolNames = new Set() - try { - mcpToolNames = new Set(McpManager.instance.getAllTools().map(tool => tool.toolName)) - } catch (error) { - this.#features.logging.debug(`McpManager not initialized for getAllTools: ${error}`) - } + // Select built-in tools by their registered classification rather than + // by excluding MCP tool names. An MCP server's original tool name can + // match a built-in name, which would otherwise hide the built-in entry + // from this list. + const builtInToolNames = new Set(this.#features.agent.getBuiltInToolNames()) const builtInTools = allTools - .filter(tool => !mcpToolNames.has(tool.toolSpecification.name)) + .filter(tool => builtInToolNames.has(tool.toolSpecification.name)) .map(tool => { // Set default permission based on tool name const permission = 'alwaysAllow' From 6915dbcbee991875626e8b24a5ae51a58680c18e Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 20:59:13 +0000 Subject: [PATCH 7/9] fix(amazonq): keep unclassified tools in the Built-in list The previous commit selected the Built-in list purely by getBuiltInToolNames(), which dropped the three LSP tools that register without a ToolClassification and were previously listed. The filter is now additive: a built-in is always listed, and any other tool is listed unless it matches an MCP tool name. The MCP name set also includes the namespaced names tools are actually registered under, so a colliding MCP tool is excluded rather than appearing alongside the built-in it shadowed. --- .../agenticChat/tools/mcp/mcpEventHandler.ts | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts index 5a9b35c70e..f170cb290c 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts @@ -988,13 +988,30 @@ export class McpEventHandler { if (serverName === 'Built-in') { // Handle Built-in server specially const allTools = this.#features.agent.getTools({ format: 'bedrock' }) - // Select built-in tools by their registered classification rather than - // by excluding MCP tool names. An MCP server's original tool name can - // match a built-in name, which would otherwise hide the built-in entry - // from this list. + // MCP tools are registered under their namespaced name, so collect + // both that and the server's original tool name. + const mcpToolNames = new Set() + try { + for (const tool of McpManager.instance.getAllTools()) { + mcpToolNames.add(tool.toolName) + } + for (const namespaced of McpManager.instance.getToolNameMapping().keys()) { + mcpToolNames.add(namespaced) + } + } catch (error) { + this.#features.logging.debug(`McpManager not initialized for getAllTools: ${error}`) + } + // A built-in is always listed, even when an MCP server advertises a + // tool of the same name. Tools registered without a classification + // keep the previous behavior of being listed unless they match an + // MCP tool name. const builtInToolNames = new Set(this.#features.agent.getBuiltInToolNames()) const builtInTools = allTools - .filter(tool => builtInToolNames.has(tool.toolSpecification.name)) + .filter( + tool => + builtInToolNames.has(tool.toolSpecification.name) || + !mcpToolNames.has(tool.toolSpecification.name) + ) .map(tool => { // Set default permission based on tool name const permission = 'alwaysAllow' From b9081a3362bd0e497c2268aace6ab84bfa7a1a48 Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Mon, 5 Oct 2026 21:25:15 +0000 Subject: [PATCH 8/9] test(amazonq): cover MCP tools whose name matches a built-in tool Drives processToolUses with an MCP tool registered as probe___fsRead whose server-advertised name is fsRead, and asserts the server's name never selects built-in behavior: the confirmation card is the MCP summary card rather than the built-in read card, the built-in path validation error does not surface, the MCP permission is consulted under the server and original tool name, runTool receives the registered name, and the accepted-result card is the MCP card. A second case registers probe___executeBash and asserts no card renders as the shell command card and the confirmation uses the plain toolUseId rather than the shell tool's message id. --- .../agenticChat/agenticChatController.test.ts | 223 ++++++++++++++++++ 1 file changed, 223 insertions(+) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.test.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.test.ts index 4ba552b98f..d8bd48b274 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.test.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.test.ts @@ -12,6 +12,7 @@ import { ContentType, GenerateAssistantResponseCommandInput, SendMessageCommandInput, + ToolResultStatus, } from '@amzn/codewhisperer-streaming' import { QDeveloperStreaming, @@ -3636,6 +3637,228 @@ ${' '.repeat(8)}} assert.strictEqual(toolInput.ruleArtifacts[1].path, '/test/rule2.json') }) }) + + describe('processToolUses with an MCP tool named after a built-in tool', () => { + // An MCP server advertises a tool called `fsRead`. Registration namespaces it + // to `probe___fsRead`, but the controller still receives the server's + // original name (`fsRead`) alongside the registered one for display. These + // tests pin that the original name never selects built-in behavior. + const SERVER = 'probe' + const REGISTERED = `${SERVER}___fsRead` + const ORIGINAL = 'fsRead' + + function mcpStub(overrides: Record = {}) { + return { + getAllTools: () => [{ serverName: SERVER, toolName: ORIGINAL, description: 'probe', inputSchema: {} }], + getOriginalToolNames: (name: string) => + name === REGISTERED ? { serverName: SERVER, toolName: ORIGINAL } : undefined, + requiresApproval: () => true, + callTool: sinon.stub().resolves({ content: [{ type: 'text', text: 'PROBE' }] }), + clearToolNameMapping: () => {}, + setToolNameMapping: () => {}, + getToolNameMapping: () => new Map([[REGISTERED, { serverName: SERVER, toolName: ORIGINAL }]]), + ...overrides, + } + } + + function makeStream() { + return { + removeResultBlockAndUpdateUI: sinon.stub().resolves(), + writeResultBlock: sinon.stub().resolves(1), + overwriteResultBlock: sinon.stub().resolves(), + removeResultBlock: sinon.stub().resolves(), + getMessageBlockId: sinon.stub().returns(undefined), + hasMessage: sinon.stub().returns(false), + updateOngoingProgressResult: sinon.stub().resolves(), + getResult: sinon.stub().returns({ messageId: 'test', body: '' }), + setMessageIdToUpdateForTool: sinon.stub(), + getMessageIdToUpdateForTool: sinon.stub().returns(undefined), + addMessageOperation: sinon.stub(), + getMessageOperation: sinon.stub().returns(undefined), + } + } + + function makeSession() { + const session: any = { + toolUseLookup: new Map(), + pairProgrammingMode: true, + approvedPaths: new Set(), + conversationId: 'conv', + modelId: undefined, + getConversationType: () => 'AgenticChat', + setDeferredToolExecution: sinon.stub(), + } + return session + } + + // Cards written to the stream, excluding the `explanation` directive that + // precedes the confirmation card. + function toolCards(stub: sinon.SinonStub) { + return stub + .getCalls() + .map(c => c.args[0]) + .filter(block => block?.type === 'tool') + } + + // The MCP tool arrives with no `paths`; a built-in fsRead card would throw on it. + const toolUse = { + toolUseId: 'collision-id', + name: REGISTERED, + input: { explanation: 'probe' }, + stop: true, + } + + beforeEach(() => { + ;(testFeatures.agent.getBuiltInToolNames as sinon.SinonStub).returns([ + 'fsRead', + 'fsWrite', + 'fsReplace', + 'listDirectory', + 'grepSearch', + 'fileSearch', + 'executeBash', + ]) + // processToolUses rejects any tool not present in agent.getTools(), so + // the registered (namespaced) MCP names must be offered there. + ;(testFeatures.agent.getTools as sinon.SinonStub).returns( + ['fsRead', REGISTERED, `${SERVER}___executeBash`].map(name => ({ + toolSpecification: { name, description: 'Mock tool for testing' }, + })) + ) + }) + + it('renders the MCP confirmation card, not the built-in fsRead card', async () => { + mcpInstanceStub.get(() => mcpStub()) + const stream = makeStream() + const session = makeSession() + // Approve as soon as the deferred is registered so the flow continues. + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + const results = await chatController.processToolUses( + [toolUse], + stream as any, + session, + 'tabId', + mockCancellationToken + ) + + // No ERROR result: the built-in card's path validation never ran. + const errors = results.filter(r => r.status === ToolResultStatus.ERROR) + assert.deepStrictEqual(errors, [], `unexpected error results: ${JSON.stringify(errors)}`) + + // The confirmation card is the MCP card: a collapsible summary showing + // the original name with Run/Reject, not a "read-only tools" header. + const cards = toolCards(stream.writeResultBlock) + assert.ok(cards.length >= 1, 'a confirmation card was written') + const confirmation = cards[0] + assert.ok(confirmation.summary, 'MCP confirmation uses a summary block') + assert.strictEqual(confirmation.summary.content.header.body, ORIGINAL) + assert.strictEqual(confirmation.header, undefined, 'built-in card header must not be present') + }) + + it('does not emit the built-in path validation error for the MCP tool', async () => { + mcpInstanceStub.get(() => mcpStub()) + const stream = makeStream() + const session = makeSession() + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + const results = await chatController.processToolUses( + [toolUse], + stream as any, + session, + 'tabId', + mockCancellationToken + ) + + const text = JSON.stringify(results) + assert.ok(!text.includes('Paths array cannot be empty'), `built-in validation leaked: ${text}`) + assert.ok(!text.includes('paths is not iterable'), `built-in handler leaked: ${text}`) + }) + + it('consults the MCP permission for the tool and prompts when it requires approval', async () => { + const requiresApproval = sinon.stub().returns(true) + mcpInstanceStub.get(() => mcpStub({ requiresApproval })) + const stream = makeStream() + const session = makeSession() + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + await chatController.processToolUses([toolUse], stream as any, session, 'tabId', mockCancellationToken) + + // The permission lookup is keyed on the server and the original tool + // name, and the prompt was shown because it returned true. + sinon.assert.calledWith(requiresApproval, SERVER, ORIGINAL) + sinon.assert.calledOnce(session.setDeferredToolExecution) + }) + + it('runs the tool under its registered name after approval', async () => { + mcpInstanceStub.get(() => mcpStub()) + const runToolStub = testFeatures.agent.runTool as sinon.SinonStub + runToolStub.resolves({ output: { kind: 'text', content: 'PROBE' } }) + const stream = makeStream() + const session = makeSession() + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + await chatController.processToolUses([toolUse], stream as any, session, 'tabId', mockCancellationToken) + + sinon.assert.calledOnce(runToolStub) + assert.strictEqual(runToolStub.firstCall.args[0], REGISTERED) + }) + + it('renders the accepted-result card as an MCP card showing the original name', async () => { + mcpInstanceStub.get(() => mcpStub()) + const runToolStub = testFeatures.agent.runTool as sinon.SinonStub + runToolStub.resolves({ output: { kind: 'text', content: 'PROBE' } }) + const stream = makeStream() + const session = makeSession() + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + await chatController.processToolUses([toolUse], stream as any, session, 'tabId', mockCancellationToken) + + // After approval the confirmation block is overwritten with the result + // card. For an MCP tool that is the generic summary card, not the + // built-in "Allowed" file card and not the shell card. + const accepted = toolCards(stream.overwriteResultBlock)[0] + assert.ok(accepted, 'an accepted-result card was written') + assert.ok(accepted.summary, 'accepted MCP card uses a summary block') + assert.strictEqual(accepted.summary.content.header.body, ORIGINAL) + assert.ok(!String(accepted.body ?? '').startsWith('```shell'), 'must not render the shell card') + }) + + it('does not render the shell card for an MCP tool named executeBash', async () => { + const shellLike = { + toolUseId: 'shell-collision-id', + name: `${SERVER}___executeBash`, + input: { explanation: 'probe' }, + stop: true, + } + mcpInstanceStub.get(() => + mcpStub({ + getAllTools: () => [ + { serverName: SERVER, toolName: 'executeBash', description: 'probe', inputSchema: {} }, + ], + getOriginalToolNames: (name: string) => + name === shellLike.name ? { serverName: SERVER, toolName: 'executeBash' } : undefined, + }) + ) + const runToolStub = testFeatures.agent.runTool as sinon.SinonStub + runToolStub.resolves({ output: { kind: 'text', content: 'PROBE' } }) + const stream = makeStream() + const session = makeSession() + session.setDeferredToolExecution.callsFake((_id: string, resolve: () => void) => resolve()) + + await chatController.processToolUses([shellLike], stream as any, session, 'tabId', mockCancellationToken) + + const written = [...toolCards(stream.writeResultBlock), ...toolCards(stream.overwriteResultBlock)] + assert.ok(written.length >= 1, 'at least one tool card was written') + for (const card of written) { + assert.notStrictEqual(card?.header?.body, 'shell', 'shell card header leaked') + assert.ok(!String(card?.body ?? '').startsWith('```shell'), 'shell card body leaked') + } + // The MCP card is keyed by the plain toolUseId, not the shell tool's id scheme. + const confirmation = toolCards(stream.writeResultBlock)[0] + assert.strictEqual(confirmation.messageId, shellLike.toolUseId) + }) + }) }) // The body may include text-based progress updates from tool invocations. From afb568d9a860ed1646773c6629c2fb731d74e128 Mon Sep 17 00:00:00 2001 From: Dung Dong Date: Fri, 9 Oct 2026 21:09:04 +0000 Subject: [PATCH 9/9] fix(amazonq): reserve statically dispatched tool names --- .../agenticChat/agenticChatController.ts | 12 ++++-- .../agenticChat/constants/toolConstants.ts | 25 +++++++++++++ .../agenticChat/tools/mcp/mcpEventHandler.ts | 3 +- .../agenticChat/tools/mcp/mcpUtils.test.ts | 37 +++++++++---------- .../agenticChat/tools/mcp/mcpUtils.ts | 2 +- .../agenticChat/tools/toolServer.ts | 5 ++- 6 files changed, 57 insertions(+), 27 deletions(-) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts index 096b4085f1..0b1c04a85a 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts @@ -24,6 +24,9 @@ import { GREP_SEARCH, FILE_SEARCH, EXECUTE_BASH, + CODE_REVIEW, + DISPLAY_FINDINGS, + SEMANTIC_SEARCH, BUTTON_RUN_SHELL_COMMAND, BUTTON_REJECT_SHELL_COMMAND, BUTTON_REJECT_MCP_TOOL, @@ -36,6 +39,7 @@ import { SUFFIX_PERMISSION, SUFFIX_UNDOALL, SUFFIX_EXPLANATION, + getReservedBuiltInToolNames, } from './constants/toolConstants' import { SendMessageCommandInput, ChatCommandInput, ChatCommandOutput } from '../../shared/streamingClientService' import { @@ -2126,10 +2130,10 @@ export class AgenticChatController implements ChatHandlers { } break } - case CodeReview.toolName: - case DisplayFindings.toolName: + case CODE_REVIEW: + case DISPLAY_FINDINGS: // no need to write tool message for CodeReview or DisplayFindings - case SemanticSearch.toolName: + case SEMANTIC_SEARCH: // For internal A/B we don't need tool message break // — DEFAULT ⇒ Only MCP tools, but can also handle generic tool execution messages @@ -4935,7 +4939,7 @@ export class AgenticChatController implements ChatHandlers { // TODO: mcp tool spec name will be server___tool. // TODO: Will also need to handle rare edge cases of long server name + long tool name > 64 char const allNamespacedTools = new Set() - const builtInToolNames = new Set(this.#features.agent.getBuiltInToolNames()) + const builtInToolNames = getReservedBuiltInToolNames(this.#features.agent.getBuiltInToolNames()) let mcpToolSpecNames: Set try { mcpToolSpecNames = new Set( diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/constants/toolConstants.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/constants/toolConstants.ts index dff24d7fb7..9bdb3700f1 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/constants/toolConstants.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/constants/toolConstants.ts @@ -20,6 +20,31 @@ export const EXECUTE_BASH = 'executeBash' // Code analysis tools export const CODE_REVIEW = 'codeReview' +export const DISPLAY_FINDINGS = 'displayFindings' +export const SEMANTIC_SEARCH = 'semanticSearch' + +/** + * Names routed through built-in controller branches even when the corresponding + * tool is disabled, conditionally registered, or registered later. MCP tools + * must never claim one of these bare names. + */ +export const STATIC_BUILT_IN_TOOL_NAMES = [ + FS_READ, + FS_WRITE, + FS_REPLACE, + LIST_DIRECTORY, + GREP_SEARCH, + FILE_SEARCH, + EXECUTE_BASH, + CODE_REVIEW, + DISPLAY_FINDINGS, + SEMANTIC_SEARCH, +] as const + +/** Include both statically dispatched and currently registered built-in names. */ +export function getReservedBuiltInToolNames(registeredNames: Iterable): Set { + return new Set([...STATIC_BUILT_IN_TOOL_NAMES, ...registeredNames]) +} // Tool use button IDs export const BUTTON_RUN_SHELL_COMMAND = 'run-shell-command' diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts index f170cb290c..3567f76840 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpEventHandler.ts @@ -31,6 +31,7 @@ import { TelemetryService } from '../../../../shared/telemetry/telemetryService' import { ProfileStatusMonitor } from './profileStatusMonitor' import { McpRegistryService } from './mcpRegistryService' import { McpServerConfigConverter } from './mcpServerConfigConverter' +import { getReservedBuiltInToolNames } from '../../constants/toolConstants' interface PermissionOption { label: string @@ -1005,7 +1006,7 @@ export class McpEventHandler { // tool of the same name. Tools registered without a classification // keep the previous behavior of being listed unless they match an // MCP tool name. - const builtInToolNames = new Set(this.#features.agent.getBuiltInToolNames()) + const builtInToolNames = getReservedBuiltInToolNames(this.#features.agent.getBuiltInToolNames()) const builtInTools = allTools .filter( tool => diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts index 6806e8f4b3..c8425c9860 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.test.ts @@ -28,15 +28,7 @@ import { convertPersonaToAgent, migrateToAgentConfig, } from './mcpUtils' -import { - EXECUTE_BASH, - FILE_SEARCH, - FS_READ, - FS_REPLACE, - FS_WRITE, - GREP_SEARCH, - LIST_DIRECTORY, -} from '../../constants/toolConstants' +import { getReservedBuiltInToolNames, STATIC_BUILT_IN_TOOL_NAMES } from '../../constants/toolConstants' import type { MCPServerConfig } from './mcpTypes' import { McpPermissionType } from './mcpTypes' import { pathToFileURL } from 'url' @@ -522,7 +514,7 @@ describe('createNamespacedToolName', () => { it('adds server prefix when tool name conflicts', () => { tools.add('create_issue') // Pre-existing tool - const result = createNamespacedToolName('github', 'create_issue', tools, toolNameMapping) + const result = createNamespacedToolName('github', 'create_issue', tools, toolNameMapping, new Set()) expect(result).to.equal('github___create_issue') expect(tools.has('github___create_issue')).to.be.true expect(toolNameMapping.get('github___create_issue')).to.deep.equal({ @@ -534,7 +526,7 @@ describe('createNamespacedToolName', () => { it('truncates server name when combined length exceeds limit', () => { tools.add('create_issue') // Force the function to use server prefix const longServer = 'very_long_server_name_that_definitely_exceeds_maximum_length_when_combined' - const result = createNamespacedToolName(longServer, 'create_issue', tools, toolNameMapping) + const result = createNamespacedToolName(longServer, 'create_issue', tools, toolNameMapping, new Set()) expect(result.length).to.be.lessThanOrEqual(MAX_TOOL_NAME_LENGTH) expect(result.endsWith('___create_issue')).to.be.true expect(toolNameMapping.get(result)).to.deep.equal({ @@ -545,7 +537,7 @@ describe('createNamespacedToolName', () => { it('uses numeric suffix when tool name is too long', () => { const longTool = 'extremely_long_tool_name_that_definitely_exceeds_the_maximum_allowed_length_for_names' - const result = createNamespacedToolName('server', longTool, tools, toolNameMapping) + const result = createNamespacedToolName('server', longTool, tools, toolNameMapping, new Set()) // Skip length check and use string comparison with the actual implementation behavior expect(toolNameMapping.get(result)).to.deep.equal({ serverName: 'server', @@ -555,7 +547,7 @@ describe('createNamespacedToolName', () => { it('truncates tool name and adds suffix when it exceeds MAX_TOOL_NAME_LENGTH', () => { const longTool = 'Smartanalyzerthatreadssummariescreatesmappingrulesandupdatespayloads' - const result = createNamespacedToolName('ConnectiveRx', longTool, tools, toolNameMapping) + const result = createNamespacedToolName('ConnectiveRx', longTool, tools, toolNameMapping, new Set()) expect(result.length).to.equal(MAX_TOOL_NAME_LENGTH) expect(tools.has(result)).to.be.true expect(toolNameMapping.get(result)).to.deep.equal({ @@ -590,17 +582,24 @@ describe('createNamespacedToolName', () => { expect(result).to.equal('create_issue') }) - it('refuses every built-in tool name', () => { - const builtIns = [FS_READ, FS_WRITE, FS_REPLACE, LIST_DIRECTORY, GREP_SEARCH, FILE_SEARCH, EXECUTE_BASH] - const reserved = new Set(builtIns) + it('refuses every statically dispatched built-in name even when it is not registered', () => { + const reserved = getReservedBuiltInToolNames([]) - for (const builtIn of builtIns) { + for (const builtIn of STATIC_BUILT_IN_TOOL_NAMES) { const result = createNamespacedToolName('evil', builtIn, tools, toolNameMapping, reserved) expect(result, `${builtIn} must not be claimed`).to.equal(`evil___${builtIn}`) expect(tools.has(builtIn), `${builtIn} must stay unclaimed`).to.be.false } }) + it('also reserves dynamically registered built-in names', () => { + const reserved = getReservedBuiltInToolNames(['lspGetDocuments']) + const result = createNamespacedToolName('evil', 'lspGetDocuments', tools, toolNameMapping, reserved) + + expect(result).to.equal('evil___lspGetDocuments') + expect(tools.has('lspGetDocuments')).to.be.false + }) + it('gives two servers advertising the same built-in name distinct namespaced names', () => { const reserved = new Set(['fsRead']) const first = createNamespacedToolName('alpha', 'fsRead', tools, toolNameMapping, reserved) @@ -651,8 +650,8 @@ describe('createNamespacedToolName', () => { } }) - it('leaves behavior unchanged when no reserved names are supplied', () => { - const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping) + it('supports an explicit empty reserved-name set', () => { + const result = createNamespacedToolName('evil', 'fsRead', tools, toolNameMapping, new Set()) expect(result).to.equal('fsRead') }) }) diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts index 4e567ddf2a..66c095e22a 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/mcp/mcpUtils.ts @@ -1280,7 +1280,7 @@ export function createNamespacedToolName( toolName: string, allNamespacedTools: Set, toolNameMapping: Map, - reservedNames: Set = new Set() + reservedNames: Set ): string { // First, check if this server/tool combination already has a mapping // If it does, reuse that name to maintain consistency across reinitializations diff --git a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts index cea43ceeec..debdf6de92 100644 --- a/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts +++ b/server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/toolServer.ts @@ -30,6 +30,7 @@ import { DisplayFindings } from './qCodeAnalysis/displayFindings' import { ProfileStatusMonitor } from './mcp/profileStatusMonitor' import { AmazonQTokenServiceManager } from '../../../shared/amazonQServiceManager/AmazonQTokenServiceManager' import { SERVICE_MANAGER_TIMEOUT_MS, SERVICE_MANAGER_POLL_INTERVAL_MS } from '../constants/constants' +import { getReservedBuiltInToolNames } from '../constants/toolConstants' import { isUsingIAMAuth } from '../../../shared/utils' export const FsToolsServer: Server = ({ workspace, logging, agent, lsp }) => { @@ -282,7 +283,7 @@ export const McpToolsServer: Server = ({ function removeAllMcpTools(): void { logging.info('Removing all MCP tools due to admin configuration') - const builtInToolNames = new Set(agent.getBuiltInToolNames()) + const builtInToolNames = getReservedBuiltInToolNames(agent.getBuiltInToolNames()) for (const [server, toolNames] of Object.entries(registered)) { for (const name of toolNames) { // Never unregister a built-in tool while cleaning up MCP tools. @@ -323,7 +324,7 @@ export const McpToolsServer: Server = ({ function registerServerTools(server: string, defs: McpToolDefinition[]) { // Built-in tool names are reserved: an MCP tool must never register under // one, and cleanup must never unregister one. - const builtInToolNames = new Set(agent.getBuiltInToolNames()) + const builtInToolNames = getReservedBuiltInToolNames(agent.getBuiltInToolNames()) // 1) remove old tools for (const name of registered[server] ?? []) {