Implement Streamable HTTP transport #33
Conversation
Ginja
changed the title
There was a problem hiding this comment.
I think it might be easier to develop and review if updating the server and transport interfaces to support notifications - including implementing the necessary methods for STDIO was done as a first separate step/PR
Then adding another PR with the Transport implementation + examples for Streamable HTTP would be easier to do and review...
Also about the current approach - I think this should be better marked as a StandaloneHTTPTransport - since I'm not sure that could be integrated into a Rails/Sinatra/Hanami/etc app as is.....
But I think the proper road ahead for those frameworks would be maybe to offer separate mcp-rails-transport mcp-sinatra-transport etc addon gems which provide their specific Transport implementation and the respective frameworks idiomatic Plugin boilerplate
lib/mcp/server.rb
Outdated
| def handle_rack_request(request) | ||
| @transport.handle_request(request) | ||
| end |
There was a problem hiding this comment.
I think it would be better if the server interface would not contain methods that only make sense for a certain family of transports..
There was a problem hiding this comment.
I can see that POV. No strong opinions over here. I don't think it looks particularly ugly to be able to do this:
response = server.handle_rack_request(request)
However, if there's consensus I can drop this method so that the above would look like this
response = transport.handle_request(request)
Similar to how stdio uses transport.open.
Or we modify the current handle method in the server that call the right method based on the transport.
def handle(request)
case transport_type
when :stdio
JsonRpcHandler.handle(request) do |method|
handle_request(request, method)
end
when :streamablehttp
@transport.handle_request(request)
else
raise "Unknown transport type: #{transport_type}"
end
endThen everything becomes
response = server.handle(request)
Open to other ideas/suggestions as well 😄
There was a problem hiding this comment.
Server should be kept as unaware of Transport concerns as possible.
What concerns me more than the intro of handle_rack_request to server is the indirection that this introduces between Server and Transport
Server having a transport attribute and Transport having a server attribute feels like a step in the wrong direction and leads to this 🔁
ruby-sdk/examples/sse_test_server.rb
Line 67 in 78061f7
Ideally we should keep handle_request isolated to the transport so that a rack-specific transport's handle_request is effectively "handle rack request"
All of this behaviour can be removed and the example can be updated to handle requests on the transport.
There was a problem hiding this comment.
Server having a transport attribute and Transport having a server attribute feels like a step in the wrong direction and leads to this 🔁
I see that and am bothered by that too - even after proposing my interface suggestion - and I'm not sure I have a good clean alternative yet..... 🤔 (Maybe I'm overlooking an obvious alternative)
My design is based on the assumption that it would be best to add those notify_... methods (which by their event driven nature should be able to be triggered from outside the normal request/response cycle started by Transport#open) to the Server interface, since it's the one object knowing about the actual Protocol right now... and keeping that knowledge/concern inside one class seemed like a cleaner design.
But of course, one could depart from that assumption and instead provide a second class for that job - like a ServerMessageSender which has a reference to the Transport internally to do its job...
But even if we do that - I think eventually the same issue would occur again once the SDK tries to support Sampling... at which point the tool that needs sampling to do its job will need a reference to something that knows the Transport in order to send the sampling request to the client - which is the same loop again.........
|
I also thought a bit about a possible interface... and my idea so far was
def support_notifications(transport, tools_list_changed: false, resources_list_changed: false, resources_subscribe: false, prompts_list_changed: false, logging: false)which would be called by the server implementer depending on the needs/capabilities of the application
def notify_tools_list_changed
@transport.send_to_client(method: Methods::NOTIFICATIONS_TOOLS_LIST_CHANGED)
end
def notify_resources_list_changed
# ...
def notify_resource_changed(uri)
# ...
def notify_prompts_list_changed
# ...
def log_to_client(data, level:, logger: nil)
# ...Sending those notifications will be the server implementer's responsibility anyways so adding them easy to use to the server interface seems best.... I was planning to send a PoC Pull Request sooner or later when I got time... but if you like I could make one quite soon (maybe during the weekend) so it's maybe easier to talk about |
|
I like what you've proposed for the notifications. I can look to add that to my PR next week.
I prefer not to rename this. This is providing a barebone implementation of the StreamableHTTP transport. I think it's best to leave it up to implementers as to how they want to expose their MCP servers. Either directly with Rack & a web server (e.g. Puma, Unicorn, WEBrick, etc...) or contained within a web framework (e.g. Rails, Sinatra, etc...). I prefer to keep this gem free of strong opinions (e.g. no nicities provided for wrapping this in a Rails controller, or including default/expected routes).
💯 this. It should not be this gem's concern to facilitate the smooth integration into web frameworks. We can however, provide examples in the documentation (which could refer to the potential framework-specific gems). |
|
Added 3 new commits:
@kfischer-okarin I added you as a co-author, I hope you don't mind. I figured it was the right thing to do as you proposed the design ❤️ If you prefer I can drop this commit and this support can be added in a dedicated PR. One method I skipped adding was this: def support_notifications(transport, tools_list_changed: false, resources_list_changed: false, resources_subscribe: false, prompts_list_changed: false, logging: false)I wasn't completely sure what the implementation looked like. I figured this can be added later. |
|
Latest set of commits are just a rebase after some new conflicts. |
Thanks very much appreciated :)
That method is basicallt an explicit expression of the fact that notifications are optional, and since the Object doesn't need a Transport reference unless it supports notifications, I thought it would be ok if we only add it here and not in the constructor. As for the flags - those would play into determining the capabilities hash I submitted a little Capabilities related refactor as #42 - if that is adopted.... adding the respective capabilities dynamically depending on which flag is specified should be easy along the lines of @capabilities.support_tools_list_changed if tools_list_changed
#... |
Co-authored-by: Topher Bullock <[email protected]>
Co-authored-by: Kevin Fischer <[email protected]>
There was a problem hiding this comment.
Sorry for not reviewing and commenting on this earlier - I was mainly concerned with the Server class and Transport changes and did not look at tests or the examples.
But in my opinion the both tests and examples still have some problems which I would prefer not to be merged (but of course I can understand if the main maintainers want to move forward and fix these issues later - so feel free to dismiss as just one contributors opinion)
I gave a few representative examples of the issues I personally see
No offense intended, but my general impression while reading was that examples and tests have maybe been generated by an coding agent (which I totally support) but not deeply reviewed
(this again is of course something which the main maintainer team might have a different opinion on - i.e. it's fine to merge code like that because with Coding Agents it's also as easy to change it again later... I personally don't think that is a good approach but of course respect the maintainers direction)
| #### Notification Format | ||
|
|
||
| Notifications follow the JSON-RPC 2.0 specification and use these method names: |
There was a problem hiding this comment.
This section seems unnecessarily specific and redundant about JSON-RPC - this is mentioned in the main section already
| puts "The server will return a session ID in the Mcp-Session-Id header." | ||
| puts "Use this session ID for subsequent requests." | ||
| puts "" |
There was a problem hiding this comment.
This mentions the Mcp-Session-Id header and it is somewhat (I think not correctly though) supported in the client - but the server does not actually create or emit any such headers...
There was a problem hiding this comment.
but the server does not actually create or emit any such headers...
Unless I'm misunderstanding, I believe it does?
$ curl -D - -s -o /dev/null -X POST http://localhost:9292/mcp \
-H "Content-Type: application/json" \
-d '{"jsonrpc":"2.0","method":"initialize","id":1,"params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"curl-test","version":"1.0"}}}'
HTTP/1.1 200 OK
Content-Type: application/json
Mcp-Session-Id: e286fd80-59b8-4281-9bdd-39e88d4d2f26
Content-Length: 184
There was a problem hiding this comment.
I'm sorry 🙈 it seems I missed checking the Transport Implementation itself - I only looked in the respective example servers. Yes it's implemented there 👍🏻
|
Also as an additional note: I'm half-way finished working on a little minimal version containing a subset of this branch that implements the general list changed notifications support for STDIO - (of course properly attributing @Ginja as Co-Author everywhere where I used any code from this PR) - since this PR seems to get quite big and if the discussion/review goes on it might still take a while until any value is provided to the main branch... Unless you just want to go on with this branch as is which is also fine of course ;) |
| class Transport | ||
| # Initialize the transport with the server instance | ||
| def initialize(server) | ||
| @server = server | ||
| end | ||
|
|
||
| # Send a response to the client | ||
| def send_response(response) | ||
| raise NotImplementedError, "Subclasses must implement send_response" | ||
| end | ||
|
|
||
| # Open the transport connection | ||
| def open | ||
| raise NotImplementedError, "Subclasses must implement open" | ||
| end | ||
|
|
||
| # Close the transport connection | ||
| def close | ||
| raise NotImplementedError, "Subclasses must implement close" | ||
| end | ||
|
|
||
| private | ||
| # Handle a JSON request | ||
| # Returns a response that should be sent back to the client | ||
| def handle_json_request(request) | ||
| response = @server.handle_json(request) | ||
| send_response(response) if response | ||
| end | ||
|
|
||
| # Handle an incoming request | ||
| # Returns a response that should be sent back to the client | ||
| def handle_request(request) | ||
| response = @server.handle(request) | ||
| send_response(response) if response | ||
| end | ||
|
|
||
| def handle_json_request(request) | ||
| response = @server.handle_json(request) | ||
| send_response(response) if response | ||
| # Send a notification to the client | ||
| # Returns true if the notification was sent successfully | ||
| def send_notification(method, params = nil) | ||
| raise NotImplementedError, "Subclasses must implement send_notification" | ||
| end | ||
| end |
There was a problem hiding this comment.
This is a rather fuzzy big picture design feedback - but I was wondering what to include in the Transport interface and what not...
The main goal of the Transport interface from my point of view is hiding the details of the transport method from the client code.
Clients of the Transport object so far
The main client until now was the main application entrypoint and the only really essential public interface methods were open and close. Everything else can be easily handled inside the loop created by open - so technically there is no need for any of the other methods to be public.
Now with the current addition of the notify methods the server becomes an additional client. And the only thing the server needs to know is how to send a notification via the transport. So send_notification is another must-have addition to the interface.
Issues surfaced through adding the Streamable HTTP implementation
What about the Rack Setup?
If we wanted to keep the pure simple open interface - it would be necessary to somehow include all the necessary Rack Setup in the current Transport class.
But considering the wider point of view that the natural Rack interface would be just exposing a callable taking a rack env object - maybe the open interface isn't the best way to handle things after all?
Make the Transport interface super simple?
So maybe removing open (and accordingly close I guess) from the interface and just have each Transport document their own preferred way of starting itself might be another valid way to do things - so the only method included in the public interface is really send_notificaton
But even in that case I'd still personally say that it would be nicer to make the Streamable HTTP a bit more out-of-the back Rack startable than it is now... which probably just means:
- exposing a way to add middlewares
- adding an option for the endpoint name (/mcp by default)
- ...?
There was a problem hiding this comment.
make the Streamable HTTP a bit more out-of-the back Rack startable than it is now
yeah, I think the next direction after this merges should be to allow base StreamableHTTP transport to be easily dropped into any Rack, Rails, etc. app - build out conventions that make adding to any existing server trivial.
this gives us a base we can work from and a reference implementation, so I'm keen to get this merged and iterate forward
There was a problem hiding this comment.
@kfischer-okarin let's continue the drop-in rack chat here #59.
I've made a separate one for Rails #60
There was a problem hiding this comment.
thanks for the clean up @Ginja. This looks good to merge.
gives us a reference for future improvements that make adding http transport to any rack/rails app trivial.
we can fix forward with any tweaks / extensions
Implements a rack-compatible Streamable HTTP transport with SSE support. As well as add notification support for the server with the following methods:
notify_tools_list_changed()- Send a notification when the tools list changesnotify_prompts_list_changed()- Send a notification when the prompts list changesnotify_resources_list_changed()- Send a notification when the resources list changesCloses #4
Motivation and Context
To fulfill the Streamable HTTP transport specification as described here.
How Has This Been Tested?
Local tests only (see
examples/). Working on implementing it with my MCP server that's a WIP.Breaking Changes
Should introduce no breaking changes as using the StreamableHTTP transport is optional
Types of changes
Checklist
Local Tests