-
-
Notifications
You must be signed in to change notification settings - Fork 103
Update SSE implementation for client-specific messaging #83
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?
Conversation
…t will conflict a little here)
| # Handle reconnection | ||
| if client_id && @sse_clients.key?(client_id) | ||
| handle_client_reconnection(client_id, browser_type) | ||
| else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this behavior from extract_client_id into the register_sse_client so that we can use extract_client_id on the /sse/messages path of request handling (to determine which client is requesting).
The logic fit succinctly within register_sse_client so I removed handle_client_reconnection from being its own method.
lib/mcp/transports/rack_transport.rb
Outdated
| params = [] | ||
| params << query_string if query_string && !query_string.empty? | ||
| params << "client_id=#{client_id}" | ||
| endpoint += "?#{params.join('&')}" unless params.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is primary change of this pr, where all other changes are consuming its changes.
- The endpoint returned to the client now has a client_id in the query param. The client is supposed to respect the endpoint we give it.
- Now when the client requests in through
/sse/messageswe can extract the client_id from the request parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brilliant 💟
|
Tests are passing and testing locally appears stable for me but given there isn't a strict client id protocol for the 2024-11-05 spec (just guidance that clients respect the endpoint) I think this would warrant some additional manual verification. Client reconnections are something I'm not too well versed in with MCP, I was using |
| end | ||
| end.to_h | ||
|
|
||
| headers['client_id'] = extract_client_id(request.env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily a http header, but after we extract http headers I chose to extract the client ID and place it in the header data as well.
yjacquin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, thanks !
I'm releasing 1.5.0 today with lots of code changes.
This could be part of the next release if you want :)
We need to solve the merge conflicts and we're good to go !
Just to keep in mind, the client_id behavior is now natively supported in the new revision with the StreamableHTTP transport (called session id in the other revision) but until then, your solution sounds perfect 👌
lib/mcp/transports/rack_transport.rb
Outdated
| params = [] | ||
| params << query_string if query_string && !query_string.empty? | ||
| params << "client_id=#{client_id}" | ||
| endpoint += "?#{params.join('&')}" unless params.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
brilliant 💟
@yjacquin, I'm happy to resolve the conflicts and get this cleaned up on top of 1.5. Super excited to see it StreamableHttp coming out! I have some spare time this week I'll get this cleaned up. |
Co-authored-by: Yorick Jacquin <yorickjacquin@gmail.com>
…client-specific-messaging
|
@yjacquin this is ready for another set of eyes. One thought I avoided putting in place just to not overdo the scope of the pr: The volume of calling methods with Not sure with your work on Streamable HTTP if you were seeing the same code pattern pop up, figured I'd note it for a code review :). |
|
Hello! When using the Python MCP SSE client I currently see some validation errors happening that I think this PR will fix. Stack trace at the bottom, but in summary, the Python client performs schema validation and this project is sending This is also happening currently when we send Tangentially, there's also two more validation issues here:
It might be a good idea to generate a series of Ruby DTO objects from the Trace follows: |
Solves #40.
Adds IO messaging per client id for SSE implementation.
Changes:
Notes:
I couldn't find a specific impl for 2024-11-05 spec on SSE client session tracking. Rather it calls out that the client should respect the endpoint provided by the server.
client_idin the endpoint for communication.