-
Notifications
You must be signed in to change notification settings - Fork 767
Fix HTTP fallback error handling #1791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -80,19 +80,19 @@ public override async Task SendMessageAsync(JsonRpcMessage message, Cancellation | |
| // for robustness. Servers occasionally emit them with 4xx codes other than 400. | ||
| if (!response.IsSuccessStatusCode && | ||
| await TryReadJsonRpcErrorAsync(response, cancellationToken).ConfigureAwait(false) is { } parsedError && | ||
| (response.StatusCode == HttpStatusCode.BadRequest || | ||
| IsPerRequestMetadataProtocolErrorCode((McpErrorCode)parsedError.Error.Code))) | ||
| ShouldSurfaceJsonRpcErrorAsProtocolException(response.StatusCode, parsedError)) | ||
| { | ||
| throw McpSessionHandler.CreateRemoteProtocolExceptionFromError(parsedError); | ||
| } | ||
|
|
||
| await response.EnsureSuccessStatusCodeWithResponseBodyAsync(cancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private static bool IsPerRequestMetadataProtocolErrorCode(McpErrorCode code) => | ||
| code is McpErrorCode.UnsupportedProtocolVersion | ||
| or McpErrorCode.MissingRequiredClientCapability | ||
| or McpErrorCode.HeaderMismatch; | ||
| internal static bool ShouldSurfaceJsonRpcErrorAsProtocolException(HttpStatusCode statusCode, JsonRpcError error) => | ||
| statusCode == HttpStatusCode.BadRequest || | ||
| (McpErrorCode)error.Error.Code is McpErrorCode.UnsupportedProtocolVersion | ||
| or McpErrorCode.MissingRequiredClientCapability | ||
| or McpErrorCode.HeaderMismatch; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this isn't new in this PR, but where did we get this list of three error codes? Looking at the "Backward Compatibility with Initialization-Based Versions" section in modelcontextprotocol/modelcontextprotocol#2844, I only see I think this means that I see modelcontextprotocol/modelcontextprotocol#2575 mentions Can we avoid classifying responses based on the JSON-RPC error code here? A non-successful HTTP response containing any JSON-RPC error should become an |
||
|
|
||
| /// <summary> | ||
| /// Reads a JSON-RPC error envelope from an <c>application/json</c> response body, returning | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,15 +57,22 @@ public async ValueTask DisposeAsync() | |
| base.Dispose(); | ||
| } | ||
|
|
||
| private async Task StartServerAsync(RequestDelegate handler) | ||
| private async Task StartServerAsync(RequestDelegate handler, bool acceptGet = false) | ||
| { | ||
| Builder.Services.Configure<JsonOptions>(options => | ||
| { | ||
| options.SerializerOptions.TypeInfoResolverChain.Add(McpJsonUtilities.DefaultOptions.TypeInfoResolver!); | ||
| }); | ||
|
|
||
| _app = Builder.Build(); | ||
| _app.MapPost("/mcp", handler); | ||
| if (acceptGet) | ||
| { | ||
| _app.MapMethods("/mcp", [HttpMethods.Get, HttpMethods.Post], handler); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Could we just do this for all the tests calling |
||
| } | ||
| else | ||
| { | ||
| _app.MapPost("/mcp", handler); | ||
| } | ||
| await _app.StartAsync(TestContext.Current.CancellationToken); | ||
| } | ||
|
|
||
|
|
@@ -302,6 +309,63 @@ await WriteJsonRpcErrorAsync(context, HttpStatusCode.BadRequest, | |
| Assert.False(initializeReceived); | ||
| } | ||
|
|
||
| [Theory] | ||
| [InlineData(HttpStatusCode.Unauthorized)] | ||
| [InlineData(HttpStatusCode.Forbidden)] | ||
| [InlineData(HttpStatusCode.InternalServerError)] | ||
| public async Task AutoDetect_OnNonModernJsonRpcErrorOutside400_PreservesHttpFailure_NoFallback(HttpStatusCode statusCode) | ||
| { | ||
| var ct = TestContext.Current.CancellationToken; | ||
| var initializeRequests = 0; | ||
| var sseGetRequests = 0; | ||
|
|
||
| await StartServerAsync(async context => | ||
| { | ||
| if (HttpMethods.IsGet(context.Request.Method)) | ||
| { | ||
| sseGetRequests++; | ||
| context.Response.StatusCode = StatusCodes.Status405MethodNotAllowed; | ||
| return; | ||
| } | ||
|
|
||
| var message = await JsonSerializer.DeserializeAsync( | ||
| context.Request.Body, | ||
| GetJsonTypeInfo<JsonRpcMessage>(), | ||
| ct); | ||
|
|
||
| if (message is JsonRpcRequest { Method: RequestMethods.Initialize }) | ||
| { | ||
| initializeRequests++; | ||
| } | ||
|
|
||
| await WriteJsonRpcErrorAsync( | ||
| context, | ||
| statusCode, | ||
| code: (int)McpErrorCode.InvalidRequest, | ||
| message: "non-modern structured error"); | ||
| }, acceptGet: true); | ||
|
|
||
| await using var transport = new HttpClientTransport(new() | ||
| { | ||
| Endpoint = new("http://localhost:5000/mcp"), | ||
| TransportMode = HttpTransportMode.AutoDetect, | ||
| }, HttpClient, LoggerFactory); | ||
|
|
||
| var exception = await Assert.ThrowsAsync<HttpRequestException>(async () => | ||
| { | ||
| await using var client = await McpClient.CreateAsync( | ||
| transport, | ||
| new McpClientOptions(), | ||
| loggerFactory: LoggerFactory, | ||
| cancellationToken: ct); | ||
| }); | ||
|
|
||
| Assert.Equal(statusCode, exception.StatusCode); | ||
| Assert.Contains("non-modern structured error", exception.Message); | ||
| Assert.Equal(0, initializeRequests); | ||
| Assert.Equal(0, sseGetRequests); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task Client_OnPerRequestMetadataResponseWithMcpSessionId_IgnoresSessionState() | ||
| { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.