Fix wire-validation attribution bugs and extend the schema erratum to 2025-06-18 (#421)
Follow-ups from the #399 review: - Stateless mock: validate error replies with a placeholder id so the JSON-RPC-mandated id:null is not recorded as a harness violation, and skip inbound validation when express left the body unparsed (wrong or missing Content-Type) — both were false positives blamed on the wrong party. Same unparsed-body guard in the stateful mock. - Stateful mock: validate client traffic against the protocol version the client declares via MCP-Protocol-Version, falling back to 2025-03-26 when the header is absent (it only exists from 2025-06-18, whose spec says to assume 2025-03-26), instead of always using the run version. - sdk-client: validate batches once as a whole array instead of also per element, which double-recorded every violation. - Make specVersion required on connectToServer/connectStateful/ createServerStateful; add isSpecVersion to types.ts. - Extend the NumberSchema erratum to 2025-06-18 (same generator bug on minimum/maximum, modelcontextprotocol#3139) and add a tripwire test that forces erratum deletion once the schemas are re-vendored. - Document the wire-schema-valid / wire-schema-harness-error check IDs in the README.
This commit is contained in:
parent
89c2c0452e
commit
5cc567c399
10 changed files with 201 additions and 39 deletions
18
README.md
18
README.md
|
|
@ -98,6 +98,24 @@ npx @modelcontextprotocol/conformance server --url <url> [--scenario <scenario>]
|
|||
|
||||
- `checks.json` - Array of conformance check results with pass/fail status
|
||||
|
||||
### Wire-schema checks
|
||||
|
||||
Every scenario also validates each JSON-RPC message on the wire against the
|
||||
spec's JSON schema for the negotiated spec version, and emits up to two
|
||||
synthetic checks alongside the scenario's own:
|
||||
|
||||
- `wire-schema-valid` - fails when a message _the implementation under test
|
||||
sent_ violates the spec JSON schema. The failure details include every
|
||||
violating message and its schema errors.
|
||||
- `wire-schema-harness-error` - fails when the _harness itself_ sent an
|
||||
invalid message. This indicates a bug in the conformance suite (or a
|
||||
deliberately nonconformant fixture), not in the implementation under test;
|
||||
please report it.
|
||||
|
||||
Scenarios that exchange no instrumented wire traffic (see issue #418) emit
|
||||
neither check. Like any other check, `wire-schema-valid` can be baselined via
|
||||
the expected-failures file.
|
||||
|
||||
## Expected Failures
|
||||
|
||||
SDKs that don't yet pass all conformance tests can specify a baseline of known failures. This allows running conformance tests in CI without failing, while still catching regressions.
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ import {
|
|||
} from '@modelcontextprotocol/sdk/types.js';
|
||||
import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js';
|
||||
|
||||
import { LATEST_SPEC_VERSION, type SpecVersion } from '../types';
|
||||
import { type SpecVersion } from '../types';
|
||||
import {
|
||||
validateWireMessage,
|
||||
type WireOrigin
|
||||
|
|
@ -51,8 +51,8 @@ function instrumentTransport(
|
|||
origin === 'harness' ? harnessRequests : implementationRequests;
|
||||
const peerRequests =
|
||||
origin === 'harness' ? implementationRequests : harnessRequests;
|
||||
// `send` accepts arrays (2025-03-26 batches); validate each element with
|
||||
// its own classification, then the batch envelope itself.
|
||||
// `send` accepts arrays (2025-03-26 batches); walk elements for request/
|
||||
// response bookkeeping, validating singles here and batches whole below.
|
||||
for (const m of Array.isArray(message) ? message : [message]) {
|
||||
const msg = (typeof m === 'object' && m !== null ? m : {}) as Record<
|
||||
string,
|
||||
|
|
@ -73,9 +73,14 @@ function instrumentTransport(
|
|||
if (id !== undefined) peerRequests.delete(id);
|
||||
context = `stateful response to '${requestMethod ?? `id ${String(id)}`}'`;
|
||||
}
|
||||
validateWireMessage(specVersion, m, { origin, context, requestMethod });
|
||||
if (!Array.isArray(message)) {
|
||||
validateWireMessage(specVersion, m, { origin, context, requestMethod });
|
||||
}
|
||||
}
|
||||
if (Array.isArray(message)) {
|
||||
// One whole-array validation covers per-element errors (prefixed [i])
|
||||
// and batch legality for the version; validating the elements in the
|
||||
// loop above too would record every violation twice.
|
||||
validateWireMessage(specVersion, message, {
|
||||
origin,
|
||||
context: 'stateful batch'
|
||||
|
|
@ -119,8 +124,8 @@ function instrumentTransport(
|
|||
*/
|
||||
export async function connectToServer(
|
||||
serverUrl: string,
|
||||
opts: ConnectOptions = {},
|
||||
specVersion: SpecVersion = LATEST_SPEC_VERSION
|
||||
opts: ConnectOptions,
|
||||
specVersion: SpecVersion
|
||||
): Promise<MCPClientConnection> {
|
||||
const client = new Client(opts.clientInfo ?? DEFAULT_CLIENT_INFO, {
|
||||
capabilities: opts.capabilities ?? DEFAULT_CAPABILITIES
|
||||
|
|
|
|||
|
|
@ -46,7 +46,7 @@ export function connectFor(
|
|||
// Pass the version through so requests declare (and are wire-schema
|
||||
// validated against) the spec version the run was invoked with.
|
||||
return isStatefulVersion(specVersion)
|
||||
? (serverUrl, opts) => connectStateful(serverUrl, opts, specVersion)
|
||||
? (serverUrl, opts) => connectStateful(serverUrl, opts ?? {}, specVersion)
|
||||
: (serverUrl, opts) => connectStateless(serverUrl, specVersion, opts);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -15,13 +15,13 @@ import {
|
|||
} from '@modelcontextprotocol/sdk/types.js';
|
||||
import { connectToServer } from './sdk-client';
|
||||
import type { JSONRPCNotification } from '../spec-types/2025-11-25';
|
||||
import { LATEST_SPEC_VERSION, type SpecVersion } from '../types';
|
||||
import { type SpecVersion } from '../types';
|
||||
import { JsonRpcError, type Connection, type ConnectOptions } from './index';
|
||||
|
||||
export async function connectStateful(
|
||||
serverUrl: string,
|
||||
opts: ConnectOptions = {},
|
||||
specVersion: SpecVersion = LATEST_SPEC_VERSION
|
||||
opts: ConnectOptions,
|
||||
specVersion: SpecVersion
|
||||
): Promise<Connection> {
|
||||
// Wire-schema validation happens inside connectToServer, which hooks the
|
||||
// SDK transport's send/onmessage so the real bytes of every message in
|
||||
|
|
|
|||
|
|
@ -8,7 +8,7 @@ import {
|
|||
CACHEABLE_RESULT_METHODS
|
||||
} from './stateless';
|
||||
import { STATELESS_SPEC_VERSIONS } from '../connection/select';
|
||||
import { DRAFT_PROTOCOL_VERSION } from '../types';
|
||||
import { LATEST_SPEC_VERSION, DRAFT_PROTOCOL_VERSION } from '../types';
|
||||
import { takeWireViolations } from '../validation/wire-schema';
|
||||
|
||||
const meta = {
|
||||
|
|
@ -500,9 +500,10 @@ describe('createServerStateful', () => {
|
|||
}
|
||||
|
||||
it('accepts initialize and routes to handlers, recording non-preamble', async () => {
|
||||
const srv = await createServerStateful({
|
||||
'tools/list': () => ({ tools: [] })
|
||||
});
|
||||
const srv = await createServerStateful(
|
||||
{ 'tools/list': () => ({ tools: [] }) },
|
||||
LATEST_SPEC_VERSION
|
||||
);
|
||||
try {
|
||||
// SDK transport in sessionless mode handles initialize internally; we
|
||||
// can drive it via the SDK Client.
|
||||
|
|
@ -524,9 +525,10 @@ describe('createServerStateful', () => {
|
|||
});
|
||||
|
||||
it('derives capabilities from handler keys; non-tools handler does not 500 initialize', async () => {
|
||||
const srv = await createServerStateful({
|
||||
'prompts/list': () => ({ prompts: [] })
|
||||
});
|
||||
const srv = await createServerStateful(
|
||||
{ 'prompts/list': () => ({ prompts: [] }) },
|
||||
LATEST_SPEC_VERSION
|
||||
);
|
||||
try {
|
||||
const { status, contentType } = await postInit(srv.url);
|
||||
expect(status).toBe(200);
|
||||
|
|
@ -537,9 +539,10 @@ describe('createServerStateful', () => {
|
|||
});
|
||||
|
||||
it('records requests for unregistered methods (parity with stateless)', async () => {
|
||||
const srv = await createServerStateful({
|
||||
'tools/list': () => ({ tools: [] })
|
||||
});
|
||||
const srv = await createServerStateful(
|
||||
{ 'tools/list': () => ({ tools: [] }) },
|
||||
LATEST_SPEC_VERSION
|
||||
);
|
||||
try {
|
||||
const { Client } =
|
||||
await import('@modelcontextprotocol/sdk/client/index.js');
|
||||
|
|
@ -565,3 +568,73 @@ describe('createServerStateful', () => {
|
|||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('wire violation attribution', () => {
|
||||
it('does not blame the harness for the JSON-RPC-mandated id:null on error replies', async () => {
|
||||
const srv = await createServerStateless({}, DRAFT_PROTOCOL_VERSION);
|
||||
try {
|
||||
// An id-less, _meta-less message forces a reject reply carrying
|
||||
// `id: null` (JSON-RPC 2.0 when the request id cannot be determined).
|
||||
await post(srv.url, { jsonrpc: '2.0', method: 'tools/call' }, headers);
|
||||
const { violations } = takeWireViolations();
|
||||
expect(violations.filter((v) => v.origin === 'harness')).toEqual([]);
|
||||
} finally {
|
||||
await srv.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('does not record an implementation violation for an unparsed body (wrong Content-Type)', async () => {
|
||||
const stateless = await createServerStateless({}, DRAFT_PROTOCOL_VERSION);
|
||||
const stateful = await createServerStateful({}, LATEST_SPEC_VERSION);
|
||||
try {
|
||||
for (const url of [stateless.url, stateful.url]) {
|
||||
await fetch(url, {
|
||||
method: 'POST',
|
||||
headers: { 'content-type': 'text/plain' },
|
||||
body: '{"jsonrpc":"2.0","id":1,"method":"ping"}'
|
||||
});
|
||||
}
|
||||
const { violations } = takeWireViolations();
|
||||
expect(violations.filter((v) => v.origin === 'implementation')).toEqual(
|
||||
[]
|
||||
);
|
||||
} finally {
|
||||
await stateless.close();
|
||||
await stateful.close();
|
||||
}
|
||||
});
|
||||
|
||||
it('validates stateful client traffic against the version the client declares, not the run version', async () => {
|
||||
const srv = await createServerStateful({}, '2025-11-25');
|
||||
try {
|
||||
const batch = [{ jsonrpc: '2.0', id: 1, method: 'ping' }];
|
||||
// Batch arrays are only legal at 2025-03-26; a client that negotiated
|
||||
// that version declares it on the request.
|
||||
await post(srv.url, batch, { 'mcp-protocol-version': '2025-03-26' });
|
||||
const negotiated = takeWireViolations();
|
||||
expect(
|
||||
negotiated.violations.filter((v) => v.origin === 'implementation')
|
||||
).toEqual([]);
|
||||
|
||||
// No header: the header only exists from 2025-06-18, whose spec says
|
||||
// to assume 2025-03-26 — where the batch is legal.
|
||||
await post(srv.url, batch);
|
||||
const headerless = takeWireViolations();
|
||||
expect(
|
||||
headerless.violations.filter((v) => v.origin === 'implementation')
|
||||
).toEqual([]);
|
||||
|
||||
await post(srv.url, batch, { 'mcp-protocol-version': '2025-11-25' });
|
||||
const runVersion = takeWireViolations();
|
||||
expect(
|
||||
runVersion.violations.some(
|
||||
(v) =>
|
||||
v.origin === 'implementation' &&
|
||||
v.errors.some((e) => e.includes('batch'))
|
||||
)
|
||||
).toBe(true);
|
||||
} finally {
|
||||
await srv.close();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ import { StreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/
|
|||
import { ErrorCode, McpError } from '@modelcontextprotocol/sdk/types.js';
|
||||
import { z } from 'zod';
|
||||
import type { JSONRPCRequest } from '../spec-types/2025-11-25';
|
||||
import { LATEST_SPEC_VERSION, type SpecVersion } from '../types';
|
||||
import { isSpecVersion, type SpecVersion } from '../types';
|
||||
import { validateWireMessage } from '../validation/wire-schema';
|
||||
import type { MockServer, RequestHandlers } from './index';
|
||||
|
||||
|
|
@ -43,7 +43,7 @@ export function capabilitiesFromHandlers(
|
|||
|
||||
export async function createServerStateful(
|
||||
handlers: RequestHandlers,
|
||||
specVersion: SpecVersion = LATEST_SPEC_VERSION
|
||||
specVersion: SpecVersion
|
||||
): Promise<MockServer> {
|
||||
const recorded: JSONRPCRequest[] = [];
|
||||
const capabilities = capabilitiesFromHandlers(handlers);
|
||||
|
|
@ -100,10 +100,27 @@ export async function createServerStateful(
|
|||
// preamble) at the HTTP layer so unregistered methods are captured too,
|
||||
// matching the stateless impl and the MockServer.recorded contract.
|
||||
const body = req.body;
|
||||
validateWireMessage(specVersion, body, {
|
||||
origin: 'implementation',
|
||||
context: `client request '${body?.method ?? '(unknown)'}' to stateful mock`
|
||||
});
|
||||
// An unparsed body (wrong or missing Content-Type) is a transport fault
|
||||
// the SDK transport rejects itself, not a JSON-RPC message to validate.
|
||||
if (body !== undefined) {
|
||||
// Clients may legitimately negotiate a different protocol version than
|
||||
// the run's (the README permits ignoring the forwarded --spec-version),
|
||||
// and they declare it on every post-initialize request; validate their
|
||||
// traffic against the version they actually speak.
|
||||
const headerVersion = req.headers['mcp-protocol-version'];
|
||||
// Absent header: the header only exists from 2025-06-18, whose spec
|
||||
// says to assume 2025-03-26 — a header-less legacy client is not a
|
||||
// violation of the run version.
|
||||
const inboundVersion = isSpecVersion(headerVersion)
|
||||
? headerVersion
|
||||
: headerVersion === undefined
|
||||
? '2025-03-26'
|
||||
: specVersion;
|
||||
validateWireMessage(inboundVersion, body, {
|
||||
origin: 'implementation',
|
||||
context: `client request '${body?.method ?? '(unknown)'}' to stateful mock`
|
||||
});
|
||||
}
|
||||
if (
|
||||
body?.method &&
|
||||
body.method !== 'initialize' &&
|
||||
|
|
|
|||
|
|
@ -202,15 +202,29 @@ export async function createServerStateless(
|
|||
// requests are captured too, matching the stateful impl and the
|
||||
// MockServer.recorded contract.
|
||||
const body = req.body as Record<string, unknown> | undefined;
|
||||
validateWireMessage(wireVersion, req.body, {
|
||||
origin: 'implementation',
|
||||
context: `client request '${body?.method ?? '(unknown)'}' to stateless mock`
|
||||
});
|
||||
// An unparsed body (wrong or missing Content-Type) is a transport fault
|
||||
// surfaced by the HTTP-level checks, not a JSON-RPC message to validate.
|
||||
if (body !== undefined) {
|
||||
validateWireMessage(wireVersion, req.body, {
|
||||
origin: 'implementation',
|
||||
context: `client request '${body?.method ?? '(unknown)'}' to stateless mock`
|
||||
});
|
||||
}
|
||||
if (body?.method && body.method !== 'server/discover') {
|
||||
recorded.push(req.body as JSONRPCRequest);
|
||||
}
|
||||
const sendJson = (status: number, payload: object, method?: string) => {
|
||||
validateWireMessage(wireVersion, payload, {
|
||||
// JSON-RPC 2.0 mandates `id: null` on error replies when the request id
|
||||
// could not be determined, but the spec's RequestId is string|integer;
|
||||
// validate with a placeholder id so the mandated null is not recorded
|
||||
// as a harness violation (mirrors connection/stateless.ts's
|
||||
// withNullIdCarveOut).
|
||||
const raw = payload as Record<string, unknown>;
|
||||
const validated =
|
||||
raw.error !== undefined && raw.id === null
|
||||
? { ...raw, id: 0 }
|
||||
: payload;
|
||||
validateWireMessage(wireVersion, validated, {
|
||||
origin: 'harness',
|
||||
context: `stateless mock response${method ? ` to '${method}'` : ''}`,
|
||||
requestMethod: method
|
||||
|
|
|
|||
|
|
@ -69,6 +69,11 @@ const SPEC_VERSION_TIMELINE: readonly SpecVersion[] = [
|
|||
DRAFT_PROTOCOL_VERSION
|
||||
];
|
||||
|
||||
/** True when `v` names a known spec version (dated or draft). */
|
||||
export function isSpecVersion(v: unknown): v is SpecVersion {
|
||||
return SPEC_VERSION_TIMELINE.includes(v as SpecVersion);
|
||||
}
|
||||
|
||||
/**
|
||||
* True when `v` is at or after `threshold` on the spec timeline. Lets a check
|
||||
* gate itself to the version that introduced its requirement (e.g. a draft-only
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
import { createServer, type Server } from 'http';
|
||||
import rawSchema2025_06_18 from '../spec-types/2025-06-18.schema.json';
|
||||
import rawSchema2025_11_25 from '../spec-types/2025-11-25.schema.json';
|
||||
import { DRAFT_PROTOCOL_VERSION } from '../types';
|
||||
import { sendStatelessRequest } from '../connection/stateless';
|
||||
import {
|
||||
|
|
@ -506,4 +508,31 @@ describe('schema errata', () => {
|
|||
)
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('accepts fractional NumberSchema bounds at 2025-06-18 (same generator bug, no default there)', () => {
|
||||
expect(
|
||||
wireSchemaErrors(
|
||||
'2025-06-18',
|
||||
elicitWithScore({ type: 'number', minimum: 0.5, maximum: 99.5 })
|
||||
)
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('tripwire: delete applySchemaErrata once the vendored schemas are fixed (modelcontextprotocol#3139)', () => {
|
||||
const numberSchema = (raw: Record<string, unknown>, defsKey: string) =>
|
||||
(
|
||||
raw[defsKey] as Record<
|
||||
string,
|
||||
{ properties: Record<string, { type: string }> }
|
||||
>
|
||||
).NumberSchema.properties;
|
||||
expect(
|
||||
numberSchema(rawSchema2025_11_25, '$defs').default.type,
|
||||
'the re-vendored 2025-11-25 schema no longer needs the erratum — delete its applySchemaErrata branch'
|
||||
).toBe('integer');
|
||||
expect(
|
||||
numberSchema(rawSchema2025_06_18, 'definitions').minimum.type,
|
||||
'the re-vendored 2025-06-18 schema no longer needs the erratum — delete its applySchemaErrata branch'
|
||||
).toBe('integer');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -75,18 +75,19 @@ interface CompiledSpec {
|
|||
|
||||
const compiledSpecs = new Map<SpecVersion, CompiledSpec>();
|
||||
|
||||
/** Patch known bugs in released (frozen) generated spec schemas at load time
|
||||
* so validation matches the schema.ts source of truth. Each erratum cites the
|
||||
* upstream fix; delete it once the dated schema.json is corrected upstream
|
||||
* and re-vendored. */
|
||||
/** Patch known generator bugs in released spec schema.json files at load time
|
||||
* so validation matches the schema.ts source of truth; delete each branch (a
|
||||
* test trips) once the dated schema is fixed upstream and re-vendored. */
|
||||
function applySchemaErrata(
|
||||
specVersion: SpecVersion,
|
||||
schema: Record<string, unknown>
|
||||
): Record<string, unknown> {
|
||||
if (specVersion !== '2025-11-25') return schema;
|
||||
// 2025-11-25 generates NumberSchema minimum/maximum/default as `integer`,
|
||||
// contradicting its own schema.ts (`default?: number`). Fixed for draft in
|
||||
// modelcontextprotocol#2710, which left the released schema.json as-is.
|
||||
if (specVersion !== '2025-11-25' && specVersion !== '2025-06-18') {
|
||||
return schema;
|
||||
}
|
||||
// NumberSchema minimum/maximum (plus default at 2025-11-25) are generated
|
||||
// as `integer`, contradicting schema.ts (`number`). Fixed for draft in
|
||||
// modelcontextprotocol#2710; dated fixes proposed in modelcontextprotocol#3139.
|
||||
const patched = structuredClone(schema);
|
||||
const defs = (patched.$defs ?? patched.definitions) as Record<
|
||||
string,
|
||||
|
|
|
|||
Loading…
Reference in a new issue