Fix isJsonRpcError mis-classifying {result,error:null} + envelope check (110->124)

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

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) |

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",

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();

src/targets/types.ts

@@ -34,10 +34,33 @@ export interface JsonRpcError {
 
 export type JsonRpcResponse<T = unknown> = JsonRpcSuccess<T> | 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 {

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));

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
 });

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);
+  });
+});