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.

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 2 commits into from
May 12, 2025

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)

@tim-watcha
Copy link
Contributor

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

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!


@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 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. 😄

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.

4 participants