Uh oh!
There was an error while loading. Please reload this page.
test(e2e): host the sse matrix column on the shipped legacy SSEServerTransport - #2229
Conversation
|
@modelcontextprotocol/client@modelcontextprotocol/codemod@modelcontextprotocol/server@modelcontextprotocol/server-legacy@modelcontextprotocol/express@modelcontextprotocol/fastify@modelcontextprotocol/hono@modelcontextprotocol/nodecommit: |
There was a problem hiding this comment.
LGTM — test-only wiring that swaps the hand-written SSE bridge for the shipped SSEServerTransport and lifts the now-resolved deferred/knownFailure entries.
Extended reasoning...
Overview
This PR only touches the e2e test workspace (test/e2e/**), its package.json/tsconfig.json path mappings, and pnpm-lock.yaml. It replaces the hand-written LegacySseServerTransport test bridge in helpers/sse-host.ts with the real SSEServerTransport from @modelcontextprotocol/server-legacy/sse (shipped by #2206), removes the corresponding knownFailures/deferred entries in requirements.ts, and adds two new compat flow tests (dual-transport hosting and streamable→SSE fallback). No production package code is modified.
Correctness checks
I verified the constructor and surface usage against the shipped transport: new SSEServerTransport(POST_PATH, res) matches packages/server-legacy/src/sse/sse.ts:47, and the host's use of sessionId, handlePostMessage, and close() all exist with compatible semantics (the shipped transport writes the SSE headers and endpoint event in start(), exactly as the deleted bridge did). SdkHttpError is exported from @modelcontextprotocol/client via the core public re-export, and a POST of initialize to the SSE host's /sse path falls through to the 404 branch, which the streamable HTTP client surfaces as an SdkHttpError with status 404 — so the fallback test's 400/404/405 assertion is sound. The new requirement notes accurately describe the new behavior (no prose/code mismatch).
Security risks
None — this is test infrastructure only; the loopback HTTP listener binds to 127.0.0.1 on an ephemeral port inside the test process and nothing here ships in any published package.
Level of scrutiny
Low-to-medium: the diff is moderately sized but is a net deletion of test-only bridge code in favor of exercising the real shipped transport, which strictly improves coverage fidelity for the back-compat claim made by #2206. The remaining risk (e.g., the stacked dependency on #2206 being present) is fully covered by CI — if the base PR isn't merged, typecheck/tests fail loudly. The bug-hunting pass found no issues, the author reports the full e2e suite passing with no manifest churn, and the changeset-bot warning is expected since @modelcontextprotocol/test-e2e is a private test package.
…Transport The sse column was hosted by a hand-written test-only bridge because v2 removed the server-side SSE transport. Now that the server-legacy package ships SSEServerTransport, the matrix can exercise the real transport end to end: - replace the bridge transport in helpers/sse-host.ts with SSEServerTransport from @modelcontextprotocol/server-legacy/sse - resolve the transport:sse:server-transport knownFailure and probe the legacy package in its surface test - un-defer and implement the two flow:compat requirements: dual-transport hosting (streamable HTTP + SSE serving the same tools concurrently) and streamable-then-SSE fallback on 4xx
d292267 to
acd9fbaCompareUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
LGTM — test-only wiring that swaps the e2e SSE host's hand-written bridge for the shipped SSEServerTransport from #2206; no runtime or public-API changes.
Extended reasoning...
Overview
This PR is confined to the e2e test workspace (test/e2e/) plus a pnpm-lock.yaml entry for the new workspace link. It replaces the hand-written LegacySseServerTransport bridge in helpers/sse-host.ts with the real SSEServerTransport shipped by @modelcontextprotocol/server-legacy/sse (added in #2206), removes the now-resolved knownFailures/deferred markers from three requirements, and adds two new flow tests (dual-transport hosting and streamable→SSE fallback). I verified the shipped transport's surface against packages/server-legacy/src/sse/sse.ts — the constructor signature (endpoint, res), sessionId getter, handlePostMessage(req, res), and close() all match how the host helper uses it, and server.connect(tx) calling start() correctly emits the SSE headers and endpoint event the SSEClientTransport expects.
Security risks
None. No production code, auth, crypto, or transport implementation is touched — only test helpers, test scenarios, requirement metadata, the test package's devDependencies/tsconfig paths, and the lockfile link. The fallback test correctly relies on the host returning 404 for a POST to the SSE path, which is within the spec's 400/404/405 fallback signal set.
Level of scrutiny
Low-to-moderate. This is test infrastructure for the e2e back-compat matrix, not shipped SDK code. The riskiest aspect would be a test that passes for the wrong reason, but the swap actually strengthens coverage: the matrix now exercises both shipped halves of the legacy transport rather than a test-only stand-in, and the previously-deferred compat flows are now implemented against the real server transport. The new fallback test asserts the spec-mandated 4xx fallback behavior with a concrete spec citation.
Other factors
No changeset is needed (@modelcontextprotocol/test-e2e is private, and the changeset bot's note is informational). The bug hunting system found no issues. The author reports the full e2e suite (1142 passed, 91 expected-fail, 0 unexpected) plus typecheck/lint clean, and the diff is consistent with that claim — the removed knownFailure for transport:sse:server-transport is genuinely satisfied because SSEServerTransport matches the test's /sse/i + /server/i export filter. The PR follows the existing host/wire patterns in the e2e helpers (per-session factory, 404 for unknown sessions, 500 on handler failure).
Wire the e2e suite to the legacy SSE transport shipped by #2206: the
ssematrix column now runs against the realSSEServerTransport, and the SSE back-compat requirements that were recorded as failures/deferred are now implemented.Motivation and Context
#2206 ships
SSEServerTransportin@modelcontextprotocol/server-legacy, but the e2e suite still records the SDK as not providing one:transport:sse:server-transportwas a knownFailure ("the server-side SSE transport was removed from the SDK")ssematrix column was hosted by a hand-written test-only bridge (helpers/sse-host.ts)flow:compat:*requirements were deferred ("flows that require an SSE server are deferred")This stacks on #2206 and resolves all three, so the package's back-compat claim is proven by the matrix (real
SSEClientTransportagainst the real shipped server transport) rather than only by its ported v1 unit tests.How Has This Been Tested?
Breaking Changes
None — test-only changes.
Types of changes
Checklist
Additional context
helpers/sse-host.tskeeps its loopback listener and per-session routing; only the transport class is swapped (hand-written bridge → shippedSSEServerTransport)