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..f283f0ac 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,41 @@ 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(); + }); + + 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 7be721b1..a1bde8f1 100644 --- a/src/scenarios/server/lifecycle.ts +++ b/src/scenarios/server/lifecycle.ts @@ -10,6 +10,8 @@ import { import type { RunContext } from '../../connection'; import { connectToServer, + type MCPClientConnection, + reportSetupFailure, terminateSessionRaw } from '../../connection/sdk-client'; @@ -47,50 +49,44 @@ 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) { - 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); } + // 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. 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