Implementation of SEP-986: Specify Format for Tool Names (#900)
Co-authored-by: Felix Weinberger <fweinberger@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com>
This commit is contained in:
Kent C. Dodds
GitHub
parent
9f06b6b499
commit
29cb08071a
4 changed files with 303 additions and 0 deletions
|
|
@ -227,6 +227,10 @@ describe('ResourceTemplate', () => {
|
|||
});
|
||||
|
||||
describe('tool()', () => {
|
||||
afterEach(() => {
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
/***
|
||||
* Test: Zero-Argument Tool Registration
|
||||
*/
|
||||
|
|
@ -1692,6 +1696,56 @@ 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' 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' as const, text: 'Success' }] })
|
||||
);
|
||||
|
||||
// Test invalid tool name (contains spaces)
|
||||
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();
|
||||
|
||||
// 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()', () => {
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> | undefined,
|
||||
callback: ToolCallback<ZodRawShape | undefined>
|
||||
): 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;
|
||||
}
|
||||
|
|
|
|||
127
src/shared/toolNameValidation.test.ts
Normal file
127
src/shared/toolNameValidation.test.ts
Normal file
|
|
@ -0,0 +1,127 @@
|
|||
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.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.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(expectedWarning);
|
||||
});
|
||||
});
|
||||
|
||||
describe('warnings for potentially problematic patterns', () => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
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.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 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);
|
||||
});
|
||||
});
|
||||
|
||||
describe('edge cases and robustness', () => {
|
||||
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);
|
||||
});
|
||||
});
|
||||
115
src/shared/toolNameValidation.ts
Normal file
115
src/shared/toolNameValidation.ts
Normal file
|
|
@ -0,0 +1,115 @@
|
|||
/**
|
||||
* 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 (-), 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}$/;
|
||||
|
||||
/**
|
||||
* 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');
|
||||
}
|
||||
|
||||
// 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: 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;
|
||||
}
|
||||
Loading…
Reference in a new issue