From 528d6d37f23609aefc190d1ee314a59080cd4322 Mon Sep 17 00:00:00 2001 From: LiuVaayne <10231735+vaayne@users.noreply.github.com> Date: Mon, 29 Dec 2025 18:52:58 +0800 Subject: [PATCH] refactor: simplify buildFunctionCallToolName to use mcp__{server}__{tool} format (#12186) --- src/main/services/MCPService.ts | 2 +- src/main/utils/__tests__/mcp.test.ts | 355 +++++++++++++++------------ src/main/utils/mcp.ts | 72 ++---- 3 files changed, 215 insertions(+), 214 deletions(-) diff --git a/src/main/services/MCPService.ts b/src/main/services/MCPService.ts index 5fc5cf8682..7d36e6d7e3 100644 --- a/src/main/services/MCPService.ts +++ b/src/main/services/MCPService.ts @@ -785,7 +785,7 @@ class McpService { ...tool, inputSchema: z.parse(MCPToolInputSchema, tool.inputSchema), outputSchema: tool.outputSchema ? z.parse(MCPToolOutputSchema, tool.outputSchema) : undefined, - id: buildFunctionCallToolName(server.name, tool.name, server.id), + id: buildFunctionCallToolName(server.name, tool.name), serverId: server.id, serverName: server.name, type: 'mcp' diff --git a/src/main/utils/__tests__/mcp.test.ts b/src/main/utils/__tests__/mcp.test.ts index b1a35f925e..706a44bc84 100644 --- a/src/main/utils/__tests__/mcp.test.ts +++ b/src/main/utils/__tests__/mcp.test.ts @@ -3,194 +3,223 @@ import { describe, expect, it } from 'vitest' import { buildFunctionCallToolName } from '../mcp' describe('buildFunctionCallToolName', () => { - describe('basic functionality', () => { - it('should combine server name and tool name', () => { + describe('basic format', () => { + it('should return format mcp__{server}__{tool}', () => { const result = buildFunctionCallToolName('github', 'search_issues') - expect(result).toContain('github') - expect(result).toContain('search') + expect(result).toBe('mcp__github__search_issues') }) - it('should sanitize names by replacing dashes with underscores', () => { - const result = buildFunctionCallToolName('my-server', 'my-tool') - // Input dashes are replaced, but the separator between server and tool is a dash - expect(result).toBe('my_serv-my_tool') - expect(result).toContain('_') - }) - - it('should handle empty server names gracefully', () => { - const result = buildFunctionCallToolName('', 'tool') - expect(result).toBeTruthy() + it('should handle simple server and tool names', () => { + expect(buildFunctionCallToolName('fetch', 'get_page')).toBe('mcp__fetch__get_page') + expect(buildFunctionCallToolName('database', 'query')).toBe('mcp__database__query') + expect(buildFunctionCallToolName('cherry_studio', 'search')).toBe('mcp__cherry_studio__search') }) }) - describe('uniqueness with serverId', () => { - it('should generate different IDs for same server name but different serverIds', () => { - const serverId1 = 'server-id-123456' - const serverId2 = 'server-id-789012' - const serverName = 'github' - const toolName = 'search_repos' - - const result1 = buildFunctionCallToolName(serverName, toolName, serverId1) - const result2 = buildFunctionCallToolName(serverName, toolName, serverId2) - - expect(result1).not.toBe(result2) - expect(result1).toContain('123456') - expect(result2).toContain('789012') + describe('valid JavaScript identifier', () => { + it('should always start with mcp__ prefix (valid JS identifier start)', () => { + const result = buildFunctionCallToolName('123server', '456tool') + expect(result).toMatch(/^mcp__/) + expect(result).toBe('mcp__123server__456tool') }) - it('should generate same ID when serverId is not provided', () => { + it('should only contain alphanumeric chars and underscores', () => { + const result = buildFunctionCallToolName('my-server', 'my-tool') + expect(result).toBe('mcp__my_server__my_tool') + expect(result).toMatch(/^[a-zA-Z][a-zA-Z0-9_]*$/) + }) + + it('should be a valid JavaScript identifier', () => { + const testCases = [ + ['github', 'create_issue'], + ['my-server', 'fetch-data'], + ['test@server', 'tool#name'], + ['server.name', 'tool.action'], + ['123abc', 'def456'] + ] + + for (const [server, tool] of testCases) { + const result = buildFunctionCallToolName(server, tool) + // Valid JS identifiers match this pattern + expect(result).toMatch(/^[a-zA-Z_][a-zA-Z0-9_]*$/) + } + }) + }) + + describe('character sanitization', () => { + it('should replace dashes with underscores', () => { + const result = buildFunctionCallToolName('my-server', 'my-tool-name') + expect(result).toBe('mcp__my_server__my_tool_name') + }) + + it('should replace special characters with underscores', () => { + const result = buildFunctionCallToolName('test@server!', 'tool#name$') + expect(result).toBe('mcp__test_server__tool_name') + }) + + it('should replace dots with underscores', () => { + const result = buildFunctionCallToolName('server.name', 'tool.action') + expect(result).toBe('mcp__server_name__tool_action') + }) + + it('should replace spaces with underscores', () => { + const result = buildFunctionCallToolName('my server', 'my tool') + expect(result).toBe('mcp__my_server__my_tool') + }) + + it('should collapse consecutive underscores', () => { + const result = buildFunctionCallToolName('my--server', 'my___tool') + expect(result).toBe('mcp__my_server__my_tool') + expect(result).not.toMatch(/_{3,}/) + }) + + it('should trim leading and trailing underscores from parts', () => { + const result = buildFunctionCallToolName('_server_', '_tool_') + expect(result).toBe('mcp__server__tool') + }) + + it('should handle names with only special characters', () => { + const result = buildFunctionCallToolName('---', '###') + expect(result).toBe('mcp____') + }) + }) + + describe('length constraints', () => { + it('should not exceed 63 characters', () => { + const longServerName = 'a'.repeat(50) + const longToolName = 'b'.repeat(50) + const result = buildFunctionCallToolName(longServerName, longToolName) + + expect(result.length).toBeLessThanOrEqual(63) + }) + + it('should truncate server name to max 20 chars', () => { + const longServerName = 'abcdefghijklmnopqrstuvwxyz' // 26 chars + const result = buildFunctionCallToolName(longServerName, 'tool') + + expect(result).toBe('mcp__abcdefghijklmnopqrst__tool') + expect(result).toContain('abcdefghijklmnopqrst') // First 20 chars + expect(result).not.toContain('uvwxyz') // Truncated + }) + + it('should truncate tool name to max 35 chars', () => { + const longToolName = 'a'.repeat(40) + const result = buildFunctionCallToolName('server', longToolName) + + const expectedTool = 'a'.repeat(35) + expect(result).toBe(`mcp__server__${expectedTool}`) + }) + + it('should not end with underscores after truncation', () => { + // Create a name that would end with underscores after truncation + const longServerName = 'a'.repeat(20) + const longToolName = 'b'.repeat(35) + '___extra' + const result = buildFunctionCallToolName(longServerName, longToolName) + + expect(result).not.toMatch(/_+$/) + expect(result.length).toBeLessThanOrEqual(63) + }) + + it('should handle max length edge case exactly', () => { + // mcp__ (5) + server (20) + __ (2) + tool (35) = 62 chars + const server = 'a'.repeat(20) + const tool = 'b'.repeat(35) + const result = buildFunctionCallToolName(server, tool) + + expect(result.length).toBe(62) + expect(result).toBe(`mcp__${'a'.repeat(20)}__${'b'.repeat(35)}`) + }) + }) + + describe('edge cases', () => { + it('should handle empty server name', () => { + const result = buildFunctionCallToolName('', 'tool') + expect(result).toBe('mcp____tool') + }) + + it('should handle empty tool name', () => { + const result = buildFunctionCallToolName('server', '') + expect(result).toBe('mcp__server__') + }) + + it('should handle both empty names', () => { + const result = buildFunctionCallToolName('', '') + expect(result).toBe('mcp____') + }) + + it('should handle whitespace-only names', () => { + const result = buildFunctionCallToolName(' ', ' ') + expect(result).toBe('mcp____') + }) + + it('should trim whitespace from names', () => { + const result = buildFunctionCallToolName(' server ', ' tool ') + expect(result).toBe('mcp__server__tool') + }) + + it('should handle unicode characters', () => { + const result = buildFunctionCallToolName('服务器', '工具') + // Unicode chars are replaced with underscores, then collapsed + expect(result).toMatch(/^mcp__/) + }) + + it('should handle mixed case', () => { + const result = buildFunctionCallToolName('MyServer', 'MyTool') + expect(result).toBe('mcp__MyServer__MyTool') + }) + }) + + describe('deterministic output', () => { + it('should produce consistent results for same input', () => { const serverName = 'github' const toolName = 'search_repos' const result1 = buildFunctionCallToolName(serverName, toolName) const result2 = buildFunctionCallToolName(serverName, toolName) + const result3 = buildFunctionCallToolName(serverName, toolName) expect(result1).toBe(result2) + expect(result2).toBe(result3) }) - it('should include serverId suffix when provided', () => { - const serverId = 'abc123def456' - const result = buildFunctionCallToolName('server', 'tool', serverId) + it('should produce different results for different inputs', () => { + const result1 = buildFunctionCallToolName('server1', 'tool') + const result2 = buildFunctionCallToolName('server2', 'tool') + const result3 = buildFunctionCallToolName('server', 'tool1') + const result4 = buildFunctionCallToolName('server', 'tool2') - // Should include last 6 chars of serverId - expect(result).toContain('ef456') - }) - }) - - describe('character sanitization', () => { - it('should replace invalid characters with underscores', () => { - const result = buildFunctionCallToolName('test@server', 'tool#name') - expect(result).not.toMatch(/[@#]/) - expect(result).toMatch(/^[a-zA-Z0-9_-]+$/) - }) - - it('should ensure name starts with a letter', () => { - const result = buildFunctionCallToolName('123server', '456tool') - expect(result).toMatch(/^[a-zA-Z]/) - }) - - it('should handle consecutive underscores/dashes', () => { - const result = buildFunctionCallToolName('my--server', 'my__tool') - expect(result).not.toMatch(/[_-]{2,}/) - }) - }) - - describe('length constraints', () => { - it('should truncate names longer than 63 characters', () => { - const longServerName = 'a'.repeat(50) - const longToolName = 'b'.repeat(50) - const result = buildFunctionCallToolName(longServerName, longToolName, 'id123456') - - expect(result.length).toBeLessThanOrEqual(63) - }) - - it('should not end with underscore or dash after truncation', () => { - const longServerName = 'a'.repeat(50) - const longToolName = 'b'.repeat(50) - const result = buildFunctionCallToolName(longServerName, longToolName, 'id123456') - - expect(result).not.toMatch(/[_-]$/) - }) - - it('should preserve serverId suffix even with long server/tool names', () => { - const longServerName = 'a'.repeat(50) - const longToolName = 'b'.repeat(50) - const serverId = 'server-id-xyz789' - - const result = buildFunctionCallToolName(longServerName, longToolName, serverId) - - // The suffix should be preserved and not truncated - expect(result).toContain('xyz789') - expect(result.length).toBeLessThanOrEqual(63) - }) - - it('should ensure two long-named servers with different IDs produce different results', () => { - const longServerName = 'a'.repeat(50) - const longToolName = 'b'.repeat(50) - const serverId1 = 'server-id-abc123' - const serverId2 = 'server-id-def456' - - const result1 = buildFunctionCallToolName(longServerName, longToolName, serverId1) - const result2 = buildFunctionCallToolName(longServerName, longToolName, serverId2) - - // Both should be within limit - expect(result1.length).toBeLessThanOrEqual(63) - expect(result2.length).toBeLessThanOrEqual(63) - - // They should be different due to preserved suffix expect(result1).not.toBe(result2) - }) - }) - - describe('edge cases with serverId', () => { - it('should handle serverId with only non-alphanumeric characters', () => { - const serverId = '------' // All dashes - const result = buildFunctionCallToolName('server', 'tool', serverId) - - // Should still produce a valid unique suffix via fallback hash - expect(result).toBeTruthy() - expect(result.length).toBeLessThanOrEqual(63) - expect(result).toMatch(/^[a-zA-Z][a-zA-Z0-9_-]*$/) - // Should have a suffix (underscore followed by something) - expect(result).toMatch(/_[a-z0-9]+$/) - }) - - it('should produce different results for different non-alphanumeric serverIds', () => { - const serverId1 = '------' - const serverId2 = '!!!!!!' - - const result1 = buildFunctionCallToolName('server', 'tool', serverId1) - const result2 = buildFunctionCallToolName('server', 'tool', serverId2) - - // Should be different because the hash fallback produces different values - expect(result1).not.toBe(result2) - }) - - it('should handle empty string serverId differently from undefined', () => { - const resultWithEmpty = buildFunctionCallToolName('server', 'tool', '') - const resultWithUndefined = buildFunctionCallToolName('server', 'tool', undefined) - - // Empty string is falsy, so both should behave the same (no suffix) - expect(resultWithEmpty).toBe(resultWithUndefined) - }) - - it('should handle serverId with mixed alphanumeric and special chars', () => { - const serverId = 'ab@#cd' // Mixed chars, last 6 chars contain some alphanumeric - const result = buildFunctionCallToolName('server', 'tool', serverId) - - // Should extract alphanumeric chars: 'abcd' from 'ab@#cd' - expect(result).toContain('abcd') + expect(result3).not.toBe(result4) }) }) describe('real-world scenarios', () => { - it('should handle GitHub MCP server instances correctly', () => { - const serverName = 'github' - const toolName = 'search_repositories' - - const githubComId = 'server-github-com-abc123' - const gheId = 'server-ghe-internal-xyz789' - - const tool1 = buildFunctionCallToolName(serverName, toolName, githubComId) - const tool2 = buildFunctionCallToolName(serverName, toolName, gheId) - - // Should be different - expect(tool1).not.toBe(tool2) - - // Both should be valid identifiers - expect(tool1).toMatch(/^[a-zA-Z][a-zA-Z0-9_-]*$/) - expect(tool2).toMatch(/^[a-zA-Z][a-zA-Z0-9_-]*$/) - - // Both should be <= 63 chars - expect(tool1.length).toBeLessThanOrEqual(63) - expect(tool2.length).toBeLessThanOrEqual(63) + it('should handle GitHub MCP server', () => { + expect(buildFunctionCallToolName('github', 'create_issue')).toBe('mcp__github__create_issue') + expect(buildFunctionCallToolName('github', 'search_repositories')).toBe('mcp__github__search_repositories') + expect(buildFunctionCallToolName('github', 'get_pull_request')).toBe('mcp__github__get_pull_request') }) - it('should handle tool names that already include server name prefix', () => { - const result = buildFunctionCallToolName('github', 'github_search_repos') - expect(result).toBeTruthy() - // Should not double the server name - expect(result.split('github').length - 1).toBeLessThanOrEqual(2) + it('should handle filesystem MCP server', () => { + expect(buildFunctionCallToolName('filesystem', 'read_file')).toBe('mcp__filesystem__read_file') + expect(buildFunctionCallToolName('filesystem', 'write_file')).toBe('mcp__filesystem__write_file') + expect(buildFunctionCallToolName('filesystem', 'list_directory')).toBe('mcp__filesystem__list_directory') + }) + + it('should handle hyphenated server names (common in npm packages)', () => { + expect(buildFunctionCallToolName('cherry-fetch', 'get_page')).toBe('mcp__cherry_fetch__get_page') + expect(buildFunctionCallToolName('mcp-server-github', 'search')).toBe('mcp__mcp_server_github__search') + }) + + it('should handle scoped npm package style names', () => { + const result = buildFunctionCallToolName('@anthropic/mcp-server', 'chat') + expect(result).toBe('mcp__anthropic_mcp_server__chat') + }) + + it('should handle tools with long descriptive names', () => { + const result = buildFunctionCallToolName('github', 'search_repositories_by_language_and_stars') + expect(result.length).toBeLessThanOrEqual(63) + expect(result).toMatch(/^mcp__github__search_repositories_by_lan/) }) }) }) diff --git a/src/main/utils/mcp.ts b/src/main/utils/mcp.ts index cfa700f2e6..34eb0e63e7 100644 --- a/src/main/utils/mcp.ts +++ b/src/main/utils/mcp.ts @@ -1,56 +1,28 @@ -export function buildFunctionCallToolName(serverName: string, toolName: string, serverId?: string) { - const sanitizedServer = serverName.trim().replace(/-/g, '_') - const sanitizedTool = toolName.trim().replace(/-/g, '_') +/** + * Builds a valid JavaScript function name for MCP tool calls. + * Format: mcp__{server_name}__{tool_name} + * + * @param serverName - The MCP server name + * @param toolName - The tool name from the server + * @returns A valid JS identifier in format mcp__{server}__{tool}, max 63 chars + */ +export function buildFunctionCallToolName(serverName: string, toolName: string): string { + // Sanitize to valid JS identifier chars (alphanumeric + underscore only) + const sanitize = (str: string): string => + str + .trim() + .replace(/[^a-zA-Z0-9]/g, '_') // Replace all non-alphanumeric with underscore + .replace(/_{2,}/g, '_') // Collapse multiple underscores + .replace(/^_+|_+$/g, '') // Trim leading/trailing underscores - // Calculate suffix first to reserve space for it - // Suffix format: "_" + 6 alphanumeric chars = 7 chars total - let serverIdSuffix = '' - if (serverId) { - // Take the last 6 characters of the serverId for brevity - serverIdSuffix = serverId.slice(-6).replace(/[^a-zA-Z0-9]/g, '') + const server = sanitize(serverName).slice(0, 20) // Keep server name short + const tool = sanitize(toolName).slice(0, 35) // More room for tool name - // Fallback: if suffix becomes empty (all non-alphanumeric chars), use a simple hash - if (!serverIdSuffix) { - const hash = serverId.split('').reduce((acc, char) => acc + char.charCodeAt(0), 0) - serverIdSuffix = hash.toString(36).slice(-6) || 'x' - } - } + let name = `mcp__${server}__${tool}` - // Reserve space for suffix when calculating max base name length - const SUFFIX_LENGTH = serverIdSuffix ? serverIdSuffix.length + 1 : 0 // +1 for underscore - const MAX_BASE_LENGTH = 63 - SUFFIX_LENGTH - - // Combine server name and tool name - let name = sanitizedTool - if (!sanitizedTool.includes(sanitizedServer.slice(0, 7))) { - name = `${sanitizedServer.slice(0, 7) || ''}-${sanitizedTool || ''}` - } - - // Replace invalid characters with underscores or dashes - // Keep a-z, A-Z, 0-9, underscores and dashes - name = name.replace(/[^a-zA-Z0-9_-]/g, '_') - - // Ensure name starts with a letter or underscore (for valid JavaScript identifier) - if (!/^[a-zA-Z]/.test(name)) { - name = `tool-${name}` - } - - // Remove consecutive underscores/dashes (optional improvement) - name = name.replace(/[_-]{2,}/g, '_') - - // Truncate base name BEFORE adding suffix to ensure suffix is never cut off - if (name.length > MAX_BASE_LENGTH) { - name = name.slice(0, MAX_BASE_LENGTH) - } - - // Handle edge case: ensure we still have a valid name if truncation left invalid chars at edges - if (name.endsWith('_') || name.endsWith('-')) { - name = name.slice(0, -1) - } - - // Now append the suffix - it will always fit within 63 chars - if (serverIdSuffix) { - name = `${name}_${serverIdSuffix}` + // Ensure max 63 chars and clean trailing underscores + if (name.length > 63) { + name = name.slice(0, 63).replace(/_+$/, '') } return name