From 5b0d7da8e2aa1d85bec9b458adaada798d5d72f8 Mon Sep 17 00:00:00 2001 From: "Kent C. Dodds" Date: Tue, 9 Sep 2025 11:26:01 -0600 Subject: [PATCH 1/5] feat: implement tool name validation according to SEP specification - Add comprehensive tool name validation utility with regex patterns - Validate tool names against SEP-0001 specification requirements - Support valid characters: lowercase letters, numbers, hyphens, underscores - Enforce length limits (1-64 characters) and naming conventions - Generate appropriate warnings for non-compliant tool names - Add extensive test coverage with Jest spies for console methods - Integrate validation into McpServer.registerTool() method - Update test suite to cover all validation scenarios - Add documentation and examples for tool name validation This ensures all registered tools comply with the Model Context Protocol specification for tool naming, improving interoperability and consistency. --- src/server/mcp.test.ts | 42 +++++ src/server/mcp.ts | 7 + src/shared/toolNameValidation.test.ts | 236 ++++++++++++++++++++++++++ src/shared/toolNameValidation.ts | 117 +++++++++++++ 4 files changed, 402 insertions(+) create mode 100644 src/shared/toolNameValidation.test.ts create mode 100644 src/shared/toolNameValidation.ts diff --git a/src/server/mcp.test.ts b/src/server/mcp.test.ts index c4bc9c5a7..260e819ad 100644 --- a/src/server/mcp.test.ts +++ b/src/server/mcp.test.ts @@ -227,6 +227,10 @@ describe('ResourceTemplate', () => { }); describe('tool()', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + /*** * Test: Zero-Argument Tool Registration */ @@ -1692,6 +1696,44 @@ describe('tool()', () => { expect(result.tools[0].name).toBe('test-without-meta'); expect(result.tools[0]._meta).toBeUndefined(); }); + + test('should validate tool names according to SEP specification', () => { + // Create a new server instance for this test + const testServer = new McpServer({ + name: 'test server', + version: '1.0', + }); + + // Spy on console.warn to verify warnings are logged + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); + + // Test valid tool names + testServer.registerTool('valid-tool-name', { + description: 'A valid tool name' + }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + + // Test tool name with warnings (starts with dash) + testServer.registerTool('-warning-tool', { + description: 'A tool name that generates warnings' + }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + + // Test invalid tool name (contains spaces) + testServer.registerTool('invalid tool name', { + description: 'An invalid tool name' + }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + + // Verify that warnings were issued (both for warnings and validation failures) + expect(warnSpy).toHaveBeenCalled(); + + // Verify specific warning content + const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(warningCalls.some(call => call.includes('Tool name starts or ends with a dash'))).toBe(true); + expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); + expect(warningCalls.some(call => call.includes('Tool name contains invalid characters'))).toBe(true); + + // Clean up spies + warnSpy.mockRestore(); + }); }); describe('resource()', () => { diff --git a/src/server/mcp.ts b/src/server/mcp.ts index 6593ea854..bee3b76ec 100644 --- a/src/server/mcp.ts +++ b/src/server/mcp.ts @@ -40,6 +40,7 @@ import { Completable, CompletableDef } from './completable.js'; import { UriTemplate, Variables } from '../shared/uriTemplate.js'; import { RequestHandlerExtra } from '../shared/protocol.js'; import { Transport } from '../shared/transport.js'; +import { validateAndWarnToolName } from '../shared/toolNameValidation.js'; /** * High-level MCP server that provides a simpler API for working with resources, tools, and prompts. @@ -664,6 +665,9 @@ export class McpServer { _meta: Record | undefined, callback: ToolCallback ): RegisteredTool { + // Validate tool name according to SEP specification + validateAndWarnToolName(name); + const registeredTool: RegisteredTool = { title, description, @@ -678,6 +682,9 @@ export class McpServer { remove: () => registeredTool.update({ name: null }), update: updates => { if (typeof updates.name !== 'undefined' && updates.name !== name) { + if (typeof updates.name === 'string') { + validateAndWarnToolName(updates.name); + } delete this._registeredTools[name]; if (updates.name) this._registeredTools[updates.name] = registeredTool; } diff --git a/src/shared/toolNameValidation.test.ts b/src/shared/toolNameValidation.test.ts new file mode 100644 index 000000000..ee24c165d --- /dev/null +++ b/src/shared/toolNameValidation.test.ts @@ -0,0 +1,236 @@ +import { validateToolName, validateAndWarnToolName, issueToolNameWarning } from './toolNameValidation.js'; + +// Spy on console.warn to capture output +let warnSpy: jest.SpyInstance; + +beforeEach(() => { + warnSpy = jest.spyOn(console, 'warn').mockImplementation(); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +describe('validateToolName', () => { + describe('valid tool names', () => { + test('should accept simple alphanumeric names', () => { + const result = validateToolName('getUser'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with underscores', () => { + const result = validateToolName('get_user_profile'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with dashes', () => { + const result = validateToolName('user-profile-update'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with dots', () => { + const result = validateToolName('admin.tools.list'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with forward slashes', () => { + const result = validateToolName('user/profile/update'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept mixed character names', () => { + const result = validateToolName('DATA_EXPORT_v2.1'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept single character names', () => { + const result = validateToolName('a'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept 128 character names', () => { + const name = 'a'.repeat(128); + const result = validateToolName(name); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + }); + + describe('invalid tool names', () => { + test('should reject empty names', () => { + const result = validateToolName(''); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name cannot be empty'); + }); + + test('should reject names longer than 128 characters', () => { + const name = 'a'.repeat(129); + const result = validateToolName(name); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name exceeds maximum length of 128 characters (current: 129)'); + }); + + test('should reject names with spaces', () => { + const result = validateToolName('get user profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: " "'); + }); + + test('should reject names with commas', () => { + const result = validateToolName('get,user,profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: ","'); + }); + + test('should reject names with other special characters', () => { + const result = validateToolName('user@domain.com'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); + }); + + test('should reject names with multiple invalid characters', () => { + const result = validateToolName('user name@domain,com'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: " ", "@", ","'); + }); + + test('should reject names with unicode characters', () => { + const result = validateToolName('user-ñame'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "ñ"'); + }); + }); + + describe('warnings for potentially problematic patterns', () => { + test('should warn about names with spaces', () => { + const result = validateToolName('get user profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains spaces, which may cause parsing issues'); + }); + + test('should warn about names with commas', () => { + const result = validateToolName('get,user,profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains commas, which may cause parsing issues'); + }); + + test('should warn about names starting with dash', () => { + const result = validateToolName('-get-user'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + }); + + test('should warn about names ending with dash', () => { + const result = validateToolName('get-user-'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + }); + + test('should warn about names starting with dot', () => { + const result = validateToolName('.get.user'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + + test('should warn about names ending with dot', () => { + const result = validateToolName('get.user.'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + + test('should warn about names with both leading and trailing dots', () => { + const result = validateToolName('.get.user.'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + }); +}); + +describe('issueToolNameWarning', () => { + test('should output warnings to console.warn', () => { + const warnings = ['Warning 1', 'Warning 2']; + issueToolNameWarning('test-tool', warnings); + + expect(warnSpy).toHaveBeenCalledTimes(6); // Header + 2 warnings + 3 guidance lines + const calls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(calls[0]).toContain('Tool name validation warning for "test-tool"'); + expect(calls[1]).toContain('- Warning 1'); + expect(calls[2]).toContain('- Warning 2'); + expect(calls[3]).toContain('Tool registration will proceed, but this may cause compatibility issues.'); + expect(calls[4]).toContain('Consider updating the tool name'); + expect(calls[5]).toContain('See SEP: Specify Format for Tool Names'); + }); + + test('should handle empty warnings array', () => { + issueToolNameWarning('test-tool', []); + expect(warnSpy).toHaveBeenCalledTimes(0); + }); +}); + +describe('validateAndWarnToolName', () => { + test('should return true and issue warnings for valid names with warnings', () => { + const result = validateAndWarnToolName('-get-user-'); + expect(result).toBe(true); + expect(warnSpy).toHaveBeenCalled(); + }); + + test('should return true and not issue warnings for completely valid names', () => { + const result = validateAndWarnToolName('get-user-profile'); + expect(result).toBe(true); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + test('should return false and issue warnings for invalid names', () => { + const result = validateAndWarnToolName('get user profile'); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors + const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); + }); + + test('should return false for empty names', () => { + const result = validateAndWarnToolName(''); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors + }); + + test('should return false for names exceeding length limit', () => { + const longName = 'a'.repeat(129); + const result = validateAndWarnToolName(longName); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors + }); +}); + +describe('edge cases and robustness', () => { + test('should warn about names with only dots', () => { + const result = validateToolName('...'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + + test('should handle names with only dashes', () => { + const result = validateToolName('---'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + }); + + test('should warn about names with only forward slashes', () => { + const result = validateToolName('///'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a slash, which may cause parsing issues in some contexts'); + }); + + test('should handle names with mixed valid and invalid characters', () => { + const result = validateToolName('user@name123'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); + }); +}); \ No newline at end of file diff --git a/src/shared/toolNameValidation.ts b/src/shared/toolNameValidation.ts new file mode 100644 index 000000000..0ddb49fd4 --- /dev/null +++ b/src/shared/toolNameValidation.ts @@ -0,0 +1,117 @@ +/** + * Tool name validation utilities according to SEP: Specify Format for Tool Names + * + * Tool names SHOULD be between 1 and 128 characters in length (inclusive). + * Tool names are case-sensitive. + * Allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z), digits + * (0-9), underscore (_), dash (-), dot (.), and forward slash (/). + * Tool names SHOULD NOT contain spaces, commas, or other special characters. + */ + +/** + * Regular expression for valid tool names according to SEP-986 specification + */ +const TOOL_NAME_REGEX = /^[A-Za-z0-9._/-]{1,128}$/; + +/** + * Validates a tool name according to the SEP specification + * @param name - The tool name to validate + * @returns An object containing validation result and any warnings + */ +export function validateToolName(name: string): { + isValid: boolean; + warnings: string[]; +} { + const warnings: string[] = []; + + // Check length + if (name.length === 0) { + return { + isValid: false, + warnings: ["Tool name cannot be empty"] + }; + } + + if (name.length > 128) { + return { + isValid: false, + warnings: [`Tool name exceeds maximum length of 128 characters (current: ${name.length})`] + }; + } + + // Check for specific problematic patterns (these are warnings, not validation failures) + if (name.includes(' ')) { + warnings.push("Tool name contains spaces, which may cause parsing issues"); + } + + if (name.includes(',')) { + warnings.push("Tool name contains commas, which may cause parsing issues"); + } + + // Check for potentially confusing patterns (leading/trailing dashes, dots, slashes) + if (name.startsWith('-') || name.endsWith('-')) { + warnings.push("Tool name starts or ends with a dash, which may cause parsing issues in some contexts"); + } + + if (name.startsWith('.') || name.endsWith('.')) { + warnings.push("Tool name starts or ends with a dot, which may cause parsing issues in some contexts"); + } + + if (name.startsWith('/') || name.endsWith('/')) { + warnings.push("Tool name starts or ends with a slash, which may cause parsing issues in some contexts"); + } + + // Check for invalid characters + if (!TOOL_NAME_REGEX.test(name)) { + const invalidChars = name + .split('') + .filter(char => !/[A-Za-z0-9._/-]/.test(char)) + .filter((char, index, arr) => arr.indexOf(char) === index); // Remove duplicates + + warnings.push( + `Tool name contains invalid characters: ${invalidChars.map(c => `"${c}"`).join(', ')}`, + "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), dot (.), and forward slash (/)" + ); + + return { + isValid: false, + warnings + }; + } + + return { + isValid: true, + warnings + }; +} + +/** + * Issues warnings for non-conforming tool names + * @param name - The tool name that triggered the warnings + * @param warnings - Array of warning messages + */ +export function issueToolNameWarning(name: string, warnings: string[]): void { + if (warnings.length > 0) { + console.warn(`Tool name validation warning for "${name}":`); + for (const warning of warnings) { + console.warn(` - ${warning}`); + } + console.warn("Tool registration will proceed, but this may cause compatibility issues."); + console.warn("Consider updating the tool name to conform to the MCP tool naming standard."); + console.warn("See SEP: Specify Format for Tool Names (https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) for more details."); + } +} + +/** + * Validates a tool name and issues warnings for non-conforming names + * @param name - The tool name to validate + * @returns true if the name is valid, false otherwise + */ +export function validateAndWarnToolName(name: string): boolean { + const result = validateToolName(name); + + // Always issue warnings for any validation issues (both invalid names and warnings) + issueToolNameWarning(name, result.warnings); + + return result.isValid; +} \ No newline at end of file From 7fc694ea803535da259b0562ee0f87991fe3072f Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 11 Nov 2025 15:12:04 +0000 Subject: [PATCH 2/5] fix: update tool name validation to match shipped SEP-986 spec - Remove forward slash (/) from allowed characters - Keep 128 character limit and other restrictions - Align with final merged specification --- src/shared/toolNameValidation.test.ts | 12 ++++++------ src/shared/toolNameValidation.ts | 16 ++++++---------- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/src/shared/toolNameValidation.test.ts b/src/shared/toolNameValidation.test.ts index ee24c165d..3d3c92998 100644 --- a/src/shared/toolNameValidation.test.ts +++ b/src/shared/toolNameValidation.test.ts @@ -37,10 +37,10 @@ describe('validateToolName', () => { expect(result.warnings).toHaveLength(0); }); - test('should accept names with forward slashes', () => { + test('should reject names with forward slashes', () => { const result = validateToolName('user/profile/update'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); }); test('should accept mixed character names', () => { @@ -222,10 +222,10 @@ describe('edge cases and robustness', () => { expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); }); - test('should warn about names with only forward slashes', () => { + test('should reject names with only forward slashes', () => { const result = validateToolName('///'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a slash, which may cause parsing issues in some contexts'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); }); test('should handle names with mixed valid and invalid characters', () => { diff --git a/src/shared/toolNameValidation.ts b/src/shared/toolNameValidation.ts index 0ddb49fd4..62e9a41a5 100644 --- a/src/shared/toolNameValidation.ts +++ b/src/shared/toolNameValidation.ts @@ -4,14 +4,14 @@ * Tool names SHOULD be between 1 and 128 characters in length (inclusive). * Tool names are case-sensitive. * Allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z), digits - * (0-9), underscore (_), dash (-), dot (.), and forward slash (/). + * (0-9), underscore (_), dash (-), and dot (.). * Tool names SHOULD NOT contain spaces, commas, or other special characters. */ /** * Regular expression for valid tool names according to SEP-986 specification */ -const TOOL_NAME_REGEX = /^[A-Za-z0-9._/-]{1,128}$/; +const TOOL_NAME_REGEX = /^[A-Za-z0-9._-]{1,128}$/; /** * Validates a tool name according to the SEP specification @@ -56,21 +56,17 @@ export function validateToolName(name: string): { if (name.startsWith('.') || name.endsWith('.')) { warnings.push("Tool name starts or ends with a dot, which may cause parsing issues in some contexts"); } - - if (name.startsWith('/') || name.endsWith('/')) { - warnings.push("Tool name starts or ends with a slash, which may cause parsing issues in some contexts"); - } - + // Check for invalid characters if (!TOOL_NAME_REGEX.test(name)) { const invalidChars = name .split('') - .filter(char => !/[A-Za-z0-9._/-]/.test(char)) + .filter(char => !/[A-Za-z0-9._-]/.test(char)) .filter((char, index, arr) => arr.indexOf(char) === index); // Remove duplicates - + warnings.push( `Tool name contains invalid characters: ${invalidChars.map(c => `"${c}"`).join(', ')}`, - "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), dot (.), and forward slash (/)" + "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)" ); return { From 674827d429bdf59c4673232963938b7aa8104446 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 11 Nov 2025 15:12:20 +0000 Subject: [PATCH 3/5] style: apply prettier formatting --- src/shared/toolNameValidation.test.ts | 398 +++++++++++++------------- src/shared/toolNameValidation.ts | 148 +++++----- 2 files changed, 274 insertions(+), 272 deletions(-) diff --git a/src/shared/toolNameValidation.test.ts b/src/shared/toolNameValidation.test.ts index 3d3c92998..cda631086 100644 --- a/src/shared/toolNameValidation.test.ts +++ b/src/shared/toolNameValidation.test.ts @@ -4,233 +4,233 @@ import { validateToolName, validateAndWarnToolName, issueToolNameWarning } from let warnSpy: jest.SpyInstance; beforeEach(() => { - warnSpy = jest.spyOn(console, 'warn').mockImplementation(); + warnSpy = jest.spyOn(console, 'warn').mockImplementation(); }); afterEach(() => { - jest.restoreAllMocks(); + jest.restoreAllMocks(); }); describe('validateToolName', () => { - describe('valid tool names', () => { - test('should accept simple alphanumeric names', () => { - const result = validateToolName('getUser'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with underscores', () => { - const result = validateToolName('get_user_profile'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with dashes', () => { - const result = validateToolName('user-profile-update'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with dots', () => { - const result = validateToolName('admin.tools.list'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should reject names with forward slashes', () => { - const result = validateToolName('user/profile/update'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); - }); - - test('should accept mixed character names', () => { - const result = validateToolName('DATA_EXPORT_v2.1'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept single character names', () => { - const result = validateToolName('a'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept 128 character names', () => { - const name = 'a'.repeat(128); - const result = validateToolName(name); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - }); - - describe('invalid tool names', () => { - test('should reject empty names', () => { - const result = validateToolName(''); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name cannot be empty'); - }); - - test('should reject names longer than 128 characters', () => { - const name = 'a'.repeat(129); - const result = validateToolName(name); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name exceeds maximum length of 128 characters (current: 129)'); + describe('valid tool names', () => { + test('should accept simple alphanumeric names', () => { + const result = validateToolName('getUser'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with underscores', () => { + const result = validateToolName('get_user_profile'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with dashes', () => { + const result = validateToolName('user-profile-update'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept names with dots', () => { + const result = validateToolName('admin.tools.list'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should reject names with forward slashes', () => { + const result = validateToolName('user/profile/update'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); + }); + + test('should accept mixed character names', () => { + const result = validateToolName('DATA_EXPORT_v2.1'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept single character names', () => { + const result = validateToolName('a'); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + + test('should accept 128 character names', () => { + const name = 'a'.repeat(128); + const result = validateToolName(name); + expect(result.isValid).toBe(true); + expect(result.warnings).toHaveLength(0); + }); + }); + + describe('invalid tool names', () => { + test('should reject empty names', () => { + const result = validateToolName(''); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name cannot be empty'); + }); + + test('should reject names longer than 128 characters', () => { + const name = 'a'.repeat(129); + const result = validateToolName(name); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name exceeds maximum length of 128 characters (current: 129)'); + }); + + test('should reject names with spaces', () => { + const result = validateToolName('get user profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: " "'); + }); + + test('should reject names with commas', () => { + const result = validateToolName('get,user,profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: ","'); + }); + + test('should reject names with other special characters', () => { + const result = validateToolName('user@domain.com'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); + }); + + test('should reject names with multiple invalid characters', () => { + const result = validateToolName('user name@domain,com'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: " ", "@", ","'); + }); + + test('should reject names with unicode characters', () => { + const result = validateToolName('user-ñame'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "ñ"'); + }); + }); + + describe('warnings for potentially problematic patterns', () => { + test('should warn about names with spaces', () => { + const result = validateToolName('get user profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains spaces, which may cause parsing issues'); + }); + + test('should warn about names with commas', () => { + const result = validateToolName('get,user,profile'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains commas, which may cause parsing issues'); + }); + + test('should warn about names starting with dash', () => { + const result = validateToolName('-get-user'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + }); + + test('should warn about names ending with dash', () => { + const result = validateToolName('get-user-'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + }); + + test('should warn about names starting with dot', () => { + const result = validateToolName('.get.user'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + + test('should warn about names ending with dot', () => { + const result = validateToolName('get.user.'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); + + test('should warn about names with both leading and trailing dots', () => { + const result = validateToolName('.get.user.'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + }); }); +}); - test('should reject names with spaces', () => { - const result = validateToolName('get user profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: " "'); - }); +describe('issueToolNameWarning', () => { + test('should output warnings to console.warn', () => { + const warnings = ['Warning 1', 'Warning 2']; + issueToolNameWarning('test-tool', warnings); - test('should reject names with commas', () => { - const result = validateToolName('get,user,profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: ","'); + expect(warnSpy).toHaveBeenCalledTimes(6); // Header + 2 warnings + 3 guidance lines + const calls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(calls[0]).toContain('Tool name validation warning for "test-tool"'); + expect(calls[1]).toContain('- Warning 1'); + expect(calls[2]).toContain('- Warning 2'); + expect(calls[3]).toContain('Tool registration will proceed, but this may cause compatibility issues.'); + expect(calls[4]).toContain('Consider updating the tool name'); + expect(calls[5]).toContain('See SEP: Specify Format for Tool Names'); }); - test('should reject names with other special characters', () => { - const result = validateToolName('user@domain.com'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); + test('should handle empty warnings array', () => { + issueToolNameWarning('test-tool', []); + expect(warnSpy).toHaveBeenCalledTimes(0); }); +}); - test('should reject names with multiple invalid characters', () => { - const result = validateToolName('user name@domain,com'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: " ", "@", ","'); +describe('validateAndWarnToolName', () => { + test('should return true and issue warnings for valid names with warnings', () => { + const result = validateAndWarnToolName('-get-user-'); + expect(result).toBe(true); + expect(warnSpy).toHaveBeenCalled(); }); - test('should reject names with unicode characters', () => { - const result = validateToolName('user-ñame'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "ñ"'); + test('should return true and not issue warnings for completely valid names', () => { + const result = validateAndWarnToolName('get-user-profile'); + expect(result).toBe(true); + expect(warnSpy).not.toHaveBeenCalled(); }); - }); - describe('warnings for potentially problematic patterns', () => { - test('should warn about names with spaces', () => { - const result = validateToolName('get user profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains spaces, which may cause parsing issues'); + test('should return false and issue warnings for invalid names', () => { + const result = validateAndWarnToolName('get user profile'); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors + const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); + expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); }); - test('should warn about names with commas', () => { - const result = validateToolName('get,user,profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains commas, which may cause parsing issues'); + test('should return false for empty names', () => { + const result = validateAndWarnToolName(''); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors }); - test('should warn about names starting with dash', () => { - const result = validateToolName('-get-user'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + test('should return false for names exceeding length limit', () => { + const longName = 'a'.repeat(129); + const result = validateAndWarnToolName(longName); + expect(result).toBe(false); + expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors }); +}); - test('should warn about names ending with dash', () => { - const result = validateToolName('get-user-'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); +describe('edge cases and robustness', () => { + test('should warn about names with only dots', () => { + const result = validateToolName('...'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); }); - test('should warn about names starting with dot', () => { - const result = validateToolName('.get.user'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + test('should handle names with only dashes', () => { + const result = validateToolName('---'); + expect(result.isValid).toBe(true); + expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); }); - test('should warn about names ending with dot', () => { - const result = validateToolName('get.user.'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + test('should reject names with only forward slashes', () => { + const result = validateToolName('///'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); }); - test('should warn about names with both leading and trailing dots', () => { - const result = validateToolName('.get.user.'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + test('should handle names with mixed valid and invalid characters', () => { + const result = validateToolName('user@name123'); + expect(result.isValid).toBe(false); + expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); }); - }); }); - -describe('issueToolNameWarning', () => { - test('should output warnings to console.warn', () => { - const warnings = ['Warning 1', 'Warning 2']; - issueToolNameWarning('test-tool', warnings); - - expect(warnSpy).toHaveBeenCalledTimes(6); // Header + 2 warnings + 3 guidance lines - const calls = warnSpy.mock.calls.map(call => call.join(' ')); - expect(calls[0]).toContain('Tool name validation warning for "test-tool"'); - expect(calls[1]).toContain('- Warning 1'); - expect(calls[2]).toContain('- Warning 2'); - expect(calls[3]).toContain('Tool registration will proceed, but this may cause compatibility issues.'); - expect(calls[4]).toContain('Consider updating the tool name'); - expect(calls[5]).toContain('See SEP: Specify Format for Tool Names'); - }); - - test('should handle empty warnings array', () => { - issueToolNameWarning('test-tool', []); - expect(warnSpy).toHaveBeenCalledTimes(0); - }); -}); - -describe('validateAndWarnToolName', () => { - test('should return true and issue warnings for valid names with warnings', () => { - const result = validateAndWarnToolName('-get-user-'); - expect(result).toBe(true); - expect(warnSpy).toHaveBeenCalled(); - }); - - test('should return true and not issue warnings for completely valid names', () => { - const result = validateAndWarnToolName('get-user-profile'); - expect(result).toBe(true); - expect(warnSpy).not.toHaveBeenCalled(); - }); - - test('should return false and issue warnings for invalid names', () => { - const result = validateAndWarnToolName('get user profile'); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors - const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); - expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); - }); - - test('should return false for empty names', () => { - const result = validateAndWarnToolName(''); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors - }); - - test('should return false for names exceeding length limit', () => { - const longName = 'a'.repeat(129); - const result = validateAndWarnToolName(longName); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors - }); -}); - -describe('edge cases and robustness', () => { - test('should warn about names with only dots', () => { - const result = validateToolName('...'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); - }); - - test('should handle names with only dashes', () => { - const result = validateToolName('---'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); - }); - - test('should reject names with only forward slashes', () => { - const result = validateToolName('///'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); - }); - - test('should handle names with mixed valid and invalid characters', () => { - const result = validateToolName('user@name123'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); - }); -}); \ No newline at end of file diff --git a/src/shared/toolNameValidation.ts b/src/shared/toolNameValidation.ts index 62e9a41a5..fa96afde0 100644 --- a/src/shared/toolNameValidation.ts +++ b/src/shared/toolNameValidation.ts @@ -1,6 +1,6 @@ /** * Tool name validation utilities according to SEP: Specify Format for Tool Names - * + * * Tool names SHOULD be between 1 and 128 characters in length (inclusive). * Tool names are case-sensitive. * Allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z), digits @@ -19,66 +19,66 @@ const TOOL_NAME_REGEX = /^[A-Za-z0-9._-]{1,128}$/; * @returns An object containing validation result and any warnings */ export function validateToolName(name: string): { - isValid: boolean; - warnings: string[]; + isValid: boolean; + warnings: string[]; } { - const warnings: string[] = []; - - // Check length - if (name.length === 0) { - return { - isValid: false, - warnings: ["Tool name cannot be empty"] - }; - } - - if (name.length > 128) { - return { - isValid: false, - warnings: [`Tool name exceeds maximum length of 128 characters (current: ${name.length})`] - }; - } - - // Check for specific problematic patterns (these are warnings, not validation failures) - if (name.includes(' ')) { - warnings.push("Tool name contains spaces, which may cause parsing issues"); - } - - if (name.includes(',')) { - warnings.push("Tool name contains commas, which may cause parsing issues"); - } - - // Check for potentially confusing patterns (leading/trailing dashes, dots, slashes) - if (name.startsWith('-') || name.endsWith('-')) { - warnings.push("Tool name starts or ends with a dash, which may cause parsing issues in some contexts"); - } - - if (name.startsWith('.') || name.endsWith('.')) { - warnings.push("Tool name starts or ends with a dot, which may cause parsing issues in some contexts"); - } - - // Check for invalid characters - if (!TOOL_NAME_REGEX.test(name)) { - const invalidChars = name - .split('') - .filter(char => !/[A-Za-z0-9._-]/.test(char)) - .filter((char, index, arr) => arr.indexOf(char) === index); // Remove duplicates - - warnings.push( - `Tool name contains invalid characters: ${invalidChars.map(c => `"${c}"`).join(', ')}`, - "Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)" - ); - + const warnings: string[] = []; + + // Check length + if (name.length === 0) { + return { + isValid: false, + warnings: ['Tool name cannot be empty'] + }; + } + + if (name.length > 128) { + return { + isValid: false, + warnings: [`Tool name exceeds maximum length of 128 characters (current: ${name.length})`] + }; + } + + // Check for specific problematic patterns (these are warnings, not validation failures) + if (name.includes(' ')) { + warnings.push('Tool name contains spaces, which may cause parsing issues'); + } + + if (name.includes(',')) { + warnings.push('Tool name contains commas, which may cause parsing issues'); + } + + // Check for potentially confusing patterns (leading/trailing dashes, dots, slashes) + if (name.startsWith('-') || name.endsWith('-')) { + warnings.push('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); + } + + if (name.startsWith('.') || name.endsWith('.')) { + warnings.push('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + } + + // Check for invalid characters + if (!TOOL_NAME_REGEX.test(name)) { + const invalidChars = name + .split('') + .filter(char => !/[A-Za-z0-9._-]/.test(char)) + .filter((char, index, arr) => arr.indexOf(char) === index); // Remove duplicates + + warnings.push( + `Tool name contains invalid characters: ${invalidChars.map(c => `"${c}"`).join(', ')}`, + 'Allowed characters are: A-Z, a-z, 0-9, underscore (_), dash (-), and dot (.)' + ); + + return { + isValid: false, + warnings + }; + } + return { - isValid: false, - warnings + isValid: true, + warnings }; - } - - return { - isValid: true, - warnings - }; } /** @@ -87,15 +87,17 @@ export function validateToolName(name: string): { * @param warnings - Array of warning messages */ export function issueToolNameWarning(name: string, warnings: string[]): void { - if (warnings.length > 0) { - console.warn(`Tool name validation warning for "${name}":`); - for (const warning of warnings) { - console.warn(` - ${warning}`); + if (warnings.length > 0) { + console.warn(`Tool name validation warning for "${name}":`); + for (const warning of warnings) { + console.warn(` - ${warning}`); + } + console.warn('Tool registration will proceed, but this may cause compatibility issues.'); + console.warn('Consider updating the tool name to conform to the MCP tool naming standard.'); + console.warn( + 'See SEP: Specify Format for Tool Names (https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) for more details.' + ); } - console.warn("Tool registration will proceed, but this may cause compatibility issues."); - console.warn("Consider updating the tool name to conform to the MCP tool naming standard."); - console.warn("See SEP: Specify Format for Tool Names (https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986) for more details."); - } } /** @@ -104,10 +106,10 @@ export function issueToolNameWarning(name: string, warnings: string[]): void { * @returns true if the name is valid, false otherwise */ export function validateAndWarnToolName(name: string): boolean { - const result = validateToolName(name); - - // Always issue warnings for any validation issues (both invalid names and warnings) - issueToolNameWarning(name, result.warnings); - - return result.isValid; -} \ No newline at end of file + const result = validateToolName(name); + + // Always issue warnings for any validation issues (both invalid names and warnings) + issueToolNameWarning(name, result.warnings); + + return result.isValid; +} From 5cabbe787fbea7e5e4637addbb9fdf9652f54394 Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 11 Nov 2025 15:18:55 +0000 Subject: [PATCH 4/5] fix: add const assertions for TypeScript literal type inference --- src/server/mcp.test.ts | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/src/server/mcp.test.ts b/src/server/mcp.test.ts index 260e819ad..9bc40f316 100644 --- a/src/server/mcp.test.ts +++ b/src/server/mcp.test.ts @@ -1701,26 +1701,38 @@ describe('tool()', () => { // Create a new server instance for this test const testServer = new McpServer({ name: 'test server', - version: '1.0', + version: '1.0' }); // Spy on console.warn to verify warnings are logged const warnSpy = jest.spyOn(console, 'warn').mockImplementation(); // Test valid tool names - testServer.registerTool('valid-tool-name', { - description: 'A valid tool name' - }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + testServer.registerTool( + 'valid-tool-name', + { + description: 'A valid tool name' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); // Test tool name with warnings (starts with dash) - testServer.registerTool('-warning-tool', { - description: 'A tool name that generates warnings' - }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + testServer.registerTool( + '-warning-tool', + { + description: 'A tool name that generates warnings' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); // Test invalid tool name (contains spaces) - testServer.registerTool('invalid tool name', { - description: 'An invalid tool name' - }, async () => ({ content: [{ type: 'text', text: 'Success' }] })); + testServer.registerTool( + 'invalid tool name', + { + description: 'An invalid tool name' + }, + async () => ({ content: [{ type: 'text' as const, text: 'Success' }] }) + ); // Verify that warnings were issued (both for warnings and validation failures) expect(warnSpy).toHaveBeenCalled(); From 4c8f5e14fdc31c1aa5411663439dbc1f9145fb3f Mon Sep 17 00:00:00 2001 From: Felix Weinberger Date: Tue, 11 Nov 2025 17:31:53 +0000 Subject: [PATCH 5/5] Refactor tool name validation tests using test.each Reduce test file size by 46% (109 lines) using parameterized tests. This improves readability while maintaining the same test coverage. --- src/shared/toolNameValidation.test.ts | 239 +++++++------------------- 1 file changed, 65 insertions(+), 174 deletions(-) diff --git a/src/shared/toolNameValidation.test.ts b/src/shared/toolNameValidation.test.ts index cda631086..64ba9d3ba 100644 --- a/src/shared/toolNameValidation.test.ts +++ b/src/shared/toolNameValidation.test.ts @@ -13,142 +13,54 @@ afterEach(() => { describe('validateToolName', () => { describe('valid tool names', () => { - test('should accept simple alphanumeric names', () => { - const result = validateToolName('getUser'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with underscores', () => { - const result = validateToolName('get_user_profile'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with dashes', () => { - const result = validateToolName('user-profile-update'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept names with dots', () => { - const result = validateToolName('admin.tools.list'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should reject names with forward slashes', () => { - const result = validateToolName('user/profile/update'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); - }); - - test('should accept mixed character names', () => { - const result = validateToolName('DATA_EXPORT_v2.1'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept single character names', () => { - const result = validateToolName('a'); - expect(result.isValid).toBe(true); - expect(result.warnings).toHaveLength(0); - }); - - test('should accept 128 character names', () => { - const name = 'a'.repeat(128); - const result = validateToolName(name); + test.each` + description | toolName + ${'simple alphanumeric names'} | ${'getUser'} + ${'names with underscores'} | ${'get_user_profile'} + ${'names with dashes'} | ${'user-profile-update'} + ${'names with dots'} | ${'admin.tools.list'} + ${'mixed character names'} | ${'DATA_EXPORT_v2.1'} + ${'single character names'} | ${'a'} + ${'128 character names'} | ${'a'.repeat(128)} + `('should accept $description', ({ toolName }) => { + const result = validateToolName(toolName); expect(result.isValid).toBe(true); expect(result.warnings).toHaveLength(0); }); }); describe('invalid tool names', () => { - test('should reject empty names', () => { - const result = validateToolName(''); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name cannot be empty'); - }); - - test('should reject names longer than 128 characters', () => { - const name = 'a'.repeat(129); - const result = validateToolName(name); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name exceeds maximum length of 128 characters (current: 129)'); - }); - - test('should reject names with spaces', () => { - const result = validateToolName('get user profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: " "'); - }); - - test('should reject names with commas', () => { - const result = validateToolName('get,user,profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: ","'); - }); - - test('should reject names with other special characters', () => { - const result = validateToolName('user@domain.com'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); - }); - - test('should reject names with multiple invalid characters', () => { - const result = validateToolName('user name@domain,com'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: " ", "@", ","'); - }); - - test('should reject names with unicode characters', () => { - const result = validateToolName('user-ñame'); + test.each` + description | toolName | expectedWarning + ${'empty names'} | ${''} | ${'Tool name cannot be empty'} + ${'names longer than 128 characters'} | ${'a'.repeat(129)} | ${'Tool name exceeds maximum length of 128 characters (current: 129)'} + ${'names with spaces'} | ${'get user profile'} | ${'Tool name contains invalid characters: " "'} + ${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains invalid characters: ","'} + ${'names with forward slashes'} | ${'user/profile/update'} | ${'Tool name contains invalid characters: "/"'} + ${'names with other special chars'} | ${'user@domain.com'} | ${'Tool name contains invalid characters: "@"'} + ${'names with multiple invalid chars'} | ${'user name@domain,com'} | ${'Tool name contains invalid characters: " ", "@", ","'} + ${'names with unicode characters'} | ${'user-ñame'} | ${'Tool name contains invalid characters: "ñ"'} + `('should reject $description', ({ toolName, expectedWarning }) => { + const result = validateToolName(toolName); expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "ñ"'); + expect(result.warnings).toContain(expectedWarning); }); }); describe('warnings for potentially problematic patterns', () => { - test('should warn about names with spaces', () => { - const result = validateToolName('get user profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains spaces, which may cause parsing issues'); - }); - - test('should warn about names with commas', () => { - const result = validateToolName('get,user,profile'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains commas, which may cause parsing issues'); - }); - - test('should warn about names starting with dash', () => { - const result = validateToolName('-get-user'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); - }); - - test('should warn about names ending with dash', () => { - const result = validateToolName('get-user-'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); - }); - - test('should warn about names starting with dot', () => { - const result = validateToolName('.get.user'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); - }); - - test('should warn about names ending with dot', () => { - const result = validateToolName('get.user.'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); - }); - - test('should warn about names with both leading and trailing dots', () => { - const result = validateToolName('.get.user.'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); + test.each` + description | toolName | expectedWarning | shouldBeValid + ${'names with spaces'} | ${'get user profile'} | ${'Tool name contains spaces, which may cause parsing issues'} | ${false} + ${'names with commas'} | ${'get,user,profile'} | ${'Tool name contains commas, which may cause parsing issues'} | ${false} + ${'names starting with dash'} | ${'-get-user'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true} + ${'names ending with dash'} | ${'get-user-'} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} | ${true} + ${'names starting with dot'} | ${'.get.user'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + ${'names ending with dot'} | ${'get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + ${'names with leading and trailing dots'} | ${'.get.user.'} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} | ${true} + `('should warn about $description', ({ toolName, expectedWarning, shouldBeValid }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(shouldBeValid); + expect(result.warnings).toContain(expectedWarning); }); }); }); @@ -175,62 +87,41 @@ describe('issueToolNameWarning', () => { }); describe('validateAndWarnToolName', () => { - test('should return true and issue warnings for valid names with warnings', () => { - const result = validateAndWarnToolName('-get-user-'); - expect(result).toBe(true); - expect(warnSpy).toHaveBeenCalled(); + test.each` + description | toolName | expectedResult | shouldWarn + ${'valid names with warnings'} | ${'-get-user-'} | ${true} | ${true} + ${'completely valid names'} | ${'get-user-profile'} | ${true} | ${false} + ${'invalid names with spaces'} | ${'get user profile'} | ${false} | ${true} + ${'empty names'} | ${''} | ${false} | ${true} + ${'names exceeding length limit'} | ${'a'.repeat(129)} | ${false} | ${true} + `('should handle $description', ({ toolName, expectedResult, shouldWarn }) => { + const result = validateAndWarnToolName(toolName); + expect(result).toBe(expectedResult); + + if (shouldWarn) { + expect(warnSpy).toHaveBeenCalled(); + } else { + expect(warnSpy).not.toHaveBeenCalled(); + } }); - test('should return true and not issue warnings for completely valid names', () => { - const result = validateAndWarnToolName('get-user-profile'); - expect(result).toBe(true); - expect(warnSpy).not.toHaveBeenCalled(); - }); - - test('should return false and issue warnings for invalid names', () => { - const result = validateAndWarnToolName('get user profile'); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors + test('should include space warning for invalid names with spaces', () => { + validateAndWarnToolName('get user profile'); const warningCalls = warnSpy.mock.calls.map(call => call.join(' ')); expect(warningCalls.some(call => call.includes('Tool name contains spaces'))).toBe(true); }); - - test('should return false for empty names', () => { - const result = validateAndWarnToolName(''); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors - }); - - test('should return false for names exceeding length limit', () => { - const longName = 'a'.repeat(129); - const result = validateAndWarnToolName(longName); - expect(result).toBe(false); - expect(warnSpy).toHaveBeenCalled(); // Now issues warnings instead of errors - }); }); describe('edge cases and robustness', () => { - test('should warn about names with only dots', () => { - const result = validateToolName('...'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dot, which may cause parsing issues in some contexts'); - }); - - test('should handle names with only dashes', () => { - const result = validateToolName('---'); - expect(result.isValid).toBe(true); - expect(result.warnings).toContain('Tool name starts or ends with a dash, which may cause parsing issues in some contexts'); - }); - - test('should reject names with only forward slashes', () => { - const result = validateToolName('///'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "/"'); - }); - - test('should handle names with mixed valid and invalid characters', () => { - const result = validateToolName('user@name123'); - expect(result.isValid).toBe(false); - expect(result.warnings).toContain('Tool name contains invalid characters: "@"'); + test.each` + description | toolName | shouldBeValid | expectedWarning + ${'names with only dots'} | ${'...'} | ${true} | ${'Tool name starts or ends with a dot, which may cause parsing issues in some contexts'} + ${'names with only dashes'} | ${'---'} | ${true} | ${'Tool name starts or ends with a dash, which may cause parsing issues in some contexts'} + ${'names with only forward slashes'} | ${'///'} | ${false} | ${'Tool name contains invalid characters: "/"'} + ${'names with mixed valid/invalid chars'} | ${'user@name123'} | ${false} | ${'Tool name contains invalid characters: "@"'} + `('should handle $description', ({ toolName, shouldBeValid, expectedWarning }) => { + const result = validateToolName(toolName); + expect(result.isValid).toBe(shouldBeValid); + expect(result.warnings).toContain(expectedWarning); }); });