Fix tools-name-format for spec Tool Names SHOULD rules on 2025-11-25+ (#380)
* fix(tools-list): align tools-name-format with 2025-11-25 spec prose Enforce dated spec Tool Names SHOULD rules (128 max, no slash), WARNING severity, 2025-11-25+ gate, negative fixture, and core-spec specReferences. Fixes #379 * test(tools-list): cast Connection.request mock for tsgo typecheck * test(tools-list): pin WARNING to offending tool name in negative test * test(tools-list): rename negative fixture to invalid-tool-names Document SEP-986 markdown vs integrated spec divergence in fixture header. * chore: trim explanatory comments to the essentials Keep the rule statement and the note that the dated spec (not the SEP-986 markdown) is authoritative; drop the process narration. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: triage <paulc@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
This commit is contained in:
dylan
GitHub
parent
98535fb92f
commit
0d824aebd8
3 changed files with 248 additions and 33 deletions
|
|
@ -8,6 +8,7 @@ import {
|
|||
JsonSchema2020_12Scenario,
|
||||
sep2106KeywordCheckStatus
|
||||
} from './json-schema-2020-12';
|
||||
import { ToolsListScenario } from './tools';
|
||||
import { DRAFT_PROTOCOL_VERSION, LATEST_SPEC_VERSION } from '../../types';
|
||||
import { takeWireViolations } from '../../validation/wire-schema';
|
||||
|
||||
|
|
@ -216,6 +217,41 @@ describe('Server scenario negative tests', () => {
|
|||
}, 10000);
|
||||
});
|
||||
|
||||
describe('tools-name-format', () => {
|
||||
let serverProcess: ChildProcess | null = null;
|
||||
const PORT = 3009;
|
||||
|
||||
beforeAll(async () => {
|
||||
serverProcess = await startServer(
|
||||
path.join(
|
||||
process.cwd(),
|
||||
'examples/servers/typescript/invalid-tool-names.ts'
|
||||
),
|
||||
PORT
|
||||
);
|
||||
}, 35000);
|
||||
|
||||
afterAll(async () => {
|
||||
await stopServer(serverProcess);
|
||||
});
|
||||
|
||||
it('emits WARNING for tools-name-format against a server advertising invalid tool names', async () => {
|
||||
const scenario = new ToolsListScenario();
|
||||
const checks = await scenario.run(
|
||||
testContext(`http://localhost:${PORT}/mcp`, '2025-11-25')
|
||||
);
|
||||
|
||||
const formatCheck = checks.find((c) => c.id === 'tools-name-format');
|
||||
expect(formatCheck?.status).toBe('WARNING');
|
||||
expect(formatCheck?.errorMessage).toContain('bad tool name');
|
||||
expect(formatCheck?.details).toMatchObject({
|
||||
results: expect.objectContaining({
|
||||
'bad tool name': expect.stringMatching(/invalid:/)
|
||||
})
|
||||
});
|
||||
}, 10000);
|
||||
});
|
||||
|
||||
describe('sep2106KeywordCheckStatus (soft version gate)', () => {
|
||||
it('passes preserved keywords at any target version', () => {
|
||||
expect(sep2106KeywordCheckStatus(true, DRAFT_PROTOCOL_VERSION)).toBe(
|
||||
|
|
|
|||
|
|
@ -1,5 +1,11 @@
|
|||
import { describe, it, expect } from 'vitest';
|
||||
import { buildToolsNameFormatCheck, validateToolNameFormat } from './tools.js';
|
||||
import {
|
||||
buildToolsNameFormatCheck,
|
||||
toolNameFormatCheckApplies,
|
||||
ToolsListScenario,
|
||||
validateToolNameFormat
|
||||
} from './tools.js';
|
||||
import type { Connection, RunContext } from '../../connection';
|
||||
|
||||
describe('validateToolNameFormat', () => {
|
||||
it('accepts a typical snake_case name', () => {
|
||||
|
|
@ -7,24 +13,30 @@ describe('validateToolNameFormat', () => {
|
|||
});
|
||||
|
||||
it('accepts all allowed character classes', () => {
|
||||
expect(validateToolNameFormat('Aa0_.-/')).toBeNull();
|
||||
expect(validateToolNameFormat('Aa0_.-')).toBeNull();
|
||||
});
|
||||
|
||||
it('accepts a single-character name (lower length boundary)', () => {
|
||||
expect(validateToolNameFormat('a')).toBeNull();
|
||||
});
|
||||
|
||||
it('accepts a 64-character name (upper length boundary)', () => {
|
||||
expect(validateToolNameFormat('a'.repeat(64))).toBeNull();
|
||||
it('accepts a 128-character name (upper length boundary)', () => {
|
||||
expect(validateToolNameFormat('a'.repeat(128))).toBeNull();
|
||||
});
|
||||
|
||||
it('rejects an empty name', () => {
|
||||
expect(validateToolNameFormat('')).toMatch(/length 0 is outside/);
|
||||
});
|
||||
|
||||
it('rejects a 65-character name', () => {
|
||||
expect(validateToolNameFormat('a'.repeat(65))).toMatch(
|
||||
/length 65 is outside/
|
||||
it('rejects a 129-character name', () => {
|
||||
expect(validateToolNameFormat('a'.repeat(129))).toMatch(
|
||||
/length 129 is outside/
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects forward slash (allowed in SEP-986 markdown but not spec prose)', () => {
|
||||
expect(validateToolNameFormat('namespace/tool')).toMatch(
|
||||
/contains characters outside/
|
||||
);
|
||||
});
|
||||
|
||||
|
|
@ -57,7 +69,7 @@ describe('buildToolsNameFormatCheck', () => {
|
|||
it('returns SUCCESS when all tool names are valid', () => {
|
||||
const check = buildToolsNameFormatCheck([
|
||||
{ name: 'test_simple_text' },
|
||||
{ name: 'namespace/tool-v1.2' }
|
||||
{ name: 'admin.tools.list' }
|
||||
]);
|
||||
expect(check.status).toBe('SUCCESS');
|
||||
expect(check.errorMessage).toBeUndefined();
|
||||
|
|
@ -65,33 +77,33 @@ describe('buildToolsNameFormatCheck', () => {
|
|||
toolCount: 2,
|
||||
results: {
|
||||
test_simple_text: 'valid',
|
||||
'namespace/tool-v1.2': 'valid'
|
||||
'admin.tools.list': 'valid'
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
it('returns FAILURE with per-tool details when some names are invalid', () => {
|
||||
it('returns WARNING with per-tool details when some names are invalid', () => {
|
||||
const check = buildToolsNameFormatCheck([
|
||||
{ name: 'good_tool' },
|
||||
{ name: 'bad name with spaces' },
|
||||
{ name: 'a'.repeat(65) }
|
||||
{ name: 'a'.repeat(129) }
|
||||
]);
|
||||
|
||||
expect(check.status).toBe('FAILURE');
|
||||
expect(check.status).toBe('WARNING');
|
||||
expect(check.errorMessage).toContain(
|
||||
'2 tool name(s) violate SEP-986 format'
|
||||
'2 tool name(s) violate spec Tool Names SHOULD rules'
|
||||
);
|
||||
|
||||
const results = (check.details as { results: Record<string, string> })
|
||||
.results;
|
||||
expect(results['good_tool']).toBe('valid');
|
||||
expect(results['bad name with spaces']).toMatch(/^invalid: /);
|
||||
expect(results['a'.repeat(65)]).toMatch(/^invalid: length 65/);
|
||||
expect(results['a'.repeat(129)]).toMatch(/^invalid: length 129/);
|
||||
});
|
||||
|
||||
it('flags a tool whose name is not a string', () => {
|
||||
const check = buildToolsNameFormatCheck([{ name: 123 as unknown }]);
|
||||
expect(check.status).toBe('FAILURE');
|
||||
expect(check.status).toBe('WARNING');
|
||||
const results = (check.details as { results: Record<string, string> })
|
||||
.results;
|
||||
expect(results['<tool[0] missing name>']).toBe(
|
||||
|
|
@ -99,9 +111,85 @@ describe('buildToolsNameFormatCheck', () => {
|
|||
);
|
||||
});
|
||||
|
||||
it('includes both MCP-Tools-List and SEP-986 spec references', () => {
|
||||
it('lists core spec Tool Names first, then SEP history for context', () => {
|
||||
const check = buildToolsNameFormatCheck([{ name: 'ok' }]);
|
||||
const ids = check.specReferences?.map((r) => r.id);
|
||||
expect(ids).toEqual(['MCP-Tools-List', 'SEP-986']);
|
||||
expect(ids).toEqual([
|
||||
'MCP-Tool-Names',
|
||||
'MCP-Tool-Names-Draft',
|
||||
'SEP-986-History',
|
||||
'SEP-986-Spec-Integration'
|
||||
]);
|
||||
expect(check.specReferences?.[0]?.url).toContain(
|
||||
'2025-11-25/server/tools#tool-names'
|
||||
);
|
||||
expect(check.specReferences?.[1]?.url).toContain(
|
||||
'draft/server/tools#tool-names'
|
||||
);
|
||||
expect(check.specReferences?.[2]?.url).toContain('issues/986');
|
||||
expect(check.specReferences?.[3]?.url).toContain('pull/1603');
|
||||
});
|
||||
});
|
||||
|
||||
describe('toolNameFormatCheckApplies', () => {
|
||||
it('is false before 2025-11-25 and true from that version onward', () => {
|
||||
expect(toolNameFormatCheckApplies('2025-03-26')).toBe(false);
|
||||
expect(toolNameFormatCheckApplies('2025-06-18')).toBe(false);
|
||||
expect(toolNameFormatCheckApplies('2025-11-25')).toBe(true);
|
||||
expect(toolNameFormatCheckApplies('2026-07-28')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('ToolsListScenario version gate', () => {
|
||||
// Scenario-level gate: invalid names must not emit the check before 2025-11-25.
|
||||
function mockContext(
|
||||
specVersion: RunContext['specVersion'],
|
||||
tools: Array<{ name: string; description: string; inputSchema: object }>
|
||||
): RunContext {
|
||||
const connection: Connection = {
|
||||
notifications: [],
|
||||
request: (async () => ({ tools })) as Connection['request'],
|
||||
discover: async () => ({}),
|
||||
close: async () => {}
|
||||
};
|
||||
|
||||
return {
|
||||
serverUrl: 'http://example.test/mcp',
|
||||
specVersion,
|
||||
connect: async () => connection
|
||||
};
|
||||
}
|
||||
|
||||
it('does not emit tools-name-format on 2025-06-18 even when names violate 2025-11-25 rules', async () => {
|
||||
const scenario = new ToolsListScenario();
|
||||
const checks = await scenario.run(
|
||||
mockContext('2025-06-18', [
|
||||
{
|
||||
name: 'bad name',
|
||||
description: 'invalid under 2025-11-25 rules',
|
||||
inputSchema: { type: 'object' }
|
||||
}
|
||||
])
|
||||
);
|
||||
|
||||
expect(checks.some((c) => c.id === 'tools-name-format')).toBe(false);
|
||||
expect(checks.find((c) => c.id === 'tools-list')?.status).toBe('SUCCESS');
|
||||
});
|
||||
|
||||
it('emits tools-name-format on 2025-11-25 when names violate spec prose', async () => {
|
||||
const scenario = new ToolsListScenario();
|
||||
const checks = await scenario.run(
|
||||
mockContext('2025-11-25', [
|
||||
{
|
||||
name: 'bad name',
|
||||
description: 'invalid under 2025-11-25 rules',
|
||||
inputSchema: { type: 'object' }
|
||||
}
|
||||
])
|
||||
);
|
||||
|
||||
expect(checks.find((c) => c.id === 'tools-name-format')?.status).toBe(
|
||||
'WARNING'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -5,7 +5,9 @@
|
|||
import {
|
||||
ClientScenario,
|
||||
ConformanceCheck,
|
||||
DRAFT_PROTOCOL_VERSION
|
||||
DRAFT_PROTOCOL_VERSION,
|
||||
specVersionAtLeast,
|
||||
type SpecVersion
|
||||
} from '../../types';
|
||||
import type { RunContext } from '../../connection';
|
||||
import type {
|
||||
|
|
@ -21,26 +23,45 @@ import {
|
|||
ElicitRequestSchema
|
||||
} from '@modelcontextprotocol/sdk/types.js';
|
||||
|
||||
const TOOL_NAME_PATTERN = /^[A-Za-z0-9_./-]+$/;
|
||||
const TOOL_NAME_MAX_LENGTH = 64;
|
||||
/**
|
||||
* Tool name rules per the 2025-11-25 spec prose (#tool-names): 1–128 chars of
|
||||
* [A-Za-z0-9_.-]. (The SEP-986 markdown still shows the older 64-char / `/`
|
||||
* rules; the published spec is authoritative.)
|
||||
*/
|
||||
const TOOL_NAME_PATTERN = /^[A-Za-z0-9_.-]+$/;
|
||||
const TOOL_NAME_MAX_LENGTH = 128;
|
||||
|
||||
const TOOLS_NAME_FORMAT_SPEC_REFS = [
|
||||
{
|
||||
id: 'MCP-Tools-List',
|
||||
url: 'https://modelcontextprotocol.io/specification/2025-11-25/server/tools#listing-tools'
|
||||
id: 'MCP-Tool-Names',
|
||||
url: 'https://modelcontextprotocol.io/specification/2025-11-25/server/tools#tool-names'
|
||||
},
|
||||
{
|
||||
id: 'SEP-986',
|
||||
url: 'https://modelcontextprotocol.io/specification/2025-11-25/server/tools#tool-names'
|
||||
id: 'MCP-Tool-Names-Draft',
|
||||
url: 'https://modelcontextprotocol.io/specification/draft/server/tools#tool-names'
|
||||
},
|
||||
// Background only; the dated spec above is the rule source.
|
||||
{
|
||||
id: 'SEP-986-History',
|
||||
url: 'https://github.com/modelcontextprotocol/modelcontextprotocol/issues/986'
|
||||
},
|
||||
{
|
||||
id: 'SEP-986-Spec-Integration',
|
||||
url: 'https://github.com/modelcontextprotocol/modelcontextprotocol/pull/1603'
|
||||
}
|
||||
];
|
||||
|
||||
/** The Tool Names rules first appear in the 2025-11-25 revision. */
|
||||
export function toolNameFormatCheckApplies(specVersion: SpecVersion): boolean {
|
||||
return specVersionAtLeast(specVersion, '2025-11-25');
|
||||
}
|
||||
|
||||
export function validateToolNameFormat(name: string): string | null {
|
||||
if (name.length < 1 || name.length > TOOL_NAME_MAX_LENGTH) {
|
||||
return `length ${name.length} is outside 1-${TOOL_NAME_MAX_LENGTH}`;
|
||||
}
|
||||
if (!TOOL_NAME_PATTERN.test(name)) {
|
||||
return 'contains characters outside [A-Za-z0-9_./-]';
|
||||
return 'contains characters outside [A-Za-z0-9_.-]';
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
|
@ -52,7 +73,8 @@ export function buildToolsNameFormatCheck(
|
|||
const baseCheck = {
|
||||
id: 'tools-name-format',
|
||||
name: 'ToolsNameFormat',
|
||||
description: 'Tool names are 1-64 characters and match ^[A-Za-z0-9_./-]+$',
|
||||
description:
|
||||
'Tool names SHOULD be 1-128 characters and match ^[A-Za-z0-9_.-]+$',
|
||||
specReferences: TOOLS_NAME_FORMAT_SPEC_REFS,
|
||||
timestamp
|
||||
};
|
||||
|
|
@ -86,10 +108,10 @@ export function buildToolsNameFormatCheck(
|
|||
|
||||
return {
|
||||
...baseCheck,
|
||||
status: violations.length === 0 ? 'SUCCESS' : 'FAILURE',
|
||||
status: violations.length === 0 ? 'SUCCESS' : 'WARNING',
|
||||
errorMessage:
|
||||
violations.length > 0
|
||||
? `${violations.length} tool name(s) violate SEP-986 format: ${violations.join('; ')}`
|
||||
? `${violations.length} tool name(s) violate spec Tool Names SHOULD rules: ${violations.join('; ')}`
|
||||
: undefined,
|
||||
details: {
|
||||
toolCount: tools.length,
|
||||
|
|
@ -110,9 +132,10 @@ export class ToolsListScenario implements ClientScenario {
|
|||
**Requirements**:
|
||||
- Return array of all available tools
|
||||
- Each tool MUST have:
|
||||
- \`name\` (string, 1-64 chars, matching \`^[A-Za-z0-9_./-]+$\`)
|
||||
- \`name\` (string)
|
||||
- \`description\` (string)
|
||||
- \`inputSchema\` (valid JSON Schema object)`;
|
||||
- \`inputSchema\` (valid JSON Schema object)
|
||||
- From 2025-11-25 onward, advertised \`name\` values SHOULD follow the spec Tool Names rules (1–128 chars, \`[A-Za-z0-9_.-]\` only) — see \`tools-name-format\` check`;
|
||||
|
||||
async run(ctx: RunContext): Promise<ConformanceCheck[]> {
|
||||
const checks: ConformanceCheck[] = [];
|
||||
|
|
@ -159,9 +182,9 @@ export class ToolsListScenario implements ClientScenario {
|
|||
}
|
||||
});
|
||||
|
||||
// Validate tool name format per SEP-986:
|
||||
// names MUST be 1-64 chars matching ^[A-Za-z0-9_./-]+$
|
||||
checks.push(buildToolsNameFormatCheck(result.tools));
|
||||
if (toolNameFormatCheckApplies(ctx.specVersion)) {
|
||||
checks.push(buildToolsNameFormatCheck(result.tools));
|
||||
}
|
||||
|
||||
await conn.close();
|
||||
} catch (error) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue