fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body by ump45nose · Pull Request #1849 · modelcontextprotocol/csharp-sdk

ump45nose · 2026-09-01T02:57:52Z

Summary

fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body

Verification

dotnet test tests/ModelContextProtocol.Tests --framework net10.0 -c Release --filter 'FullyQualifiedName~~HttpClientTransportAutoDetectTests' (10 passed); 'FullyQualifiedName~~July2026ProtocolFallbackTests|FullyQualifiedName~HttpClientTransportTests' (34 passed)

Related to #1848

AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. The submission was checked against the prepared artifact and recorded verification evidence.

halter73

#1791 merged after this PR was opened and now conflicts with these changes. Could you rebase and update this to apply the spec’s full 400/404/405 allowlist, while preserving 401/403/5xx without trying SSE? Tests covering both structured and unstructured responses would make this ready for another pass.

This should also supersede #1444 once merged.

halter73 · 2026-09-04T01:16:54Z

                ActiveTransport = streamableHttpTransport;
            }
-            else if (await StreamableHttpClientSessionTransport.TryReadJsonRpcErrorAsync(response, cancellationToken).ConfigureAwait(false) is { } parsedError)
+            else if (response.StatusCode != HttpStatusCode.MethodNotAllowed &&


This skips recognized modern errors on every 405, while unstructured 401/403/5xx responses still fall through to SSE. The spec limits this fallback to 400, 404, or 405 when the body is not a recognized modern JSON-RPC error. Can we classify the full response here instead of special-casing only 405?

halter73 · 2026-09-04T01:16:54Z

-                        ex.GetStatusCode() is HttpStatusCode.BadRequest or HttpStatusCode.NotFound)
+                        ex.GetStatusCode() is HttpStatusCode.BadRequest
+                                              or HttpStatusCode.NotFound
+                                              or HttpStatusCode.MethodNotAllowed)


A 405 here means the POST endpoint rejected the request, so retrying initialize over the same transport is not useful. The spec's 405 handling is the AutoDetect transport fallback to SSE. Can we keep that in AutoDetectingClientSessionTransport and remove 405 from this catch?

- 仅对 400、404、405 探测失败回退初始化握手。 - 保留 401、403、5xx 的 HTTP 语义且不发起 SSE 请求。 - 覆盖结构化与非结构化探测响应的回归场景。

ump45nose · 2026-09-04T07:05:49Z

Rebased onto current main and resolved the #1791 conflict at 32ef18a. The probe fallback is now explicitly limited to 400/404/405 for both structured and unstructured responses; 401/403/5xx preserve their HTTP failure without an SSE GET. The regression matrix covers both transport modes and asserts that non-allowlisted AutoDetect failures do not request SSE.\n\nI could not run the local .NET suite because this host has only SDK 6.0.136 while global.json requires 10.0.101; I did run git diff --check. The fork CI workflows are currently awaiting maintainer approval.\n\nAI-assisted maintenance; I reviewed the conflict resolution and validation evidence.

405 means the POST endpoint rejected the request method, so retrying initialize over the same transport is not useful. The spec's 405 handling is the AutoDetect transport's SSE fallback. Remove MethodNotAllowed from McpClientImpl's HTTP-layer fallback catch and route the regression coverage through the AutoDetect test matrix. Also realign three AutoDetect transport tests that still encoded the pre-allowlist 'always fall back to SSE' behavior (403/415), switching them to an allowlisted 404 so they continue to exercise the dual-failure surface path.

ump45nose · 2026-09-09T10:58:48Z

Addressed the second review point: removed 405/MethodNotAllowed from the discover-probe initialize fallback in McpClientImpl. A 405 means the POST endpoint rejected the request method, so retrying initialize over the same transport is not useful — the spec routes 405 to the AutoDetect transport's SSE fallback, which is where it now lands.

Test updates:

July2026ProtocolFallbackTests: 405 removed from the 400/404 fallback theories; added Client_On405FromProbe_DoesNotFallBackTo_Initialize (asserts Streamable HTTP surfaces the 405 directly while AutoDetect attempts SSE, and neither path attempts initialize) and Client_OnStructured405FromProbe_DoesNotFallBackTo_Initialize.
HttpClientTransportAutoDetectTests: three tests still encoded the pre-allowlist "always fall back to SSE" behavior (403/415 statuses); switched them to an allowlisted 404 so they keep exercising the dual-failure surface path.

