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.
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
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.
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
ae54743 to
7eb29bc
Compare
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
9fcdcc2 to
246c77f
Compare
There was a problem hiding this comment.
LGTM — test-only fix that makes the entryModern cells actually exercise the documented strict posture, plus a sound widening of the non-POST probe set; my earlier inline notes are non-blocking.
Extended reasoning...
Overview
This PR touches only the e2e harness: 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 source code changes.
Correctness verification
The stated rationale checks out against the helper: wire() builds the entry config as { legacy: 'reject', ...sniff.entry } on entryModern (test/e2e/helpers/index.ts:151), so the previous override silently replaced the strict posture and made those cells config-duplicates of entryStateless. Removing the override restores the documented per-arm posture. The widened probe set is safe on both arms: the legacy stateless fallback rejects every non-POST method with 405/−32000 (packages/server/src/server/createMcpHandler.ts:309-310), and the modern-only strict path returns the same shape via modern-only-method-not-allowed (packages/core/src/shared/inboundClassification.ts:862).
Security risks
None — test-only changes; no auth, transport, or protocol logic is modified.
Level of scrutiny
Low. This is e2e test coverage hygiene with no public API or behavior impact, and the author reports the scenario file passing 10/10 plus a full-matrix run with no new failures.
Other factors
The bug-hunting system found no bugs in this revision. My two earlier inline comments (a leftover override in the sibling hosting-entry.test.ts and a pre-existing missing Allow header in the SDK's entry-side 405s) are explicitly non-blocking and out of this PR's scope, so they don't gate approval.
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
7eb29bc to
d0bbf34
Compare
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
246c77f to
601893a
Compare
There was a problem hiding this comment.
LGTM — test-only fix that drops the misordered entry override so each arm exercises its documented posture, widens the method-405 probe set, and also folds in the same cleanup for the notification-202 cell raised earlier.
Extended reasoning...
Overview
The PR touches three e2e test files only: test/e2e/requirements.ts (rewords the method-405 requirement note for the wider probe set), test/e2e/scenarios/hosting-entry-http.test.ts (drops the { entry: { legacy: 'stateless' } } override from method-405, parse-error-400, and no-session-id, and widens the probe loop to GET/DELETE/PUT/PATCH), and test/e2e/scenarios/hosting-entry.test.ts (drops the same override from notification-202, addressing the earlier inline comment). No SDK package code changes.
Security risks
None. No production code, auth, crypto, or transport logic is modified — only test wiring and a requirement-prose update.
Level of scrutiny
Low. The change is a small, self-contained coverage-fidelity fix in the e2e harness. The rationale is sound: wire() spreads the per-test entry override after the arm posture, so the override silently flattened the entryModern arm into a duplicate of entryStateless; removing it makes each arm exercise its documented configuration (legacy: 'stateless' vs 'reject'). The intentional override at the dual-era-one-factory cell is correctly left in place. The widened probe set only adds methods both entry legs already reject identically (405 / −32000), and the author reports 10/10 locally plus an unchanged full-matrix cell count.
Other factors
The earlier 🟡 inline finding (the surviving override on notification-202) is addressed in the current diff. The 🟣 missing-Allow-header note is a pre-existing SDK gap unrelated to this test-only change and explicitly non-blocking. No changeset is needed for test-only changes. No outstanding human reviewer comments.
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
d0bbf34 to
463350c
Compare
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
601893a to
0f18a51
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.
felixweinberger
force-pushed
the
fweinberger/on-e2e
branch
from
0f18a51 to
3c4a829
Compare
felixweinberger
force-pushed
the
fweinberger/on-m12
branch
from
463350c to
109bc4f
Compare
There was a problem hiding this comment.
LGTM — test-only cleanup; the arm-posture override removal is correct given how wire() spreads sniff.entry after the arm default, and the notification-202 leftover from my earlier comment is now folded in.
Extended reasoning...
Overview
This PR touches only e2e test files: test/e2e/requirements.ts (requirement note rewording), 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 all four non-POST methods), and test/e2e/scenarios/hosting-entry.test.ts (drops the same override from notification-202). No SDK/package source code is modified.
Correctness
I verified the PR's central claim against test/e2e/helpers/index.ts:151: the helper spreads sniff.entry after the arm posture, so the removed override did indeed turn the entryModern cells into config-duplicates of entryStateless. Dropping it restores the documented per-arm posture (stateless vs reject). The widened probe set (GET/DELETE/PUT/PATCH) matches the entry's behavior of rejecting every non-POST method with 405/-32000 on both legs. The only surviving override (hosting-entry.test.ts:39, dual-era-one-factory) is intentional per its requirement.
Security risks
None — test-only change with no production code paths affected.
Level of scrutiny
Low. Test-only, small, mechanical, with clear intent and an accurate description. The author reports the affected scenario file passes 10/10 and the full matrix is unchanged. The earlier inline feedback (notification-202 override) is addressed in the current diff; the other prior comment was a pre-existing SDK gap (missing Allow header) explicitly out of scope for this PR.
Other factors
No changeset is needed (test-only, changeset-bot warning is expected). The bug-hunting system found no issues on this revision.
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.