diff --git a/src/main/services/MCPService.ts b/src/main/services/MCPService.ts index 3831d0af1e..0b8db73930 100644 --- a/src/main/services/MCPService.ts +++ b/src/main/services/MCPService.ts @@ -620,7 +620,7 @@ class McpService { tools.map((tool: SDKTool) => { const serverTool: MCPTool = { ...tool, - id: buildFunctionCallToolName(server.name, tool.name), + id: buildFunctionCallToolName(server.name, tool.name, server.id), 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 new file mode 100644 index 0000000000..b1a35f925e --- /dev/null +++ b/src/main/utils/__tests__/mcp.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, it } from 'vitest' + +import { buildFunctionCallToolName } from '../mcp' + +describe('buildFunctionCallToolName', () => { + describe('basic functionality', () => { + it('should combine server name and tool name', () => { + const result = buildFunctionCallToolName('github', 'search_issues') + expect(result).toContain('github') + expect(result).toContain('search') + }) + + 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() + }) + }) + + 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') + }) + + it('should generate same ID when serverId is not provided', () => { + const serverName = 'github' + const toolName = 'search_repos' + + const result1 = buildFunctionCallToolName(serverName, toolName) + const result2 = buildFunctionCallToolName(serverName, toolName) + + expect(result1).toBe(result2) + }) + + it('should include serverId suffix when provided', () => { + const serverId = 'abc123def456' + const result = buildFunctionCallToolName('server', 'tool', serverId) + + // 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') + }) + }) + + 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 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) + }) + }) +}) diff --git a/src/main/utils/mcp.ts b/src/main/utils/mcp.ts index 23d19806d9..cfa700f2e6 100644 --- a/src/main/utils/mcp.ts +++ b/src/main/utils/mcp.ts @@ -1,7 +1,25 @@ -export function buildFunctionCallToolName(serverName: string, toolName: string) { +export function buildFunctionCallToolName(serverName: string, toolName: string, serverId?: string) { const sanitizedServer = serverName.trim().replace(/-/g, '_') const sanitizedTool = toolName.trim().replace(/-/g, '_') + // 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, '') + + // 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' + } + } + + // 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))) { @@ -20,9 +38,9 @@ export function buildFunctionCallToolName(serverName: string, toolName: string) // Remove consecutive underscores/dashes (optional improvement) name = name.replace(/[_-]{2,}/g, '_') - // Truncate to 63 characters maximum - if (name.length > 63) { - name = name.slice(0, 63) + // 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 @@ -30,5 +48,10 @@ export function buildFunctionCallToolName(serverName: string, toolName: string) name = name.slice(0, -1) } + // Now append the suffix - it will always fit within 63 chars + if (serverIdSuffix) { + name = `${name}_${serverIdSuffix}` + } + return name }