I installed .NET SDK 10.0.101 and ran the full ModelContextProtocol.Tests suite on net10.0: 2367 passed, 0 failed (5 skipped, environment-gated). git diff --check is clean.

AI-assisted maintenance; I reviewed the changes and the test evidence.

ump45nose · 2026-09-11T02:16:10Z

Both review points are addressed at 1596ee5, and the earlier "could not run the local suite" caveat no longer applies.

Review point 1 — rebase, apply the full 400/404/405 allowlist, keep 401/403/5xx off SSE

AutoDetectingClientSessionTransport.InitializeAsync now gates the fallback through ShouldTrySseFallback, which allows only 400/404/405; 401/403/5xx throw the structured HTTP failure before any deprecated SSE GET is attempted, for both structured and unstructured bodies.

Coupled with the #1791 rebase the original conflict is gone and the PR is MERGEABLE against current main.

Review point 2 — remove 405 from the discover-probe initialize fallback

McpClientImpl.ConnectAsync keeps 400/404 only. A 405 means the POST endpoint rejected the request method, so the connection no longer retries initialize over the same transport; the spec's 405 handling stays in the AutoDetect transport's SSE fallback, and explicit Streamable HTTP surfaces the 405 to the caller.

Tests for both structured and unstructured responses

Client_OnFallbackHttpStatusFromProbe_FallsBackTo_Initialize (unstructured 400/404, both modes)
Client_OnStructuredFallbackHttpStatusFromProbe_FallsBackTo_Initialize (structured 400/404, both modes)
Client_On405FromProbe_DoesNotFallBackTo_Initialize (asserts Streamable HTTP surfaces the 405, AutoDetect attempts SSE, neither attempts initialize)
Client_OnStructured405FromProbe_DoesNotFallBackTo_Initialize
Client_OnOtherHttpErrorFromProbe_Surfaces_NoFallback (now also asserts no SSE GET for 401/403/5xx in AutoDetect mode)

Both review threads are outdated by these commits but still show as unresolved — a re-review would be appreciated.

Verification (local Release run on fork head 1596ee58, .NET SDK 10.0.101, net10.0)

dotnet build tests/ModelContextProtocol.Tests -c Release -f net10.0 → 0 warnings, 0 errors
dotnet test tests/ModelContextProtocol.Tests -c Release -f net10.0 → 2367 passed, 5 skipped, 0 failed (6m02s)
filtered July2026ProtocolFallbackTests|HttpClientTransportAutoDetectTests → 37 passed

The suite needed -p:NuGetAudit=false: restore on this branch fails NU1902 for Microsoft.Build.Tasks.Git 8.0.0, which reproduces on current main too and is unrelated to this change. No workflow runs are recorded for the fork branch, so CI has not executed on it yet.

AI assistance disclosure: AI was used to discover this opportunity and draft the change or text. The submission was checked against the prepared artifact and recorded verification evidence.

halter73 reviewed Sep 4, 2026

View reviewed changes

fix(client): [CSharp SDK/Client] 对齐 HTTP 探测回退策略

32ef18a

- 仅对 400、404、405 探测失败回退初始化握手。 - 保留 401、403、5xx 的 HTTP 语义且不发起 SSE 请求。 - 覆盖结构化与非结构化探测响应的回归场景。

ump45nose force-pushed the contrib/213636541c56 branch from 3566f99 to 32ef18a Compare September 4, 2026 07:05

scottt732 mentioned this pull request Sep 4, 2026

fix(client): skip the SSE fallback when the server/discover probe is rejected with 400/404 #1855

Open

Navigation Menu

Uh oh!

fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body - #1849

fix: fall back to SSE when AutoDetect probe gets 405 with JSON-RPC error body#1849
ump45nose wants to merge 2 commits into
modelcontextprotocol:mainfrom
ump45nose:contrib/213636541c56

ump45nose commented Sep 1, 2026

Uh oh!

halter73 left a comment

Uh oh!

halter73 Sep 4, 2026

Uh oh!

halter73 Sep 4, 2026

Uh oh!

ump45nose commented Sep 4, 2026

Uh oh!

ump45nose commented Sep 9, 2026

Uh oh!

ump45nose commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ump45nose commented Sep 1, 2026

Summary

Verification

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

halter73 Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

halter73 Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

ump45nose commented Sep 4, 2026

Uh oh!

ump45nose commented Sep 9, 2026

Uh oh!

ump45nose commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants