diff --git a/docs/mcpgodebug.md b/docs/mcpgodebug.md index 45b9c7d9..4562821b 100644 --- a/docs/mcpgodebug.md +++ b/docs/mcpgodebug.md @@ -46,9 +46,12 @@ Options listed below were added and will be removed in the 1.9.0 version of the requested method in STDIO transport is not found. - `noprotocolerrorbody` added. If set to `1`, the streamable HTTP client will - not attempt to decode the JSON-RPC error body of a non-2xx HTTP response, - restoring the previous behavior. The default behavior was changed so that - the client always attempts to surface the underlying JSON-RPC error. + not attempt to decode the JSON-RPC error body of a non-2xx HTTP response, + and any non-transient error will permanently fail the connection, restoring + the previous behavior. The default behavior was changed so that the client + attempts to decode the JSON-RPC error body of a non-2xx response, surfaces + the underlying JSON-RPC error, and wraps it with `jsonrpc2.ErrRejected` so + that per-call rejections do not tear down the session. - `nowrapinvalidparams` added. If set to `1`, the server will not wrap params-decoding failures with `jsonrpc2.ErrInvalidParams`, so wire responses diff --git a/internal/docs/mcpgodebug.src.md b/internal/docs/mcpgodebug.src.md index e3ca770c..c009a7c5 100644 --- a/internal/docs/mcpgodebug.src.md +++ b/internal/docs/mcpgodebug.src.md @@ -45,9 +45,12 @@ Options listed below were added and will be removed in the 1.9.0 version of the requested method in STDIO transport is not found. - `noprotocolerrorbody` added. If set to `1`, the streamable HTTP client will - not attempt to decode the JSON-RPC error body of a non-2xx HTTP response, - restoring the previous behavior. The default behavior was changed so that - the client always attempts to surface the underlying JSON-RPC error. + not attempt to decode the JSON-RPC error body of a non-2xx HTTP response, + and any non-transient error will permanently fail the connection, restoring + the previous behavior. The default behavior was changed so that the client + attempts to decode the JSON-RPC error body of a non-2xx response, surfaces + the underlying JSON-RPC error, and wraps it with `jsonrpc2.ErrRejected` so + that per-call rejections do not tear down the session. - `nowrapinvalidparams` added. If set to `1`, the server will not wrap params-decoding failures with `jsonrpc2.ErrInvalidParams`, so wire responses diff --git a/mcp/streamable.go b/mcp/streamable.go index f642db1d..2799d146 100644 --- a/mcp/streamable.go +++ b/mcp/streamable.go @@ -300,7 +300,12 @@ var allowsessionsinstateless = mcpgodebug.Value("allowsessionsinstateless") // noprotocolerrorbody is a compatibility parameter that restores the previous // behavior of [streamableClientConn.checkResponse]. When unset (the default), -// the client always attempts to surface the underlying JSON-RPC error. +// the client attempts to decode the response body of a non-2xx response as a +// JSON-RPC error. If successful, the underlying JSON-RPC error is surfaced to +// the caller and wrapped with [jsonrpc2.ErrRejected] so that per-call +// rejections do not tear down the session. When set to "1", the client +// ignores response bodies on non-2xx responses and any non-transient error +// permanently fails the connection. var noprotocolerrorbody = mcpgodebug.Value("noprotocolerrorbody") // disablecontenttypecheck is a compatibility parameter that allows to disable @@ -2304,10 +2309,13 @@ func (c *streamableClientConn) Write(ctx context.Context, msg jsonrpc.Message) e } if err := c.checkResponse(ctx, requestSummary, resp); err != nil { - if requestMethod == methodDiscover { + if requestMethod == methodDiscover && !errors.Is(err, jsonrpc2.ErrRejected) { // Wrap the discover failure with ErrRejected so the jsonrpc2 layer // doesn't set writeErr, which would prevent the legacy initialize - // fallback from succeeding on the same connection. + // fallback from succeeding on the same connection. This covers the + // case where a legacy server rejects server/discover with a + // non-JSON-RPC body (e.g. plain text 400), which checkResponse + // cannot classify as a per-call rejection on its own. err = fmt.Errorf("%w: %w", err, jsonrpc2.ErrRejected) } else if !errors.Is(err, jsonrpc2.ErrRejected) { // Only fail the connection for non-transient errors. @@ -2528,31 +2536,30 @@ func (c *streamableClientConn) checkResponse(ctx context.Context, requestSummary resp.Body.Close() } }() - // §2.5.3: "The server MAY terminate the session at any time, after - // which it MUST respond to requests containing that session ID with HTTP - // 404 Not Found." - if resp.StatusCode == http.StatusNotFound { - // Return an ErrSessionMissing to avoid sending a redundant DELETE when the - // session is already gone. - return fmt.Errorf("%s: failed to connect (session ID: %v): %w", requestSummary, c.sessionID, ErrSessionMissing) - } // Transient server errors (502, 503, 504, 429) should not break the connection. // Wrap them with ErrRejected so the jsonrpc2 layer doesn't set writeErr. if isTransientHTTPStatus(resp.StatusCode) { return fmt.Errorf("%w: %s: %v", jsonrpc2.ErrRejected, requestSummary, http.StatusText(resp.StatusCode)) } - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - // By default, always try to decode the body and surface the underlying - // JSON-RPC error. - // Setting MCPGODEBUG=noprotocolerrorbody=1 restores the previous behavior. - if noprotocolerrorbody == "1" { - return fmt.Errorf("%s: %v", requestSummary, http.StatusText(resp.StatusCode)) - } + // By default, always try to decode the body and surface the underlying + // JSON-RPC error, wrapping it with ErrRejected to prevent the connection from closing. + // Setting MCPGODEBUG=noprotocolerrorbody=1 restores the previous behavior. + if noprotocolerrorbody != "1" && (resp.StatusCode < 200 || resp.StatusCode >= 300) { body, _ := io.ReadAll(resp.Body) msg, _ := jsonrpc.DecodeMessage(body) if response, ok := msg.(*jsonrpc.Response); ok && response.Error != nil { - return fmt.Errorf("%s: %w: %v", requestSummary, response.Error, http.StatusText(resp.StatusCode)) + return fmt.Errorf("%s: %w: %w: %v", requestSummary, response.Error, jsonrpc2.ErrRejected, http.StatusText(resp.StatusCode)) } + } + // §2.5.3: "The server MAY terminate the session at any time, after + // which it MUST respond to requests containing that session ID with HTTP + // 404 Not Found." + if resp.StatusCode == http.StatusNotFound { + // Return an ErrSessionMissing to avoid sending a redundant DELETE when the + // session is already gone. + return fmt.Errorf("%s: failed to connect (session ID: %v): %w", requestSummary, c.sessionID, ErrSessionMissing) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { return fmt.Errorf("%s: %v", requestSummary, http.StatusText(resp.StatusCode)) } return nil diff --git a/mcp/streamable_client_test.go b/mcp/streamable_client_test.go index c8ab0669..2b51bc75 100644 --- a/mcp/streamable_client_test.go +++ b/mcp/streamable_client_test.go @@ -1751,3 +1751,147 @@ func TestStreamableClientConnect_DiscoverUnsupportedVersionNegotiation(t *testin t.Errorf("InitializeResult.ProtocolVersion = %q, want %q", got, protocolVersion20260728) } } + +// TestStreamableClientHandlerErrorPropagation verifies that per-call +// handler-level HTTP errors carrying a JSON-RPC error body do not tear down +// the session, and that setting MCPGODEBUG=noprotocolerrorbody=1 restores the +// pre-fix behavior in which non-transient errors permanently failed the +// connection. +func TestStreamableClientHandlerErrorPropagation(t *testing.T) { + ctx := context.Background() + + // Build a JSON-RPC error response body for a given tools/call request. + jsonRPCErrorBody := func(id int64, code int64, msg string) string { + return jsonBody(t, resp(id, nil, &jsonrpc.Error{Code: code, Message: msg})) + } + + tests := []struct { + name string + callStatus int // HTTP status returned for the tools/call + callBody string // response body for the tools/call ("" for empty; may be a JSON-RPC error) + disableBodyDecode bool // if true, sets noprotocolerrorbody="1" for the duration of the test + wantErrRejected bool // whether the returned error should match errors.Is(err, jsonrpc2.ErrRejected) + wantSessionAlive bool // whether a subsequent call on the same session should succeed + }{ + { + // SEP-2575 §"Missing Required Capabilities": server returns + // -32021 at HTTP 400. Session must survive. + name: "400 with JSON-RPC error body (default)", + callStatus: http.StatusBadRequest, + callBody: jsonRPCErrorBody(1, CodeMissingRequiredClientCapabilities, "missing capability"), + wantErrRejected: true, + wantSessionAlive: true, + }, + { + // Same server response, but the user opted out via + // MCPGODEBUG=noprotocolerrorbody=1. Pre-fix behavior: the + // session is torn down. + name: "400 with JSON-RPC error body (noprotocolerrorbody=1)", + callStatus: http.StatusBadRequest, + callBody: jsonRPCErrorBody(1, CodeMissingRequiredClientCapabilities, "missing capability"), + disableBodyDecode: true, + wantErrRejected: false, + wantSessionAlive: false, + }, + { + // Legacy server returns plain-text 400 with no JSON-RPC + // body: cannot be classified as a per-call rejection, so the + // session is still torn down. + name: "400 with plain-text body", + callStatus: http.StatusBadRequest, + callBody: "Bad Request", + wantErrRejected: false, + wantSessionAlive: false, + }, + { + // SEP-2575: server returns -32601 for an unimplemented + // method at HTTP 404. Must be treated as a per-call + // rejection, not a terminated session. + name: "404 with JSON-RPC MethodNotFound body", + callStatus: http.StatusNotFound, + callBody: jsonRPCErrorBody(1, jsonrpc.CodeMethodNotFound, `method not found: "tools/call"`), + wantErrRejected: true, + wantSessionAlive: true, + }, + { + // A bare 404 with no JSON-RPC body means the session has + // been terminated. Session must not survive. + name: "404 with empty body (terminated session)", + callStatus: http.StatusNotFound, + callBody: "", + wantErrRejected: false, + wantSessionAlive: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if test.disableBodyDecode { + prev := noprotocolerrorbody + noprotocolerrorbody = "1" + t.Cleanup(func() { noprotocolerrorbody = prev }) + } + + // Track how many tools/call requests we've served: first + // returns the error under test, subsequent calls succeed so + // we can observe whether the session is still alive. + var callsServed atomic.Int32 + fake := &fakeStreamableServer{ + t: t, + responses: fakeResponses{ + {"POST", "", methodDiscover, ""}: { + header: header{ + "Content-Type": "application/json", + }, + wantProtocolVersion: protocolVersion20260728, + responseFunc: func(r *jsonrpc.Request) (string, int) { + return jsonBody(t, resp(r.ID.Raw().(int64), discoverResult, nil)), http.StatusOK + }, + }, + {"POST", "", "tools/call", ""}: { + header: header{"Content-Type": "application/json"}, + responseFunc: func(r *jsonrpc.Request) (string, int) { + if callsServed.Add(1) == 1 { + return test.callBody, test.callStatus + } + return jsonBody(t, resp(r.ID.Raw().(int64), &CallToolResult{}, nil)), 0 + }, + optional: true, + }, + }, + } + + httpServer := httptest.NewServer(fake) + defer httpServer.Close() + + transport := &StreamableClientTransport{Endpoint: httpServer.URL} + client := NewClient(testImpl, nil) + session, err := client.Connect(ctx, transport, &ClientSessionOptions{protocolVersion: protocolVersion20260728}) + if err != nil { + t.Fatalf("Connect: %v", err) + } + defer session.Close() + + // First call: triggers the error under test. + _, err = session.CallTool(ctx, &CallToolParams{Name: "nonexistent"}) + if err == nil { + t.Fatal("first CallTool succeeded unexpectedly, want error") + } + if got := errors.Is(err, jsonrpc2.ErrRejected); got != test.wantErrRejected { + t.Errorf("errors.Is(err, ErrRejected) = %v, want %v (err = %v)", got, test.wantErrRejected, err) + } + + // Second call: verifies whether the session survived. + _, err = session.CallTool(ctx, &CallToolParams{Name: "nonexistent"}) + if test.wantSessionAlive { + if err != nil { + t.Errorf("second CallTool failed: %v (session should survive per-call rejection)", err) + } + } else { + if err == nil { + t.Error("second CallTool succeeded unexpectedly, want session torn down") + } + } + }) + } +}