test(e2e): hosting-entry-http arm-posture fix; widen method-405 probe set#2334
test(e2e): hosting-entry-http arm-posture fix; widen method-405 probe set#2334felixweinberger wants to merge 2 commits into
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only cleanup that drops a redundant/incorrect entry override and widens the method-405 probe set; the helper's spread order confirms the fix is correct.
Extended reasoning...
Overview
This PR touches only the e2e test harness: test/e2e/requirements.ts (reworded behavior/note text for hosting:entry:method-405) and test/e2e/scenarios/hosting-entry-http.test.ts (drops the { entry: { legacy: 'stateless' } } override from three cells and widens the method-405 probe loop from PUT/PATCH to GET/DELETE/PUT/PATCH). No SDK source code is modified.
Correctness
I verified the rationale against test/e2e/helpers/index.ts:151: the helper builds the entry config as { legacy: 'reject', ...sniff.entry } on entryModern, so the previous per-test override genuinely replaced the arm posture and made the entryModern cells config-duplicates of entryStateless. Removing the override restores the documented arm posture as the configuration under test. The widened probe set and the requirements wording match each other and the existing assertion shape; the author reports 10/10 locally and the full matrix unchanged.
Security risks
None — test-only change with no runtime, auth, or transport behavior modified.
Level of scrutiny
Low. This is a small, self-contained coverage-fidelity fix in the e2e matrix following an established pattern. The one finding from the bug-hunting pass is a non-blocking nit about the same leftover override surviving in a sibling file outside this diff (hosting-entry.test.ts notification-202 cell); it does not affect the correctness of this PR and is already surfaced as an inline comment.
Other factors
No changeset is needed (test-only). The branch is stacked on fweinberger/on-m12 and carries an empty marker commit the author flags for dropping on squash — neither affects the review verdict.
| verifies('typescript:hosting:entry:method-405', async ({ transport }: TestArgs) => { | ||
| const client = new Client({ name: 'method-405-client', version: '1.0.0' }); | ||
| await using wired = await wire(transport, echoFactory, client, { entry: { legacy: 'stateless' } }); | ||
| // No `entry` override: the arm posture (`stateless` on entryStateless, | ||
| // `reject` on entryModern) is the configuration under test. | ||
| await using wired = await wire(transport, echoFactory, client); |
There was a problem hiding this comment.
🟡 The same arm-posture override this PR removes still survives in the sibling file at test/e2e/scenarios/hosting-entry.test.ts:126 — the notification-202 cell passes { entry: { legacy: 'stateless' } }, so its entryModern arm silently runs legacy: 'stateless' instead of the documented 'reject' posture. The assertion still passes either way, so this is just a coverage-fidelity cleanup: drop the override there too (the one at line 39, dual-era-one-factory, is intentional and should stay).
Extended reasoning...
What the bug is. This PR removes the wire(..., { entry: { legacy: 'stateless' } }) override from method-405, parse-error-400, and no-session-id in hosting-entry-http.test.ts, on the rationale that the helper spreads the override after the arm posture, so on entryModern the handler silently runs dual-era stateless instead of the arm's documented modern-only strict (legacy: 'reject'). Exactly one more leftover instance of that same pattern survives outside the diff: the typescript:hosting:entry:notification-202 cell at test/e2e/scenarios/hosting-entry.test.ts:126.
The code path. wire() builds the entry config at test/e2e/helpers/index.ts:151 as transport === 'entryStateless' ? { legacy: 'stateless', ...sniff.entry } : { legacy: 'reject', ...sniff.entry } — sniff.entry wins. notification-202 runs on both ['entryStateless', 'entryModern'] arms (requirements.ts), so on the entryModern arm the passed { legacy: 'stateless' } replaces 'reject' and the cell hosts a dual-era stateless handler rather than the modern-only strict endpoint the file header and test/e2e/CLAUDE.md document for that arm.
Step-by-step proof. (1) The matrix schedules notification-202 on entryModern. (2) wire('entryModern', greetFactory, client, { entry: { legacy: 'stateless' } }) reaches helpers/index.ts:151 and evaluates { legacy: 'reject', ...{ legacy: 'stateless' } } → { legacy: 'stateless' }. (3) createMcpHandler is therefore created with the legacy-stateless slot enabled — the same configuration the entryStateless arm already covers. (4) The test then sends the enveloped 2026-era notifications/cancelled, which rides the modern leg under either posture and gets 202, so the test still passes — but the 202-for-enveloped-notification behavior is never observed under the strict (legacy: 'reject') posture this arm exists to test.
Why nothing prevents it. The override is silent: nothing warns that a per-test entry override contradicts the arm posture, and the assertion is posture-insensitive, so the matrix stays green while the entryModern cell is effectively a config-near-duplicate of entryStateless for the handler configuration.
Why line 39 is excluded. The other surviving site, hosting-entry.test.ts:39 (dual-era-one-factory), is intentional — its requirement explicitly pins the same one-factory, legacy-stateless-slot handler shape on both arms, so the override there is the configuration under test. The notification-202 requirement note only says the arm selects which leg the notification rides; it never asks for the stateless slot on entryModern.
Impact and fix. No test failure — purely a coverage-fidelity gap and an inconsistency with the principle this PR states ('the arm posture is the configuration under test'). Fix is the same one-line change applied to the three cells in this PR: drop the fourth argument at hosting-entry.test.ts:126 so the entryModern arm actually exercises legacy: 'reject'. Worth folding into this PR since it completes the cleanup, but non-blocking.
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
6f302eb to
e579ca8
Compare
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
d81cc51 to
5725817
Compare
No SDK changes. Design proposals written to the project docs/ tree (outside this worktree): - docs/2026-06-19-sep2106-design.md - docs/2026-06-19-client-caching-design.md Report lines appended to logs/agent-reports/37-overnight.md.
… set review-3 on #33: O1 MEDIUM — method-405 / parse-error-400 / no-session-id passed { entry: { legacy: 'stateless' } } to wire(), which spreads after the arm posture and so overrode entryModern's legacy:'reject' with 'stateless'. The entryModern cells were config-duplicates of entryStateless. Dropped the override entirely: the arm posture ('stateless' on entryStateless, 'reject' on entryModern) is exactly what each cell should test, so no override is needed. O2 LOW — method-405 now probes GET, DELETE, PUT, PATCH (was PUT, PATCH only). Both legs answer 405 / -32000 / 'Method not allowed.' for every non-POST method. Note reworded ('POSTed through' -> 'sent through'); behavior text updated to name the four methods now covered.
There was a problem hiding this comment.
LGTM — test-only fix; verified the wire() spread order makes the removed override the wrong posture on entryModern, and both legs answer 405/-32000 for all four non-POST methods.
Extended reasoning...
Overview
This PR touches only test/e2e/requirements.ts (requirement wording) and test/e2e/scenarios/hosting-entry-http.test.ts. It removes the { entry: { legacy: 'stateless' } } override from three cells so each arm runs its documented posture, and widens the method-405 probe loop from PUT/PATCH to GET/DELETE/PUT/PATCH.
Verification
I confirmed the PR's premise in the helper: test/e2e/helpers/index.ts:151 spreads sniff.entry after the arm posture, so the old override silently turned the entryModern arm into a stateless duplicate. I also confirmed both legs answer the widened probe set identically — non-POST requests classify as the http-method route (packages/core/src/shared/inboundClassification.ts:801-804), which the legacy stateless fallback rejects with 405/-32000 and the modern-only strict path rejects via modernOnlyStrictRejection (inboundClassification.ts:856-858) with the same status/code/message, matching the test assertions.
Security risks
None — no SDK/runtime code is touched; the changes are confined to e2e test scenarios and requirement prose.
Level of scrutiny
Low: test-only, small, self-contained, with clear intent and assertions that would fail in CI if the posture change altered observed behavior. The only finding from this round is a pre-existing SDK gap (missing Allow header on the entry-side 405 paths) that is unrelated to and not introduced by this PR; my earlier inline comment about a leftover override in the sibling hosting-entry.test.ts is a non-blocking coverage-fidelity cleanup outside this diff.
Other factors
The author reports the scenario file passes 10/10 and the full matrix is unchanged; no changeset is needed for test-only changes.
| 'typescript:hosting:entry:method-405': { | ||
| source: 'sdk', | ||
| behavior: | ||
| 'An unsupported HTTP method (PUT, PATCH) on a createMcpHandler endpoint is answered 405 with a JSON-RPC Method-not-allowed body on both legs: the stateless legacy fallback rejects every non-POST method, and the modern-only strict path rejects body-less non-POST traffic via the modern-only-method-not-allowed cell.', | ||
| 'A non-POST HTTP method (GET, DELETE, PUT, PATCH) on a createMcpHandler endpoint is answered 405 with a JSON-RPC Method-not-allowed body on both legs: the stateless legacy fallback rejects every non-POST method, and the modern-only strict path rejects body-less non-POST traffic via the modern-only-method-not-allowed cell.', | ||
| transports: ['entryStateless', 'entryModern'], | ||
| note: 'Runs on the createMcpHandler entry arms; the unsupported methods are POSTed through wired.fetch so the HTTP status and body are observed directly. The entry does not emit an Allow header (the per-session server transport does), so only the status and JSON-RPC error shape are pinned.' | ||
| note: 'Runs on the createMcpHandler entry arms; each non-POST method is sent through wired.fetch so the HTTP status and body are observed directly. The entry does not emit an Allow header (the per-session server transport does), so only the status and JSON-RPC error shape are pinned.' | ||
| }, |
There was a problem hiding this comment.
🟣 Pre-existing SDK gap, unrelated to this test-only PR: both 405 paths reachable through createMcpHandler (the legacy stateless fallback's non-POST rejection and the modern-only strict modern-only-method-not-allowed rejection) return 405 without the Allow header that RFC 9110 §15.5.6 requires, even though the per-session WebStandardStreamableHTTPServerTransport already emits Allow: GET, POST, DELETE on its 405. The eventual fix is to emit Allow: POST from the two entry-side 405 sites — at which point the requirement note reworded here ("The entry does not emit an Allow header…") will need updating.
Extended reasoning...
What the gap is. RFC 9110 §15.5.6 says an origin server generating a 405 (Method Not Allowed) response MUST generate an Allow header field listing the methods the target resource supports. Both 405 paths reachable through createMcpHandler omit it:
- The legacy stateless fallback's non-POST rejection at
packages/server/src/server/createMcpHandler.ts:309-311returnsjsonRpcErrorResponse(405, -32000, 'Method not allowed.'), andjsonRpcErrorResponse(lines 261-270) builds theResponse.jsonwith only a status — no headers. - The modern-only strict path's
modern-only-method-not-allowedrejection atpackages/core/src/shared/inboundClassification.ts:856-858is rendered throughrejectionResponse()(createMcpHandler.ts:272-274), which delegates to the same header-lessjsonRpcErrorResponse.
Why it's an inconsistency, not a design choice. The per-session WebStandardStreamableHTTPServerTransport already complies: its handleUnsupportedRequest emits Allow: 'GET, POST, DELETE' on its 405 (packages/server/src/server/streamableHttp.ts:625). So the SDK follows the RFC on the sibling transport but not on the two entry-side sites — a parity gap rather than a deliberate omission.
How this PR interacts with it. The PR is test-only and does not introduce the gap — the sentence "The entry does not emit an Allow header (the per-session server transport does)" already appears verbatim in the line being replaced. But the PR widens the method-405 probe loop from ['PUT','PATCH'] to ['GET','DELETE','PUT','PATCH'] and rewords the requirement note at test/e2e/requirements.ts:2405-2411, so it now documents and observes (without asserting) the non-compliant 405 shape for four methods on both entry arms.
Step-by-step proof. (1) The widened loop sends GET to the entryStateless arm's URL via wired.fetch. (2) createMcpHandler routes the body-less non-POST request to legacyStatelessFallback, which hits the request.method !== 'POST' guard at createMcpHandler.ts:309 and returns jsonRpcErrorResponse(405, -32000, 'Method not allowed.'). (3) jsonRpcErrorResponse constructs the response with { status: 405 } only, so response.headers.get('allow') is null — violating the RFC 9110 MUST. (4) On the entryModern arm the same GET is classified as modern-only-method-not-allowed (inboundClassification.ts:856-858) and rendered by rejectionResponse() → the same header-less builder, so Allow is again absent. (5) By contrast, sending PUT to a hand-hosted per-session transport reaches handleUnsupportedRequest at streamableHttp.ts:611-625 and gets Allow: GET, POST, DELETE back.
Impact and fix. Impact is HTTP-compliance/interop polish: well-behaved generic HTTP clients and proxies use Allow to discover supported methods after a 405. The fix is a one-liner per site — emit Allow: 'POST' (the only method either entry leg serves) from the two entry-side 405 paths. That change belongs in an SDK PR, not this test-only one; when it lands, the requirement note text touched here (and the note that the test deliberately doesn't pin the header) should be updated to assert Allow: POST. Filing as pre_existing so it's tracked without blocking this PR.
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
e579ca8 to
ae54743
Compare
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
5725817 to
9fcdcc2
Compare
There was a problem hiding this comment.
LGTM — test-only fix that makes the entryModern cells actually exercise the 'reject' posture, plus a safe widening of the non-POST probe set.
Extended reasoning...
Overview
Test-only PR touching two e2e files: test/e2e/scenarios/hosting-entry-http.test.ts (drops the { entry: { legacy: 'stateless' } } override from three cells and widens the method-405 probe loop to GET/DELETE/PUT/PATCH) and test/e2e/requirements.ts (rewords the matching requirement note). No SDK/runtime code is changed.
Correctness
The override removal is sound: wire() spreads the per-test entry override after the arm posture, so the old override silently turned the entryModern arm into a duplicate of entryStateless. Dropping it makes each arm test its documented posture. The probe widening is safe on both legs — legacyStatelessFallback rejects every non-POST with 405/−32000 (createMcpHandler.ts:309-310), and the modern-only strict path returns the identical 405/−32000 "Method not allowed." rejection for any non-POST (inboundClassification.ts:856-857), so GET/DELETE assert the same shape PUT/PATCH already did.
Security risks
None — no production code paths are modified; the change only affects which configuration the e2e harness hosts and which HTTP methods are probed.
Level of scrutiny
Low. Small, self-contained test-coverage-fidelity fix with clear motivation, author-reported local pass (10/10 in the file, full matrix unchanged), and no public API or behavior impact. The two prior inline comments from earlier runs are non-blocking (a sibling-file cleanup suggestion and a pre-existing SDK Allow-header gap) and don't bear on this PR's correctness.
Other factors
No changeset is needed (test-only). The bug-hunting system found no bugs in this revision.
There was a problem hiding this comment.
LGTM — test-only fix; the dropped entry override was indeed clobbering the entryModern arm's legacy: 'reject' posture (the helper spreads sniff.entry last), and the widened non-POST probe set is answered 405/−32000 on both legs.
Extended reasoning...
Overview
This PR touches only test/e2e/requirements.ts (requirement-note rewording for method-405) and test/e2e/scenarios/hosting-entry-http.test.ts (drops a per-test { entry: { legacy: 'stateless' } } override from three cells and widens the method-405 probe loop from PUT/PATCH to GET/DELETE/PUT/PATCH). No SDK source, public API, or production code paths are modified.
Verification of the premise
I confirmed in test/e2e/helpers/index.ts:151 that wire() spreads sniff.entry after the arm posture, so the removed override did force legacy: 'stateless' on the entryModern arm — the PR's stated rationale is accurate, and removing the override makes the arm posture the configuration under test as documented. I also confirmed both 405 paths the widened probe set exercises: the legacy stateless fallback rejects every non-POST method, and the modern-only strict path returns the modern-only-method-not-allowed rejection (405, −32000, 'Method not allowed.') in packages/core/src/shared/inboundClassification.ts:856-858, so GET and DELETE behave identically to PUT/PATCH on both arms.
Security risks
None — test-only changes; nothing touches auth, transports, or request handling in shipped code.
Level of scrutiny
Low. This is a small, self-contained e2e test-fidelity fix with a clear and verified rationale, no behavior change to the SDK, no changeset needed, and the author reports the scenario file passes 10/10 plus a clean full-matrix run.
Other factors
The two prior inline comments from earlier review runs are non-blocking (a leftover override in a sibling file and a pre-existing missing-Allow-header SDK gap); neither affects the correctness of this PR.
1 participant
Fixes the arm-posture override in the
hosting-entry-httpe2e scenarios introduced infweinberger/on-m12and widens themethod-405probe set. Stacked onfweinberger/on-m12.Motivation and Context
method-405/parse-error-400/no-session-idwere passingwire(..., { entry: { legacy: 'stateless' } }); the helper spreadssniff.entryafter the arm posture, so onentryModernthe handler ranlegacy: 'stateless'instead of'reject'— theentryModerncells were config-duplicates ofentryStateless.Fix: drop the override entirely. The arm posture (
'stateless'onentryStateless,'reject'onentryModern) is exactly what each cell should test; the override was redundant on one arm and wrong on the other. Verified the modern-strict path answers identically for these probes (non-POST → 405/−32000; non-JSON POST → 400/−32700; noMcp-Session-Idemitted).Also widens the
method-405probe loop['PUT','PATCH']→['GET','DELETE','PUT','PATCH'](both legs answer 405/−32000/"Method not allowed." for every non-POST method) and rewords therequirements.tsnote to match.How Has This Been Tested?
hosting-entry-http.test.ts10/10scenarios/stdio.test.tsspawn fails are pre-existing at the base and unrelated (sandbox artifacts)Breaking Changes
None. Test-only.
Types of changes
Checklist
Additional context
This branch also carries an empty marker commit (
951baaae5) — no SDK changes; can be dropped on squash.