Skip to content

fix: Fix handle_sse method to match Starlette route endpoint expectations #156

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

Merged
merged 5 commits into from
May 29, 2025

Conversation

tofarr
Copy link
Contributor

@tofarr tofarr commented May 28, 2025

Description

This PR fixes the handle_sse method in the router.py file to match the format expected by Starlette for route endpoints.

Problem

The current implementation of handle_sse has a signature that does not match what Starlette expects. Starlette route endpoints should either be async functions that take a Request object and return a Response, or be callables that take scope, receive, and send parameters.

Solution

Updated the handle_sse method to return an ASGI application function that takes scope, receive, and send parameters, making it compatible with Starlettes routing system.

Testing

All tests are passing, confirming that the changes are working correctly.

@Copilot Copilot AI review requested due to automatic review settings May 28, 2025 04:16
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the handle_sse method to return an ASGI application function, aligning it with Starlette’s route endpoint requirements.

  • Changed handle_sse to wrap SSE logic in a nested ASGI app callable
  • Adjusted parameters passed to sse.connect_sse to use the ASGI scope, receive, and send
  • Removed the return type annotation and now return the nested app function
Comments suppressed due to low confidence (1)

src/mcpm/router/router.py:618

  • [nitpick] The nested function name app is quite generic; renaming it to something more descriptive (e.g., sse_asgi_app) could improve readability.
async def app(scope: Scope, receive: Receive, send: Send) -> None:

Copy link
Contributor

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you ❤️ ! But I would prefer to follow the official example which returns an empty response

https://github.com/modelcontextprotocol/python-sdk/blob/main/src/mcp/server/sse.py#L26

@JoJoJoJoJoJoJo JoJoJoJoJoJoJo changed the title Fix handle_sse method to match Starlette route endpoint expectations fix: Fix handle_sse method to match Starlette route endpoint expectations May 28, 2025
@JoJoJoJoJoJoJo JoJoJoJoJoJoJo merged commit 7d13fc1 into pathintegral-institute:main May 29, 2025
3 checks passed
mcpm-semantic-release bot pushed a commit that referenced this pull request May 29, 2025
## [1.13.1](v1.13.0...v1.13.1) (2025-05-29)

### Bug Fixes

*  Fix handle_sse method to match Starlette route endpoint expectations ([#156](#156)) ([7d13fc1](7d13fc1))
@mcpm-semantic-release
Copy link

🎉 This PR is included in version 1.13.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants