8000 Properly infer prefix for SSE messages by jlowin · Pull Request #659 · modelcontextprotocol/python-sdk · GitHub
[go: up one dir, main page]
Skip to content

Properly infer prefix for SSE messages #659

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 12, 2025
Merged

Properly infer prefix for SSE messages #659

merged 2 commits into from
May 12, 2025
+87 −3

Conversation

jlowin
Copy link
Contributor
@jlowin jlowin commented May 8, 2025

I've been following the issue in #386 and see the solution merged in #540. However, it feels very wrong. An app should never have to know its mount path in advance in order to work properly. Furthermore, this results in a cumbersome DX where either users specify the mount paths when creating the FastMCP server, effectively preventing it from being used in any other context or users double-specify mount paths when mounting the SSE app into another Starlette application, which is confusing and awkward.

With that in mind, consider this example from the docs of #540. The GitHub and browser apps will simply break if they are mounted under any other path. This is not a good user-facing outcome.

# Create Starlette app with multiple mounted servers
app = Starlette(
    routes=[
        # Using settings-based configuration
        Mount("/github", app=github_mcp.sse_app()),
        Mount("/browser", app=browser_mcp.sse_app()),
        # Using direct mount path parameter
        Mount("/curl", app=curl_mcp.sse_app("/curl")),
        Mount("/search", app=search_mcp.sse_app("/search")),
    ]
)

Happily I don't think we need to settle, or pollute the FastMCP server with new user-facing configuration. The ASGI spec provides for pretty much this exact circumstance with a root_path attribute on the Scope object. By examining the root path when constructing the messages POST endpoint, we can ensure that it is always generated at the correct absolute path, without having to perform any complex parsing or even asking users to provide additional configuration.

With this PR, the following works out of the box:

import uvicorn
from starlette.applications import Starlette
from starlette.routing import Mount

from mcp.server.fastmcp import FastMCP

mcp = FastMCP()


@mcp.tool()
async def hello() -> str:
    return "Hello, world!"


sse_app = mcp.sse_app()
main_app = Starlette(routes=[Mount("/nested/mount/", app=sse_app)])

if __name__ == "__main__":
    uvicorn.run(main_app, host="127.0.0.1", port=8000)

This was referenced May 8, 2025
Closed
Merged
@tim-watcha
Copy link
Contributor

@ihrpr It seems this approach is better than my approach #540 how do you think

@LiangYang666 LiangYang666 mentioned this pull request May 12, 2025
Closed
ihrpr
ihrpr approved these changes May 12, 2025
View reviewed changes
Copy link
Contributor
@ihrpr ihrpr 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!

tests/shared/test_sse.py

@pytest.fixture()
def mounted_server(server_port: int) -> Generator[None, None, None]:
proc = multiprocessing.Process(
Copy link
Contributor

Choose a reason for hiding this comment

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

note for the future, need to extract it into a function...

@ihrpr ihrpr merged commit 1cb7407 into modelcontextprotocol:main May 12, 2025
6 checks passed
@jlowin jlowin mentioned this pull request May 12, 2025
Merged
This was referenced May 13, 2025
Closed
Closed
@jlowin jlowin mentioned this pull request May 13, 2025
Merged
9 tasks
@jlowin jlowin deleted the sse-mount-prefix branch May 13, 2025 22:51
@dwayn
Copy link
dwayn commented May 15, 2025

Thank you for doing this! This is exactly the fix that was needed. 😄

@abidlabs abidlabs mentioned this pull request May 20, 2025
Merged
@ihrpr ihrpr mentioned this pull request May 23, 2025
Closed
4 tasks
@mindcrime
Copy link

Does this fix also work for the Vanilla Python SDK case, where one is using SSEServerTransport directly (eg, no FastMCP)? If so, could someone be kind enough to share an example of how to configure things in that case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihrpr ihrpr ihrpr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jlowin @tim-watcha @dwayn @mindcrime @ihrpr
0