From eb00079158770c76a0cbf2a9cab9d0516e656126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D0=BB=D0=B5=D0=BA=D1=81=D0=B0=D0=BD=D0=B4=D1=80=20?= =?UTF-8?q?=D0=A0=D0=BE=D0=B4=D0=B8=D0=BE=D0=BD=D0=BE=D0=B2?= Date: Sat, 31 May 2025 23:12:52 +0300 Subject: [PATCH] respect review --- src/__mocks__/fs/promises.ts | 2 +- .../prompts/instructions/create-mcp-server.ts | 4 +- src/services/mcp/McpHub.ts | 211 ++++++++---------- src/services/mcp/__tests__/McpHub.spec.ts | 22 +- 4 files changed, 101 insertions(+), 138 deletions(-) diff --git a/src/__mocks__/fs/promises.ts b/src/__mocks__/fs/promises.ts index 63d7a42007..daed479d4a 100644 --- a/src/__mocks__/fs/promises.ts +++ b/src/__mocks__/fs/promises.ts @@ -168,7 +168,7 @@ const mockFs = { args: ["test.js"], disabled: false, alwaysAllow: ["existing-tool"], - disabledForPromptTools: [], + disabledTools: [], }, }, }), diff --git a/src/core/prompts/instructions/create-mcp-server.ts b/src/core/prompts/instructions/create-mcp-server.ts index f2b4b2e61e..a63fad1de5 100644 --- a/src/core/prompts/instructions/create-mcp-server.ts +++ b/src/core/prompts/instructions/create-mcp-server.ts @@ -50,7 +50,7 @@ Common configuration options for both types: - \`disabled\`: (optional) Set to true to temporarily disable the server - \`timeout\`: (optional) Maximum time in seconds to wait for server responses (default: 60) - \`alwaysAllow\`: (optional) Array of tool names that don't require user confirmation -- \`disabledForPromptTools\`: (optional) Array of tool names that are not included in the system prompt and won't be used +- \`disabledTools\`: (optional) Array of tool names that are not included in the system prompt and won't be used ### Example Local MCP Server @@ -277,7 +277,7 @@ npm run build 5. Install the MCP Server by adding the MCP server configuration to the settings file located at '${await mcpHub.getMcpSettingsFilePath()}'. The settings file may have other MCP servers already configured, so you would read it first and then add your new server to the existing \`mcpServers\` object. -IMPORTANT: Regardless of what else you see in the MCP settings file, you must default any new MCP servers you create to disabled=false, alwaysAllow=[] and disabledForPromptTools=[]. +IMPORTANT: Regardless of what else you see in the MCP settings file, you must default any new MCP servers you create to disabled=false, alwaysAllow=[] and disabledTools=[]. \`\`\`json { diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 88a2248dcb..d95ea241b8 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -45,7 +45,7 @@ const BaseConfigSchema = z.object({ timeout: z.number().min(1).max(3600).optional().default(60), alwaysAllow: z.array(z.string()).default([]), watchPaths: z.array(z.string()).optional(), // paths to watch for changes and restart server - disabledForPromptTools: z.array(z.string()).default([]), + disabledTools: z.array(z.string()).default([]), }) // Custom error messages for better user feedback @@ -820,7 +820,7 @@ export class McpHub { const actualSource = connection.server.source || "global" let configPath: string let alwaysAllowConfig: string[] = [] - let disabledForPromptToolsList: string[] = [] + let disabledToolsList: string[] = [] // Read from the appropriate config file based on the actual source try { @@ -841,7 +841,7 @@ export class McpHub { } if (serverConfigData) { alwaysAllowConfig = serverConfigData.mcpServers?.[serverName]?.alwaysAllow || [] - disabledForPromptToolsList = serverConfigData.mcpServers?.[serverName]?.disabledForPromptTools || [] + disabledToolsList = serverConfigData.mcpServers?.[serverName]?.disabledTools || [] } } catch (error) { console.error(`Failed to read tool configuration for ${serverName}:`, error) @@ -852,7 +852,7 @@ export class McpHub { const tools = (response?.tools || []).map((tool) => ({ ...tool, alwaysAllow: alwaysAllowConfig.includes(tool.name), - enabledForPrompt: !disabledForPromptToolsList.includes(tool.name), + enabledForPrompt: !disabledToolsList.includes(tool.name), })) return tools @@ -1481,6 +1481,84 @@ export class McpHub { ) } + /** + * Helper method to update a specific tool list (alwaysAllow or disabledTools) + * in the appropriate settings file. + * @param serverName The name of the server to update + * @param source Whether to update the global or project config + * @param toolName The name of the tool to add or remove + * @param listName The name of the list to modify ("alwaysAllow" or "disabledTools") + * @param addTool Whether to add (true) or remove (false) the tool from the list + */ + private async updateServerToolList( + serverName: string, + source: "global" | "project", + toolName: string, + listName: "alwaysAllow" | "disabledTools", + addTool: boolean, + ): Promise { + // Find the connection with matching name and source + const connection = this.findConnection(serverName, source) + + if (!connection) { + throw new Error(`Server ${serverName} with source ${source} not found`) + } + + // Determine the correct config path based on the source + let configPath: string + if (source === "project") { + // Get project MCP config path + const projectMcpPath = await this.getProjectMcpPath() + if (!projectMcpPath) { + throw new Error("Project MCP configuration file not found") + } + configPath = projectMcpPath + } else { + // Get global MCP settings path + configPath = await this.getMcpSettingsFilePath() + } + + // Normalize path for cross-platform compatibility + // Use a consistent path format for both reading and writing + const normalizedPath = process.platform === "win32" ? configPath.replace(/\\/g, "/") : configPath + + // Read the appropriate config file + const content = await fs.readFile(normalizedPath, "utf-8") + const config = JSON.parse(content) + + if (!config.mcpServers) { + config.mcpServers = {} + } + + if (!config.mcpServers[serverName]) { + config.mcpServers[serverName] = { + type: "stdio", + command: "node", + args: [], // Default to an empty array; can be set later if needed + } + } + + if (!config.mcpServers[serverName][listName]) { + config.mcpServers[serverName][listName] = [] + } + + const targetList = config.mcpServers[serverName][listName] + const toolIndex = targetList.indexOf(toolName) + + if (addTool && toolIndex === -1) { + targetList.push(toolName) + } else if (!addTool && toolIndex !== -1) { + targetList.splice(toolIndex, 1) + } + + await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) + + if (connection) { + connection.server.tools = await this.fetchToolsList(serverName, source) + await this.notifyWebviewOfServerChanges() + } + } + async toggleToolAlwaysAllow( serverName: string, source: "global" | "project", @@ -1488,74 +1566,7 @@ export class McpHub { shouldAllow: boolean, ): Promise { try { - // Find the connection with matching name and source - const connection = this.findConnection(serverName, source) - - if (!connection) { - throw new Error(`Server ${serverName} with source ${source} not found`) - } - - // Determine the correct config path based on the source - let configPath: string - if (source === "project") { - // Get project MCP config path - const projectMcpPath = await this.getProjectMcpPath() - if (!projectMcpPath) { - throw new Error("Project MCP configuration file not found") - } - configPath = projectMcpPath - } else { - // Get global MCP settings path - configPath = await this.getMcpSettingsFilePath() - } - - // Normalize path for cross-platform compatibility - // Use a consistent path format for both reading and writing - const normalizedPath = process.platform === "win32" ? configPath.replace(/\\/g, "/") : configPath - - // Read the appropriate config file - const content = await fs.readFile(normalizedPath, "utf-8") - const config = JSON.parse(content) - - // Initialize mcpServers if it doesn't exist - if (!config.mcpServers) { - config.mcpServers = {} - } - - // Initialize server config if it doesn't exist - if (!config.mcpServers[serverName]) { - config.mcpServers[serverName] = { - type: "stdio", - command: "node", - args: [], // Default to an empty array; can be set later if needed - } - } - - // Initialize alwaysAllow if it doesn't exist - if (!config.mcpServers[serverName].alwaysAllow) { - config.mcpServers[serverName].alwaysAllow = [] - } - - const alwaysAllow = config.mcpServers[serverName].alwaysAllow - const toolIndex = alwaysAllow.indexOf(toolName) - - if (shouldAllow && toolIndex === -1) { - // Add tool to always allow list - alwaysAllow.push(toolName) - } else if (!shouldAllow && toolIndex !== -1) { - // Remove tool from always allow list - alwaysAllow.splice(toolIndex, 1) - } - - // Write updated config back to file - await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) - - // Update the tools list to reflect the change - if (connection) { - // Explicitly pass the source to ensure we're updating the correct server's tools - connection.server.tools = await this.fetchToolsList(serverName, source) - await this.notifyWebviewOfServerChanges() - } + await this.updateServerToolList(serverName, source, toolName, "alwaysAllow", shouldAllow) } catch (error) { this.showErrorMessage( `Failed to toggle always allow for tool "${toolName}" on server "${serverName}" with source "${source}"`, @@ -1572,58 +1583,10 @@ export class McpHub { isEnabled: boolean, ): Promise { try { - const connection = this.findConnection(serverName, source) - if (!connection) { - throw new Error(`Server ${serverName} with source ${source} not found`) - } - - let configPath: string - if (source === "project") { - const projectMcpPath = await this.getProjectMcpPath() - if (!projectMcpPath) { - throw new Error("Project MCP configuration file not found") - } - configPath = projectMcpPath - } else { - configPath = await this.getMcpSettingsFilePath() - } - - const normalizedPath = process.platform === "win32" ? configPath.replace(/\\/g, "/") : configPath - const content = await fs.readFile(normalizedPath, "utf-8") - const config = JSON.parse(content) - - if (!config.mcpServers) { - config.mcpServers = {} - } - if (!config.mcpServers[serverName]) { - // Initialize with minimal valid config if server entry doesn't exist - config.mcpServers[serverName] = { - type: "stdio", - command: "echo", - args: ["MCP server not fully configured"], - } - } - if (!config.mcpServers[serverName].disabledForPromptTools) { - config.mcpServers[serverName].disabledForPromptTools = [] - } - - const disabledList = config.mcpServers[serverName].disabledForPromptTools - const toolIndex = disabledList.indexOf(toolName) - - if (!isEnabled && toolIndex === -1) { - // If tool should be disabled (not included in prompt) and is not in disabled list - disabledList.push(toolName) - } else if (isEnabled && toolIndex !== -1) { - // If tool should be enabled (included in prompt) and is in disabled list - disabledList.splice(toolIndex, 1) - } - - await fs.writeFile(normalizedPath, JSON.stringify(config, null, 2)) - - if (connection) { - connection.server.tools = await this.fetchToolsList(serverName, source) - await this.notifyWebviewOfServerChanges() - } + // When isEnabled is true, we want to remove the tool from the disabledTools list. + // When isEnabled is false, we want to add the tool to the disabledTools list. + const addToolToDisabledList = !isEnabled + await this.updateServerToolList(serverName, source, toolName, "disabledTools", addToolToDisabledList) } catch (error) { this.showErrorMessage(`Failed to update disabledForPromptTool settings for tool ${toolName}`, error) throw error // Re-throw to ensure the error is properly handled diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index a9e8c39a7e..902288c97e 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -100,7 +100,7 @@ describe("McpHub", () => { command: "node", args: ["test.js"], alwaysAllow: ["allowed-tool"], - disabledForPromptTools: ["disabled-tool"], + disabledTools: ["disabled-tool"], }, }, }), @@ -259,14 +259,14 @@ describe("McpHub", () => { }) describe("toggleToolEnabledForPrompt", () => { - it("should add tool to disabledForPromptTools list when enabling", async () => { + it("should add tool to disabledTools list when enabling", async () => { const mockConfig = { mcpServers: { "test-server": { type: "stdio", command: "node", args: ["test.js"], - disabledForPromptTools: [], + disabledTools: [], }, }, } @@ -290,17 +290,17 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers).toBeDefined() expect(writtenConfig.mcpServers["test-server"]).toBeDefined() expect(Array.isArray(writtenConfig.mcpServers["test-server"].enabledForPrompt)).toBe(false) - expect(writtenConfig.mcpServers["test-server"].disabledForPromptTools).toContain("new-tool") + expect(writtenConfig.mcpServers["test-server"].disabledTools).toContain("new-tool") }) - it("should remove tool from disabledForPromptTools list when disabling", async () => { + it("should remove tool from disabledTools list when disabling", async () => { const mockConfig = { mcpServers: { "test-server": { type: "stdio", command: "node", args: ["test.js"], - disabledForPromptTools: ["existing-tool"], + disabledTools: ["existing-tool"], }, }, } @@ -324,10 +324,10 @@ describe("McpHub", () => { expect(writtenConfig.mcpServers).toBeDefined() expect(writtenConfig.mcpServers["test-server"]).toBeDefined() expect(Array.isArray(writtenConfig.mcpServers["test-server"].enabledForPrompt)).toBe(false) - expect(writtenConfig.mcpServers["test-server"].disabledForPromptTools).not.toContain("existing-tool") + expect(writtenConfig.mcpServers["test-server"].disabledTools).not.toContain("existing-tool") }) - it("should initialize disabledForPromptTools if it does not exist", async () => { + it("should initialize disabledTools if it does not exist", async () => { const mockConfig = { mcpServers: { "test-server": { @@ -344,7 +344,7 @@ describe("McpHub", () => { // Call with false because of "true" is default value await mcpHub.toggleToolEnabledForPrompt("test-server", "global", "new-tool", false) - // Verify the config was updated with initialized disabledForPromptTools + // Verify the config was updated with initialized disabledTools // Find the write call with the normalized path const normalizedSettingsPath = "/mock/settings/path/cline_mcp_settings.json" const writeCalls = (fs.writeFile as jest.Mock).mock.calls @@ -354,8 +354,8 @@ describe("McpHub", () => { const callToUse = writeCall || writeCalls[0] const writtenConfig = JSON.parse(callToUse[1]) - expect(writtenConfig.mcpServers["test-server"].disabledForPromptTools).toBeDefined() - expect(writtenConfig.mcpServers["test-server"].disabledForPromptTools).toContain("new-tool") + expect(writtenConfig.mcpServers["test-server"].disabledTools).toBeDefined() + expect(writtenConfig.mcpServers["test-server"].disabledTools).toContain("new-tool") }) })