Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Kludex
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/server/mcpserver/server.py">
<violation number="1" location="src/mcp/server/mcpserver/server.py:1198">
P2: `connect_sse()` already sends the SSE response, so returning `Response()` makes `request_response` send a second response when a GET stream closes. Keep the GET handler as a raw ASGI endpoint and return a `Response` only for HEAD.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| streams[0], streams[1], self._lowlevel_server.create_initialization_options() | ||
| ) | ||
| return Response() | ||
| return Response() # pragma: no cover |
There was a problem hiding this comment.
P2: connect_sse() already sends the SSE response, so returning Response() makes request_response send a second response when a GET stream closes. Keep the GET handler as a raw ASGI endpoint and return a Response only for HEAD.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/mcp/server/mcpserver/server.py, line 1198:
<comment>`connect_sse()` already sends the SSE response, so returning `Response()` makes `request_response` send a second response when a GET stream closes. Keep the GET handler as a raw ASGI endpoint and return a `Response` only for HEAD.</comment>
<file context>
@@ -1178,21 +1177,25 @@ def sse_app(
await self._lowlevel_server.run(
streams[0], streams[1], self._lowlevel_server.create_initialization_options()
)
+ return Response() # pragma: no cover
# Create routes
</file context>
Fixes #3736
MCPServer.sse_app()now answersHEADon the SSE endpoint at once with200and the headers aGETgets (Content-Type: text/event-stream,Cache-Control: no-store,X-Accel-Buffering: no), without a body and without opening a session or a stream.Motivation and Context
Starlette adds
HEADto everyGETroute, soHEAD /sseran the full SSE handler:connect_sse()opened a session andEventSourceResponsestarted a stream that never ends. The server sent the headers, discarded the body, and never completed the response. A keep-alive client or reverse proxy (nginx returns a HEAD's upstream connection to its pool once the headers arrive) then hung on its next request, and the handler, its session and the socket stayed alive. LibreChat sendsHEADto every MCP server at startup, which is how this showed up in production.SseServerTransportgains a publichead_response(request), which runs the transport's own Host/Origin validation (so DNS-rebinding protection still refuses a disallowedHEADwith 421/403) and otherwise returns the headers aGETgets, with no body. Both routes insse_app()go through onesse_endpointthat returns it beforeconnect_sse(). With auth configured the route isRequireAuthMiddleware(request_response(sse_endpoint)), so authentication runs first and an unauthenticatedHEADkeeps getting its 401 (what OAuth-detection probes look for). The module docstring example, which downstream servers copy, calls the same method, so servers built directly on the transport get the fix and the validation without duplicating either.This follows RFC 9110: servers must support
HEADwherever they supportGET(§9.1), and should send the same header fields without content (§9.3.2). Starlette's automaticContent-Length: 0is dropped, since aHEADresponse must not carry one that differs from what theGETwould send (§8.6), and an SSE stream has no length.How Has This Been Tested?
test_head_on_the_sse_endpoint_is_answered_with_headers_onlydrives the realMCPServer.sse_app()in process and snapshots the status, full header set and empty body. Without the fix it times out, reproducing the hang.test_head_on_the_sse_endpoint_is_subject_to_dns_rebinding_protectionsendsHEADwith a disallowed Host and expects 421.transport:sse:headrequirement (issue:#3736).strict-no-coverand pyright clean.Breaking Changes
HEAD /ssereturns its headers instead of hanging.GET /sseis unchanged.SseServerTransport.head_response()is a new public method.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
The GET path of
sse_app()stays excluded from coverage, as before, and the auth branch is not exercised by a test. Going throughrequest_responsemeans the auth route now also sends the trailingResponse()after a GET stream completes, as the no-auth route always has; it only shows when the stream ends while the client is still connected.The generic part (
EventSourceResponsestreaming forever onHEAD) is tracked upstream in sysid/sse-starlette#218.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.