From 660b042152dad597f62fa1d3b3b8e91d8fb7e033 Mon Sep 17 00:00:00 2001 From: rinaldofesta Date: Mon, 1 Jun 2026 13:06:42 +0200 Subject: [PATCH 1/2] feat(server): standardize setup-failure reporting via reportSetupFailure helper Scenarios that can't execute (connect failure, missing fixture, capability not advertised) currently hand-roll a try/catch around connect and pin the error onto whichever check ID happens to be first. That mislabels the failure and silently drops any *other* checks the scenario would emit (e.g. a connect failure in tools-list also makes tools-name-format vanish). Add `reportSetupFailure(scenarioName, error, specReferences?)` to sdk-client.ts, emitting a single dedicated `-setup` check as FAILURE with the error detail. Route the setup path of four representative multi-check server scenarios through it (server-initialize, tools-list, prompts-list, resources-list): connect runs in its own try/catch that returns the setup check; genuine post-connect failures keep their real check IDs. Rebased onto the version-aware connection abstraction (#318): scenarios now take a RunContext and connect via ctx.connect(); the setup/exec split and the reportSetupFailure helper carry over unchanged. This is the "-setup check as a first cut" from #248. Refs #248. --- src/connection/sdk-client.test.ts | 40 ++++++++++++++++++++++- src/connection/sdk-client.ts | 44 +++++++++++++++++++++++++- src/scenarios/server/lifecycle.test.ts | 23 ++++++++++++++ src/scenarios/server/lifecycle.ts | 21 +++--------- src/scenarios/server/prompts.ts | 12 +++++-- src/scenarios/server/resources.ts | 16 ++++++++-- src/scenarios/server/tools.ts | 15 +++++++-- 7 files changed, 146 insertions(+), 25 deletions(-) diff --git a/src/connection/sdk-client.test.ts b/src/connection/sdk-client.test.ts index 57cf7c4d..db6df0d9 100644 --- a/src/connection/sdk-client.test.ts +++ b/src/connection/sdk-client.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, afterEach } from 'vitest'; import express from 'express'; import type { Server as HttpServer } from 'http'; -import { connectToServer } from './sdk-client'; +import { connectToServer, reportSetupFailure } from './sdk-client'; // Regression guard for the session-termination behavior of close(): a bad // merge of connectToServer must not silently drop the DELETE (issue #79). @@ -75,3 +75,41 @@ describe('connectToServer close()', () => { expect(deletes).toEqual([]); }); }); + +describe('reportSetupFailure', () => { + it('emits a single FAILURE check id-d "-setup"', () => { + const checks = reportSetupFailure( + 'tools-list', + new Error('connect ECONNREFUSED') + ); + + expect(checks).toHaveLength(1); + expect(checks[0]).toMatchObject({ + id: 'tools-list-setup', + status: 'FAILURE', + errorMessage: 'Setup failed: connect ECONNREFUSED' + }); + }); + + it('stringifies a non-Error thrown value', () => { + const checks = reportSetupFailure('prompts-list', 'boom'); + + expect(checks[0]?.errorMessage).toBe('Setup failed: boom'); + }); + + it('attaches spec references when provided and omits the field otherwise', () => { + const withRefs = reportSetupFailure('resources-list', new Error('nope'), [ + { id: 'MCP-Resources-List' } + ]); + expect(withRefs[0]?.specReferences).toEqual([{ id: 'MCP-Resources-List' }]); + + const withoutRefs = reportSetupFailure('resources-list', new Error('nope')); + expect(withoutRefs[0]).not.toHaveProperty('specReferences'); + }); + + it('sets a timestamp', () => { + const checks = reportSetupFailure('server-initialize', new Error('x')); + expect(typeof checks[0]?.timestamp).toBe('string'); + expect(checks[0]?.timestamp.length).toBeGreaterThan(0); + }); +}); diff --git a/src/connection/sdk-client.ts b/src/connection/sdk-client.ts index 0d7575db..389e70c0 100644 --- a/src/connection/sdk-client.ts +++ b/src/connection/sdk-client.ts @@ -12,7 +12,7 @@ import { } from '@modelcontextprotocol/sdk/types.js'; import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; -import { type SpecVersion } from '../types'; +import { type ConformanceCheck, type SpecVersion } from '../types'; import { validateWireMessage, type WireOrigin @@ -114,6 +114,48 @@ function instrumentTransport( }); } +/** + * Emit a single `-setup` check as FAILURE for a scenario that + * could not get far enough to evaluate its real checks (connect failure, + * missing fixture, capability not advertised, etc.). + * + * See #248: previously each scenario hand-rolled a try/catch around connect + * and pinned the setup error onto whichever check ID happened to be first. + * That mislabels the failure — the error ends up under a check that has + * nothing to do with the actual problem, and any *other* checks the scenario + * would have emitted silently disappear. Routing setup failures through this + * helper gives them a dedicated, semantically honest ID and a consistent + * output shape across scenarios. + * + * The convention is that a scenario that cannot execute counts as a FAILURE; + * the escape hatches are scenario filtering (`--suite`/`--scenario`) and the + * expected-failures baseline, not in-scenario skipping or silent passes. + * + * @param scenarioName The scenario's `name`; the emitted check id is + * `-setup`. + * @param error The thrown setup error. + * @param specReferences Optional spec references to attach to the check. + * @returns A one-element array, so a scenario can `return reportSetupFailure(...)`. + */ +export function reportSetupFailure( + scenarioName: string, + error: unknown, + specReferences?: ConformanceCheck['specReferences'] +): ConformanceCheck[] { + const message = error instanceof Error ? error.message : String(error); + return [ + { + id: `${scenarioName}-setup`, + name: `${scenarioName} setup`, + description: `Scenario "${scenarioName}" could not be set up (connect/fixture/capability)`, + status: 'FAILURE', + timestamp: new Date().toISOString(), + errorMessage: `Setup failed: ${message}`, + ...(specReferences ? { specReferences } : {}) + } + ]; +} + /** * Create and connect an MCP client to a server. `opts.capabilities` and * `opts.clientInfo` override the harness defaults — scenarios that diff --git a/src/scenarios/server/lifecycle.test.ts b/src/scenarios/server/lifecycle.test.ts index ab3628a9..7e8f828b 100644 --- a/src/scenarios/server/lifecycle.test.ts +++ b/src/scenarios/server/lifecycle.test.ts @@ -7,6 +7,8 @@ vi.mock('../../connection/sdk-client', async (importOriginal) => { await importOriginal(); return { ...actual, + // Only the connection factory is mocked; reportSetupFailure stays real so + // the scenario's setup-failure path is exercised end-to-end. connectToServer: vi.fn() }; }); @@ -140,4 +142,25 @@ describe('ServerInitializeScenario', () => { ); expect(deleteCalls).toHaveLength(0); }); + + it('reports a single setup FAILURE when the connection cannot be established', async () => { + vi.mocked(connectToServer).mockRejectedValueOnce( + new Error('connect ECONNREFUSED 127.0.0.1:3000') + ); + + const checks = await new ServerInitializeScenario().run( + testContext(serverUrl) + ); + + // A connect failure should not be mislabeled as the initialize or + // session-id check failing (#248): a single dedicated setup check instead. + expect(checks).toHaveLength(1); + expect(checks[0]).toMatchObject({ + id: 'server-initialize-setup', + status: 'FAILURE', + errorMessage: 'Setup failed: connect ECONNREFUSED 127.0.0.1:3000' + }); + // The session-id check is never reached, so no raw fetch is attempted. + expect(fetchMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/scenarios/server/lifecycle.ts b/src/scenarios/server/lifecycle.ts index 7be721b1..7728e99e 100644 --- a/src/scenarios/server/lifecycle.ts +++ b/src/scenarios/server/lifecycle.ts @@ -10,6 +10,7 @@ import { import type { RunContext } from '../../connection'; import { connectToServer, + reportSetupFailure, terminateSessionRaw } from '../../connection/sdk-client'; @@ -73,22 +74,10 @@ and validates session ID format if one is assigned.`; await connection.close(); } catch (error) { - checks.push({ - id: 'server-initialize', - name: 'ServerInitialize', - description: - 'Server responds to initialize request with valid structure', - status: 'FAILURE', - timestamp: new Date().toISOString(), - errorMessage: `Failed to initialize: ${error instanceof Error ? error.message : String(error)}`, - specReferences: [ - { - id: 'MCP-Initialize', - url: 'https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle#initialization' - } - ] - }); - return checks; + // The handshake never completed, so neither the initialize check nor the + // session-id check below can be evaluated. Report a single setup failure + // rather than mislabeling it as one specific check failing (#248). + return reportSetupFailure(this.name, error); } // Check: Session ID visible ASCII validation diff --git a/src/scenarios/server/prompts.ts b/src/scenarios/server/prompts.ts index c5a5f380..25dc99cc 100644 --- a/src/scenarios/server/prompts.ts +++ b/src/scenarios/server/prompts.ts @@ -3,7 +3,8 @@ */ import { ClientScenario, ConformanceCheck } from '../../types'; -import type { RunContext } from '../../connection'; +import type { Connection, RunContext } from '../../connection'; +import { reportSetupFailure } from '../../connection/sdk-client'; import type { ListPromptsResult, GetPromptResult @@ -28,9 +29,16 @@ export class PromptsListScenario implements ClientScenario { async run(ctx: RunContext): Promise { const checks: ConformanceCheck[] = []; + let conn: Connection; try { - const conn = await ctx.connect(); + conn = await ctx.connect(); + } catch (error) { + // A connect failure isn't a `prompts-list` failure; report it as a setup + // failure rather than mislabeling the check (#248). + return reportSetupFailure(this.name, error); + } + try { const result = await conn.request('prompts/list'); // Validate response structure diff --git a/src/scenarios/server/resources.ts b/src/scenarios/server/resources.ts index 382b0db8..7029e854 100644 --- a/src/scenarios/server/resources.ts +++ b/src/scenarios/server/resources.ts @@ -7,7 +7,12 @@ import { ConformanceCheck, DRAFT_PROTOCOL_VERSION } from '../../types'; -import { JsonRpcError, type RunContext } from '../../connection'; +import { + JsonRpcError, + type Connection, + type RunContext +} from '../../connection'; +import { reportSetupFailure } from '../../connection/sdk-client'; import type { ListResourcesResult, ReadResourceResult, @@ -36,9 +41,16 @@ export class ResourcesListScenario implements ClientScenario { async run(ctx: RunContext): Promise { const checks: ConformanceCheck[] = []; + let conn: Connection; try { - const conn = await ctx.connect(); + conn = await ctx.connect(); + } catch (error) { + // A connect failure isn't a `resources-list` failure; report it as a + // setup failure rather than mislabeling the check (#248). + return reportSetupFailure(this.name, error); + } + try { const result = await conn.request('resources/list'); // Validate response structure diff --git a/src/scenarios/server/tools.ts b/src/scenarios/server/tools.ts index f18df3cf..da278f45 100644 --- a/src/scenarios/server/tools.ts +++ b/src/scenarios/server/tools.ts @@ -7,14 +7,15 @@ import { ConformanceCheck, DRAFT_PROTOCOL_VERSION } from '../../types'; -import type { RunContext } from '../../connection'; +import type { Connection, RunContext } from '../../connection'; import type { ListToolsResult, CallToolResult } from '../../spec-types/2025-06-18'; import { connectToServer, - NotificationCollector + NotificationCollector, + reportSetupFailure } from '../../connection/sdk-client'; import { CreateMessageRequestSchema, @@ -117,9 +118,17 @@ export class ToolsListScenario implements ClientScenario { async run(ctx: RunContext): Promise { const checks: ConformanceCheck[] = []; + let conn: Connection; try { - const conn = await ctx.connect(); + conn = await ctx.connect(); + } catch (error) { + // A connect failure isn't a `tools-list` failure; pinning it there would + // also drop the `tools-name-format` check entirely. Report it as setup + // (#248). + return reportSetupFailure(this.name, error); + } + try { const result = await conn.request('tools/list'); // Validate response structure From 10620fa9fd4e596cd5f5d83d88dc58eba4796b56 Mon Sep 17 00:00:00 2001 From: rinaldofesta Date: Sun, 9 Aug 2026 15:04:38 +0200 Subject: [PATCH 2/2] fix(lifecycle): keep a teardown failure out of the setup check connectToServer, the initialize SUCCESS push and close() all sat inside one try, so a close() failure after a completed handshake returned `server-initialize-setup` and discarded the SUCCESS just recorded. That is the same misattribution this PR exists to remove, in the scenario it is named after; prompts.ts, resources.ts and tools.ts already isolate the connect call. Isolate connect so only a handshake failure reports as setup. Teardown is best-effort at this layer to match terminateSessionRaw, which already swallows the session-terminating DELETE, so the only thing that can still reject is the client-side close, and that is not a server conformance result. --- src/scenarios/server/lifecycle.test.ts | 16 ++++++++ src/scenarios/server/lifecycle.ts | 55 +++++++++++++++----------- 2 files changed, 47 insertions(+), 24 deletions(-) diff --git a/src/scenarios/server/lifecycle.test.ts b/src/scenarios/server/lifecycle.test.ts index 7e8f828b..f283f0ac 100644 --- a/src/scenarios/server/lifecycle.test.ts +++ b/src/scenarios/server/lifecycle.test.ts @@ -163,4 +163,20 @@ describe('ServerInitializeScenario', () => { // The session-id check is never reached, so no raw fetch is attempted. expect(fetchMock).not.toHaveBeenCalled(); }); + + it('keeps the initialize SUCCESS when the client close fails', async () => { + fetchMock.mockResolvedValue(new Response(null)); + closeMock.mockRejectedValueOnce(new Error('transport already closed')); + + const checks = await new ServerInitializeScenario().run( + testContext(serverUrl) + ); + + // Teardown is best-effort, so a close failure must neither retract the + // recorded SUCCESS nor be reported as a setup failure (#248). + expect(checks).toContainEqual( + expect.objectContaining({ id: 'server-initialize', status: 'SUCCESS' }) + ); + expect(checks.some((c) => c.id === 'server-initialize-setup')).toBe(false); + }); }); diff --git a/src/scenarios/server/lifecycle.ts b/src/scenarios/server/lifecycle.ts index 7728e99e..a1bde8f1 100644 --- a/src/scenarios/server/lifecycle.ts +++ b/src/scenarios/server/lifecycle.ts @@ -10,6 +10,7 @@ import { import type { RunContext } from '../../connection'; import { connectToServer, + type MCPClientConnection, reportSetupFailure, terminateSessionRaw } from '../../connection/sdk-client'; @@ -48,31 +49,9 @@ and validates session ID format if one is assigned.`; const { serverUrl } = ctx; const checks: ConformanceCheck[] = []; + let connection: MCPClientConnection; try { - const connection = await connectToServer(serverUrl, {}, ctx.specVersion); - - // The connection process already does initialization - // Check that we have a connected client - checks.push({ - id: 'server-initialize', - name: 'ServerInitialize', - description: - 'Server responds to initialize request with valid structure', - status: 'SUCCESS', - timestamp: new Date().toISOString(), - specReferences: [ - { - id: 'MCP-Initialize', - url: 'https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle#initialization' - } - ], - details: { - serverUrl, - connected: true - } - }); - - await connection.close(); + connection = await connectToServer(serverUrl, {}, ctx.specVersion); } catch (error) { // The handshake never completed, so neither the initialize check nor the // session-id check below can be evaluated. Report a single setup failure @@ -80,6 +59,34 @@ and validates session ID format if one is assigned.`; return reportSetupFailure(this.name, error); } + // The connection process already does initialization + // Check that we have a connected client + checks.push({ + id: 'server-initialize', + name: 'ServerInitialize', + description: 'Server responds to initialize request with valid structure', + status: 'SUCCESS', + timestamp: new Date().toISOString(), + specReferences: [ + { + id: 'MCP-Initialize', + url: 'https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle#initialization' + } + ], + details: { + serverUrl, + connected: true + } + }); + + // Teardown on an already-recorded handshake. The session-terminating + // DELETE inside close() is best-effort by design (see terminateSessionRaw), + // so what can still reject here is the client-side close, which is not a + // server conformance result. Keeping it in the connect try above reported + // it as `server-initialize-setup` and dropped the SUCCESS just recorded, + // which is the misattribution #248 is about. + await connection.close().catch(() => {}); + // Check: Session ID visible ASCII validation // Use a raw fetch to inspect the MCP-Session-Id response header, // since the SDK client transport does not expose it.