diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f204bc..335d247 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,40 @@ All notable changes follow [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.2.1] — 2026-06-21 + +### Fixed + +- **Correctness:** `isJsonRpcError()` no longer misclassifies a + `{ jsonrpc, id, result, error: null }` response as an error. JSON-RPC 2.0 and + the MCP `Error` interface require an error response to carry an `error` + member that is an object with a numeric `code`; a success response carries + `result` and omits `error`. The previous `"error" in response` check turned + a passing roundtrip into a spurious FAIL/WARN against the (common) servers + that always serialise an `error: null` default — affecting the smoke, schema, + capability and version suites. The helper now matches the wire contract and + the HTTP adapter's own envelope detection. This is a check-accuracy fix: a + wrong verdict here is worse than a missing check. + +### Added + +- `jsonrpc` suite gains a `jsonrpc-response-envelope` check enforcing + JSON-RPC 2.0 §5 — a response must contain exactly one of `result`/`error`. + It FAILs on `{ result, error: {…} }` (forbidden), WARNs on a hybrid + `{ result, error: null }` success envelope (tolerated but non-strict) and on + an empty envelope. No prior suite caught this. +- Test coverage: `tests/jsonrpc-helpers.test.ts` (10 cases pinning the + `isJsonRpcError` edge cases) + a `hybrid-envelope-server.mjs` fixture and + four integration tests proving the hybrid `error: null` shape is treated as + success by smoke/schema yet warned by the envelope check. 110 → 124 tests. + +### Changed + +- Migrated `vitest.config.ts` off the removed-in-Vitest-4 nested + `poolOptions.forks.singleFork` to the top-level `pool: "forks"` + + `fileParallelism: false`, clearing the deprecation warning while keeping + serial-per-file execution for the stdio/HTTP suites. + ## [0.2.0] — 2026-06-06 ### Added diff --git a/README.md b/README.md index 05a1126..514f006 100644 --- a/README.md +++ b/README.md @@ -114,7 +114,7 @@ Tools (all read-only, all `destructiveHint: false`): | # | Tool | Purpose | |---|------|---------| -| 1 | `runJsonRpcCompliance` | JSON-RPC 2.0 error-code suite | +| 1 | `runJsonRpcCompliance` | JSON-RPC 2.0 error-code matrix + response-envelope (result/error mutual exclusivity) | | 2 | `runSpecVersionAssertion` | Verify advertised protocolVersion | | 3 | `runTransportSuite` | Transport-layer ping + session-id | | 4 | `runOauthPkceFlow` | OAuth 2.1 PKCE S256 (mock-AS or real-tenant) | diff --git a/package.json b/package.json index f683cbe..309b919 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "mcp-protocol-conformance", - "version": "0.2.0", + "version": "0.2.1", "mcpName": "io.studiomeyer/protocol-conformance", "description": "Conformance test harness for Model Context Protocol servers. Validates JSON-RPC 2.0, spec-version handshake, transport, OAuth 2.1 PKCE, tool schemas, capabilities, smoke roundtrip, and annotations against MCP spec 2024-11-05 / 2025-03-26 / 2025-06-18 / 2025-11-25.", "type": "module", diff --git a/src/suites/jsonrpc.ts b/src/suites/jsonrpc.ts index e204d8e..4402a64 100644 --- a/src/suites/jsonrpc.ts +++ b/src/suites/jsonrpc.ts @@ -61,6 +61,59 @@ export async function runJsonRpcCompliance( }); } + // 1b. response-envelope mutual exclusivity → JSON-RPC 2.0 §5. + // A response object MUST contain either `result` or `error`, never + // both and never neither. `ping` is the cheapest required method to + // probe a well-formed *success* envelope; the method-not-found probe + // above already exercised a well-formed *error* envelope. Servers that + // emit `{ result, error: null }` (a common serialiser default) or + // `{ result, error: {...} }` (a real bug) are flagged here — no other + // suite catches this, and the harness's own isJsonRpcError() narrows + // such hybrids to "not an error", so without this check the violation + // would pass silently. + { + const start = Date.now(); + const ping = (await adapter.request("ping")) as unknown as Record< + string, + unknown + >; + const hasResult = "result" in ping && ping["result"] !== undefined; + const errVal = ping["error"]; + const hasRealError = + errVal !== undefined && + errVal !== null && + typeof errVal === "object" && + typeof (errVal as { code?: unknown }).code === "number"; + const hasNullishError = "error" in ping && !hasRealError; + let status: "pass" | "fail" | "warn"; + let message: string | undefined; + if (hasResult && hasRealError) { + status = "fail"; + message = + "Response carries both 'result' and a real 'error' object — JSON-RPC 2.0 forbids both in one response."; + } else if (!hasResult && !hasRealError) { + // ping not implemented (optional pre-2025-03-26) surfaces as an error + // envelope, which is fine; a truly empty envelope is the violation. + status = "warn"; + message = + "ping response carries neither a 'result' nor a valid 'error' object — empty JSON-RPC envelope."; + } else if (hasResult && hasNullishError) { + status = "warn"; + message = + "Response carries 'result' alongside a null/!code 'error' field. Tolerated by this harness, but a strict JSON-RPC 2.0 success response should omit 'error' entirely."; + } else { + status = "pass"; + } + runner.add({ + id: "jsonrpc-response-envelope", + description: + "Response contains exactly one of result/error (JSON-RPC 2.0 §5)", + status, + message, + durationMs: Date.now() - start, + }); + } + // 2. invalid-params → -32602 (call tools/call with wrong shape) { const start = Date.now(); diff --git a/src/targets/types.ts b/src/targets/types.ts index 96ff461..91b5f51 100644 --- a/src/targets/types.ts +++ b/src/targets/types.ts @@ -34,10 +34,33 @@ export interface JsonRpcError { export type JsonRpcResponse = JsonRpcSuccess | JsonRpcError; +/** + * Narrow a JSON-RPC response to the error variant. + * + * Per JSON-RPC 2.0 (and the MCP schema's `Error` interface) an error response + * carries an `error` member that is an object with a numeric `code` and a + * string `message`; a successful response carries `result` and omits `error`. + * + * Correctness note: a bare `"error" in response` check misclassifies the + * `{ jsonrpc, id, result, error: null }` shape — emitted by real-world servers + * that always serialise an `error: null` default alongside a valid `result` — + * as an error, turning a passing roundtrip into a spurious FAIL/WARN across + * every suite. We therefore require `error` to be a non-null object with a + * numeric `code`, matching the wire contract and the HTTP adapter's own + * envelope detection. `error: null`, a missing `code`, or a non-object `error` + * are treated as "not an error response". + */ export function isJsonRpcError( response: JsonRpcResponse, ): response is JsonRpcError { - return "error" in response; + if (!response || typeof response !== "object") return false; + if (!("error" in response)) return false; + const err = (response as { error?: unknown }).error; + return ( + err !== null && + typeof err === "object" && + typeof (err as { code?: unknown }).code === "number" + ); } export interface TargetAdapter { diff --git a/tests/fixtures/hybrid-envelope-server.mjs b/tests/fixtures/hybrid-envelope-server.mjs new file mode 100644 index 0000000..91d6658 --- /dev/null +++ b/tests/fixtures/hybrid-envelope-server.mjs @@ -0,0 +1,75 @@ +#!/usr/bin/env node +// Off-spec MCP stdio server that emits a HYBRID success envelope: +// every successful reply carries `result` AND `error: null`. +// +// This is a common serialiser default (struct with an always-present nullable +// error field) and a real interop hazard. It exercises two things at once: +// 1. isJsonRpcError() must still treat { result, error: null } as SUCCESS +// (otherwise the whole run fails spuriously). +// 2. the jsonrpc suite's `jsonrpc-response-envelope` check must WARN on it +// (a strict JSON-RPC 2.0 success response omits `error` entirely). +// +// Plain JS so tests can spawn it without a TS toolchain. + +import { createInterface } from "node:readline"; + +function send(obj) { + process.stdout.write(JSON.stringify(obj) + "\n"); +} +// Note the deliberate `error: null` alongside every result. +function reply(id, result) { + send({ jsonrpc: "2.0", id, result, error: null }); +} +function fail(id, code, message) { + send({ jsonrpc: "2.0", id, error: { code, message } }); +} + +const rl = createInterface({ input: process.stdin }); +rl.on("line", (line) => { + const trimmed = line.trim(); + if (!trimmed) return; + let req; + try { + req = JSON.parse(trimmed); + } catch { + send({ jsonrpc: "2.0", id: null, error: { code: -32700, message: "Parse error" } }); + return; + } + if (req.method === "notifications/initialized") return; + if (req.method === "initialize") { + reply(req.id, { + protocolVersion: "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "hybrid-envelope-server", version: "0.0.1" }, + }); + return; + } + if (req.method === "ping") { + reply(req.id, {}); + return; + } + if (req.method === "tools/list") { + reply(req.id, { + tools: [ + { + name: "echo", + description: "Echo a message back.", + inputSchema: { + type: "object", + properties: { message: { type: "string" } }, + required: ["message"], + additionalProperties: false, + }, + }, + ], + }); + return; + } + if (req.method === "tools/call") { + reply(req.id, { content: [{ type: "text", text: "ok" }], isError: false }); + return; + } + fail(req.id, -32601, `Method not found: ${req.method}`); +}); + +process.on("SIGTERM", () => process.exit(0)); diff --git a/tests/integration.test.ts b/tests/integration.test.ts index 36a65ee..487bc76 100644 --- a/tests/integration.test.ts +++ b/tests/integration.test.ts @@ -34,6 +34,13 @@ const MCP2511_TARGET: ServerTarget = { args: [MCP2511], }; +const HYBRID = resolve(__dirname, "fixtures", "hybrid-envelope-server.mjs"); +const HYBRID_TARGET: ServerTarget = { + kind: "stdio", + cmd: process.execPath, + args: [HYBRID], +}; + describe("echo-server: positive integration", () => { it("passes JSON-RPC compliance suite", async () => { const report = await runJsonRpcCompliance(ECHO_TARGET, "2025-06-18"); @@ -163,6 +170,40 @@ describe("mcp2511-server: 2025-11-25 surface checks", () => { }, 15_000); }); +describe("jsonrpc suite: response-envelope conformance (JSON-RPC 2.0 §5)", () => { + it("passes the envelope check against a clean result-only server", async () => { + const report = await runJsonRpcCompliance(ECHO_TARGET, "2025-06-18"); + const env = report.checks.find((c) => c.id === "jsonrpc-response-envelope"); + expect(env?.status).toBe("pass"); + }, 15_000); + + it("warns on a hybrid { result, error: null } success envelope", async () => { + const report = await runJsonRpcCompliance(HYBRID_TARGET, "2025-06-18"); + const env = report.checks.find((c) => c.id === "jsonrpc-response-envelope"); + expect(env?.status).toBe("warn"); + expect(env?.message ?? "").toMatch(/null/i); + }, 15_000); + + it("does NOT spuriously fail a hybrid-envelope server's method-not-found probe", async () => { + // Regression guard: with the loose isJsonRpcError, { result, error: null } + // on success leaked into error-classification. The method-not-found check + // must still see the real -32601 error envelope and pass. + const report = await runJsonRpcCompliance(HYBRID_TARGET, "2025-06-18"); + const mnf = report.checks.find((c) => c.id === "jsonrpc-method-not-found"); + expect(mnf?.status).toBe("pass"); + }, 15_000); + + it("smoke + schema treat a hybrid-envelope tool result as success, not error", async () => { + // The whole point of the isJsonRpcError fix: a server that always serialises + // error:null must not have its valid tool roundtrip mis-reported as an error. + const schema = await runToolSchemaValidation(HYBRID_TARGET, undefined, "2025-06-18"); + expect(schema.status).not.toBe("fail"); + const smoke = await runRoundtripSmoke(HYBRID_TARGET, undefined, "2025-06-18"); + const echo = smoke.checks.find((c) => c.id === "smoke-echo"); + expect(echo?.status).toBe("pass"); + }, 20_000); +}); + beforeAll(() => { // smoke check that fixtures exist on disk }); diff --git a/tests/jsonrpc-helpers.test.ts b/tests/jsonrpc-helpers.test.ts new file mode 100644 index 0000000..816a5fe --- /dev/null +++ b/tests/jsonrpc-helpers.test.ts @@ -0,0 +1,102 @@ +/** + * Unit tests for isJsonRpcError — the load-bearing response classifier used by + * every suite. A wrong verdict here turns a passing roundtrip into a spurious + * FAIL/WARN (or vice-versa) across the whole harness, so the edge cases are + * pinned explicitly. + * + * Spec basis (verified against the MCP schema's `Error` interface and + * JSON-RPC 2.0 §5): an error response carries an `error` member that is an + * object with a numeric `code`; a success response carries `result` and omits + * `error`. `error: null`, `error` without a numeric `code`, and a non-object + * `error` are NOT error responses. + */ +import { describe, expect, it } from "vitest"; +import { isJsonRpcError } from "../src/targets/types.js"; +import type { JsonRpcResponse } from "../src/targets/types.js"; + +// Most fixtures below are intentionally off-spec wire shapes a real server +// might emit; we cast through unknown so the test can assert the runtime +// narrowing without fighting the compile-time union. +function asResponse(v: unknown): JsonRpcResponse { + return v as JsonRpcResponse; +} + +describe("isJsonRpcError", () => { + it("returns true for a well-formed error envelope", () => { + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + error: { code: -32601, message: "Method not found" }, + }); + expect(isJsonRpcError(r)).toBe(true); + }); + + it("returns true for an error envelope with a data payload", () => { + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + error: { code: -32002, message: "Resource not found", data: { uri: "x" } }, + }); + expect(isJsonRpcError(r)).toBe(true); + }); + + it("returns false for a success envelope (result only)", () => { + const r = asResponse({ jsonrpc: "2.0", id: 1, result: { ok: true } }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("returns false when result is present and error is null (serialiser default)", () => { + // The core regression: { result, error: null } is a SUCCESS response. A + // bare `"error" in response` check would misclassify this as an error and + // fail an otherwise-passing roundtrip. + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + result: { ok: true }, + error: null, + }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("returns false when error is null and no result is present", () => { + const r = asResponse({ jsonrpc: "2.0", id: 1, error: null }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("returns false when error is an object without a numeric code", () => { + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + error: { message: "no code here" }, + }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("returns false when error.code is a string (not a number)", () => { + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + error: { code: "-32601", message: "stringly typed" }, + }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("returns false when error is a non-object (string)", () => { + const r = asResponse({ jsonrpc: "2.0", id: 1, error: "boom" }); + expect(isJsonRpcError(r)).toBe(false); + }); + + it("accepts code 0 (a valid JSON-RPC numeric code)", () => { + const r = asResponse({ + jsonrpc: "2.0", + id: 1, + error: { code: 0, message: "zero is a number" }, + }); + expect(isJsonRpcError(r)).toBe(true); + }); + + it("does not throw on a null/undefined response", () => { + expect(isJsonRpcError(asResponse(null))).toBe(false); + expect(isJsonRpcError(asResponse(undefined))).toBe(false); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index ac52b17..dd54249 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,10 +5,12 @@ export default defineConfig({ include: ["tests/**/*.test.ts"], testTimeout: 30_000, hookTimeout: 10_000, + // Run forked workers, one test file at a time. The stdio suites spawn + // child MCP servers and the OAuth/HTTP suites bind ephemeral localhost + // ports; serial file execution avoids port collisions and child-process + // noise. Vitest 4 removed nested `poolOptions.forks.singleFork` — the + // top-level equivalent is `pool: "forks"` + `fileParallelism: false`. pool: "forks", - // Sequential pool for stdio tests — avoids port collisions and child-process noise - poolOptions: { - forks: { singleFork: true }, - }, + fileParallelism: false, }, });