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. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| endpoint=RequireAuthMiddleware( | ||
| request_response(sse_endpoint), required_scopes, resource_metadata_url | ||
| ), |
There was a problem hiding this comment.
🟡 (optional) Auth-enabled SSE servers now send a second response after each GET stream ends, which the base branch never did on that route. request_response(sse_endpoint) at server.py:1245 sends the trailing Response() from server.py:1186 on the same send the SSE stream used. An ASGI server that still accepts messages then raises on the extra http.response.start, for example when shutdown ends the stream. Fix: once a stream has run, send nothing more on either route (for example return a response whose call is a no-op), while HEAD still gets its 405. The no-auth route already does this (pre-existing there).
Why this was flagged
The trigger is a GET on the SSE path of MCPServer.sse_app() with a token verifier configured, whose stream ends while the ASGI server still processes send (server-side stream end such as shutdown, or a server that does not ignore sends after disconnect). sse.connect_sse runs EventSourceResponse(...)(scope, receive, send) at src/mcp/server/sse.py:205, so the response is already started on that send. sse_endpoint then returns Response() at server.py:1186 and request_response (server.py:1245) calls it with the same send, emitting a second http.response.start. On the base, RequireAuthMiddleware(handle_sse, ...) awaited a raw ASGI function whose Response() return value was discarded, so nothing was sent after the stream. Operators get an exception and traceback per affected connection instead of a quiet return. RequireAuthMiddleware.__call__ (bearer_auth.py:133) only forwards and does not guard this.
Verification: On the base, RequireAuthMiddleware.__call__ ends with await self.app(scope, receive, send) (src/mcp/server/auth/middleware/bearer_auth.py:133) and discards the return value, so the trailing return Response() was never sent on that route. So return Response() at server.py:1186 now emits a second http.response.start on the auth route after every GET stream.
Finding ID: f-8ee46866
| @requirement("transport:sse:head") | ||
| async def test_head_on_the_sse_endpoint_is_answered_without_opening_a_stream() -> None: | ||
| """`MCPServer.sse_app()` answers HEAD at once instead of streaming forever.""" | ||
| app = MCPServer("legacy").sse_app( | ||
| transport_security=TransportSecuritySettings(enable_dns_rebinding_protection=False) | ||
| ) | ||
| async with httpx2.AsyncClient(transport=StreamingASGITransport(app), base_url=BASE_URL) as http: | ||
| with anyio.fail_after(5): | ||
| response = await http.head("/sse") | ||
| assert (response.status_code, response.headers["allow"]) == snapshot((405, "GET")) |
There was a problem hiding this comment.
🟡 nit (optional): The new requirement claims more than its test proves, so a regression that opens a session before answering HEAD would still pass CI. transport:sse:head says HEAD is answered "without opening a session or a stream, so the connection stays usable" (tests/interaction/_requirements.py:2711). The test at test_sse.py:104 asserts only (405, "GET"). Fix: make claim and assertions match, either by narrowing the requirement text and test name to the 405/Allow outcome, or by asserting the rest, e.g. a second request on the same client inside anyio.fail_after(5). [also at: tests/interaction/transports/test_sse.py:97 - nit: The new test's docstring does not say where the asserted behaviour comes from, so a later failure cannot be triaged from the test alone.]
Why this was flagged
The trigger is any later change that keeps the 405 but answers it after sse.connect_sse(...) has been entered, reached through MCPServer.sse_app() at src/mcp/server/mcpserver/server.py:1174-1186. The test at tests/interaction/transports/test_sse.py:98-104 builds the app through MCPServer("legacy").sse_app(...), so it holds no reference to the SseServerTransport and cannot inspect _read_stream_writers the way the first test in the file does at test_sse.py:58-61. It makes one http.head("/sse") call and snapshots status and allow only; nothing checks that no session was registered or that a following request succeeds. The requirement text at tests/interaction/_requirements.py:2711-2712 and the test name ..._is_answered_without_opening_a_stream therefore record coverage the suite does not have. The base branch has no such requirement entry, so this is new with the PR. anyio.fail_after(5) only bounds a hang; it does not prove the session or connection claims. This is what the hollow-proof check in .claude/skills/test-quality/SKILL.md guards against.
Verification: nit — triggering condition: a later change that registers a session before answering HEAD with 405 (no such code exists today, so nothing breaks at merge). tests/interaction/transports/test_sse.py:95-104 links the test to that requirement, and its only assertion is (response.status_code, response.headers["allow"]) == snapshot((405, "GET")) under anyio.fail_after(5). "Without opening a session" has no assertion.
Finding ID: f-53e8b984
| if request.method == "HEAD": | ||
| # The SSE stream never ends, so a HEAD answered with it would never complete. | ||
| return Response(status_code=405, headers={"Allow": "GET"}) |
There was a problem hiding this comment.
🟡 nit (optional): Users now get 405 with Allow: GET from HEAD on the SSE endpoint, but no page under docs/ says so. The behaviour change is at server.py:1177, and the only prose updated is the module docstring example in src/mcp/server/sse.py:18-20. Fix: add a sentence to the page that covers sse_app() (docs/run/asgi.md:36 is its only mention outside docs/migration.md) stating that HEAD is answered 405, and that a hand-rolled connect_sse route needs the same guard. [also at: src/mcp/server/mcpserver/server.py:1177 - nit: AGENTS.md (Documentation) asks that a change to user-visible behaviour update the relevant page under docs/ in the same PR.; src/mcp/server/mcpserver/server.py:1175 - nit: The new 405 answer to HEAD on the SSE endpoint is user-visible, but no page under docs/ is updated. Fix: add a sentence where the SSE server transport is covered (docs/run/index.md:15-18, or the health-check bullet at docs/run/deploy.md:178) saying HEAD on the SSE path is answered 405 with Allow: GET, so probes belong on a custom route.]
Why this was flagged
The trigger is the diff itself: sse_endpoint at src/mcp/server/mcpserver/server.py:1175-1177 changes what HEAD /sse returns from MCPServer.sse_app() on both the auth route (server.py:1244-1246) and the no-auth route (server.py:1261). The diff touches four files and none is under docs/. A search of docs/ for HEAD or 405 finds nothing about the SSE endpoint, and sse_app() is mentioned once, at docs/run/asgi.md:36. On the base branch the same request hung, so this is a user-visible behaviour change. Servers that copied the old handle_sse example from src/mcp/server/sse.py still hang on HEAD, and the only place that tells them to add the guard is that module docstring. No safeguard applies; the repository instruction asks for the docs page to be updated in the same PR.
Verification: nit — triggered by the diff itself: it changes what HEAD on the SSE endpoint returns and touches no page under docs/. src/mcp/server/mcpserver/server.py:1175-1177 adds if request.method == "HEAD": return Response(status_code=405, headers={"Allow": "GET"}) in the shared sse_endpoint. A grep of docs/ for HEAD and 405 finds nothing about the SSE endpoint.
Finding ID: f-728c631d
| async with sse.connect_sse( # pragma: no cover | ||
| request.scope, | ||
| request.receive, | ||
| request._send, # type: ignore[reportPrivateUsage] | ||
| ) as streams: | ||
| await self._lowlevel_server.run( | ||
| streams[0], streams[1], self._lowlevel_server.create_initialization_options() | ||
| ) | ||
| return Response() | ||
| return Response() # pragma: no cover |
There was a problem hiding this comment.
🟡 nit (optional): The GET path of MCPServer.sse_app() stays excluded from coverage, so a regression in the rewritten shared handler would pass CI unnoticed. The rewrite re-adds # pragma: no cover at server.py:1178 and server.py:1186, plus a # type: ignore at server.py:1181. Fix: cover the GET path with an in-process test that connects sse_client to MCPServer.sse_app() through StreamingASGITransport, as the new HEAD test already does for HEAD, then delete both pragmas. [also at: src/mcp/server/mcpserver/server.py:1178 - nit: AGENTS.md (Coverage) says to avoid adding # pragma: no cover / # type: ignore, and that in src/ a # pragma: no cover needs very good reasoning because it usually means a test is missing.]
Why this was flagged
The trigger is any GET to the SSE endpoint built by MCPServer.sse_app(), which now runs through the merged sse_endpoint on both the auth and no-auth routes. The handler body is marked # pragma: no cover on the async with sse.connect_sse( header at src/mcp/server/mcpserver/server.py:1178 and on return Response() at :1186, so no test executes connect_sse, the _lowlevel_server.run call, or the trailing response. The base carried the same two exclusions on the handle_sse and inner sse_endpoint definitions, so the net count is unchanged, but the diff writes them onto new lines while restructuring the code they cover. AGENTS.md says a # pragma: no cover in src/ usually means a test is missing, and the new test at tests/interaction/transports/test_sse.py:96 shows the real app can be driven in process. No safeguard applies: strict-no-cover only fails when a pragma line is executed, and no test sends GET to this app.
Verification: nit — triggered whenever the rewritten GET path of MCPServer.sse_app() regresses, since no test executes it. The pragma on the async with header at src/mcp/server/mcpserver/server.py:1178 excludes the whole block. tests/interaction/transports/test_sse.py:98 (the new test) sends only HEAD. These are relocations, not net-new suppressions.
Finding ID: f-ae798a8e
|
|
||
|
|
||
| @requirement("transport:sse:head") | ||
| async def test_head_on_the_sse_endpoint_is_answered_without_opening_a_stream() -> None: |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md (Testing) says test files mirror the source tree and tests go in the existing file for that module. The new test exercises MCPServer.sse_app() from src/mcp/server/mcpserver/server.py, but is added to tests/interaction/transports/test_sse.py, whose other tests drive build_sse_app(Server(...)) rather than sse_app(). Fix: put the HEAD test in tests/server/mcpserver/test_server.py, next to test_sse_app_applies_the_configured_request_body_limit, keeping an interaction-suite entry only if the requirements manifest needs one.
Why this was flagged
Nothing fails at runtime; this is a layout/discoverability point only. The instruction guards being able to find a module's tests at the mirrored path. The behaviour under test lives in src/mcp/server/mcpserver/server.py (the only non-docstring source change), whose mirrored file tests/server/mcpserver/test_server.py exists and already holds an in-process HTTP test of sse_app() (line 1971). The new test instead lands in the interaction suite's SSE file, whose module docstring scopes it to the SSE wiring driven through the suite's own build_sse_app helper; it is the only test there that constructs MCPServer(...).sse_app() directly. Counterweight for the maintainer: tests/interaction/ is an established cross-cutting suite with a requirements manifest (the PR registers transport:sse:head there), it does not mirror the source tree by design, and the test was added to an existing file rather than a new one, so the author may reasonably consider this the right home.
Verification: Instruction: root AGENTS.md (base a525a3d), Testing section: "Test files mirror the source tree: src/mcp/client/stdio.py → tests/client/test_stdio.py. Add tests to the existing file for that module." It is a placement convention with no carve-out for any suite, and it binds the whole repo. Code: the diff adds test_head_on_the_sse_endpoint_is_answered_without_opening_a_stream at tests/interaction/transports/test_sse.py:95-104, which builds MCPServer("legacy").sse_app(...) and asserts (405, "GET").
Finding ID: f-e12e5ad9
|
|
||
| async with sse.connect_sse(scope, receive, send) as streams: | ||
| async def sse_endpoint(request: Request) -> Response: | ||
| if request.method == "HEAD": |
There was a problem hiding this comment.
🟡 nit (optional): AGENTS.md (Branching Model) says any change to an existing API's observable behaviour on the v2 line is a design decision a maintainer makes explicitly and should generally be avoided. MCPServer.sse_app() now answers HEAD on the SSE path with 405 + Allow: GET instead of running the SSE handler, and the auth route now goes through request_response(sse_endpoint), so the trailing Response() is sent on that path too. Fix: get an explicit maintainer sign-off on the new HEAD contract (405 vs a headers-only 200) recorded in the PR, and mark it as a behaviour change rather than non-breaking.
Why this was flagged
Nothing fails at runtime. The instruction guards the 2.x compatibility contract: observable behaviour of a released public API should not change without an explicit maintainer decision. Here the changed behaviour is HEAD on the sse_app() SSE endpoint: before, the request ran the SSE handler (headers sent, response never completed); after, it gets 405 with Allow: GET. A client or probe that treated a 200 header response to HEAD /sse as 'server is up' would now see 405. The PR description itself lists this under 'Breaking Changes' ('HEAD /sse returns 405 instead of hanging') while ticking only 'Bug fix (non-breaking change)'. Sizing: this is a bug fix replacing a hang that reportedly broke keep-alive clients and proxies (LibreChat, nginx), so the old behaviour is unlikely to be relied on; the PR author is shown as Kludex and the checklist line mentions being a maintainer, which I cannot verify from the checkout.
Verification: Instruction: root AGENTS.md at base a525a3d, "Branching Model", second bullet: "v2 is released; its public API is a compatibility contract for the 2.x line. Removals, renames, or any change to an existing API's signature or observable behaviour (including ones softened by a @ deprecated shim) is a design decision a maintainer makes explicitly, and should generally be avoided." It is present essentially as quoted and binds src/.
Finding ID: f-dfcf6cdc
| # Starlette also routes HEAD here; the stream never ends, so a HEAD would never complete | ||
| if request.method == "HEAD": | ||
| return Response(status_code=405, headers={"Allow": "GET"}) |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: Servers built directly on SseServerTransport still open a never-ending stream on HEAD after this merges; only MCPServer.sse_app() is fixed. connect_sse at sse.py:141 checks scope["type"] but never the method, and the documented Route("/sse", ..., methods=["GET"]) also routes HEAD. The new guard at sse.py:19-20 is docstring text, so handlers already copied from it keep hanging. Fix: reject HEAD inside connect_sse before a session is created, as it already does for a failed Host check (send the 405, then raise), so every caller is covered without editing its handler.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
The trigger is a HEAD request to a low-level server whose handler follows the module example: Route("/sse", endpoint=handle_sse, methods=["GET"]) at src/mcp/server/sse.py:12 with a handler that calls sse.connect_sse(request.scope, request.receive, request._send). Starlette adds HEAD to a GET route, so the request reaches connect_sse at src/mcp/server/sse.py:141. That function only checks scope["type"] != "http" at sse.py:142 and the Host/Origin rules at sse.py:148, then registers a session at sse.py:162 and starts EventSourceResponse at sse.py:205, which never completes for HEAD. The diff only edits the docstring at sse.py:18-20, so no existing downstream handler changes behaviour: a keep-alive client or reverse proxy still hangs on its next request, exactly as on the base branch. The sibling entry point already enforces its method itself: handle_post_message at sse.py:228 answers non-POST with 405.
Verification: pre-existing — triggers when a HEAD request reaches a server that calls SseServerTransport.connect_sse from its own handler without a method check. src/mcp/server/sse.py:141-151: connect_sse rejects only scope["type"] != "http" (line 142) and a failed Host/Origin validation (lines 148-151). The only change in sse.py is docstring text at lines 18-20, so it has no runtime effect.
Finding ID: f-6f2cce2e
Fixes #3736
MCPServer.sse_app()now answersHEADon the SSE endpoint with405andAllow: GET, 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.Both routes in
sse_app()now go through onesse_endpoint, which returns the 405 beforeconnect_sse(). With auth configured the route isRequireAuthMiddleware(request_response(sse_endpoint)), so authentication still runs first and an unauthenticatedHEADkeeps getting its 401 (what OAuth-detection probes look for). TheSseServerTransportmodule docstring example, which downstream servers copy, gets the same guard.405 rather than a headers-only 200 matches how the Streamable HTTP transport and the SSE message endpoint already reject other methods.
How Has This Been Tested?
test_head_on_the_sse_endpoint_is_answered_without_opening_a_streamdrives the realMCPServer.sse_app()in process and expects(405, "GET"). Without the fix it times out, reproducing the hang.transport:sse:headrequirement (issue:#3736).strict-no-coverand pyright clean.Breaking Changes
HEAD /ssereturns 405 instead of hanging.GET /sseis unchanged.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
The auth branch of
sse_app()is excluded from coverage, as before, so therequest_responsewrapping there is not exercised by a test. It also means the trailingResponse()after a GET stream ends is now sent on that path too, as the no-auth path already did; by then the client has disconnected.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.