Repository navigation
Give each SSE response its own header list - #3726
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. |
There was a problem hiding this comment.
All reported issues were addressed across 1 file (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.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the other response paths in src/mcp/server/_streamable_http_modern.py (the 202, 405, 406 and JSON responses at lines 197, 255, 402, 407, 433) for the same shared-header reuse — they all go through Starlette's Response, which builds a fresh header list per call, so only the SSE http.response.start at line 515 was affected.
Extended reasoning...
The code fix is a one-line copy of the module-level _SSE_HEADERS list at src/mcp/server/_streamable_http_modern.py:515 plus a regression test; it touches no auth, crypto or input-handling surface. The non-SSE response paths were verified to construct headers per call via Starlette Response, so the fix is complete for the header-leak itself. The inline finding on the unrelated AGENTS.md deletion and the new test's missing anyio.fail_after(5) guard is what keeps this from an approve.
Closes #3701.
handle_modern_requestpassed the module-level_SSE_HEADERSlist straight intohttp.response.start. Middleware that appends tomessage["headers"]in place mutated that shared list, so every later SSE response carried all previously appended headers. Each SSE response now gets its own copy.AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.