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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1336ccfa27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| class EventSource: | ||
| def __init__(self, response: Response, max_event_size: int | None = DEFAULT_MAX_EVENT_SIZE_BYTES) -> None: | ||
| self._response = response | ||
| self._response.encoding = "utf-8-sig" |
There was a problem hiding this comment.
If a caller accesses response.text before passing a buffered response to the public EventSource constructor, this assignment now raises ValueError: the Response.encoding setter explicitly rejects changes once _text exists. This also affects client.sse() when a response hook reads and logs the response body before the source is constructed. Previously, these responses could still be parsed because iter_text() decodes the buffered bytes independently of the cached text. Apply the SSE-specific decoder to the byte iterator instead of requiring that the response encoding remain mutable.
Useful? React with 👍 / 👎.
Merging this PR will not alter performance
Comparing 
Footnotes
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found and verified against the latest diff
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="tests/httpx2/test_sse.py">
<violation number="1" location="tests/httpx2/test_sse.py:256">
P3: These decoding regressions are covered only through synchronous SSE iteration. Add async coverage for the split leading BOM and non-UTF-8 charset cases so the separate `aiter_text()` path is verified as well.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| assert [event.data for event in events] == ["hello", "world"] | ||
|
|
||
|
|
||
| def test_leading_bom_split_across_chunks_is_ignored() -> None: |
There was a problem hiding this comment.
P3: These decoding regressions are covered only through synchronous SSE iteration. Add async coverage for the split leading BOM and non-UTF-8 charset cases so the separate aiter_text() path is verified as well.
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 tests/httpx2/test_sse.py, line 256:
<comment>These decoding regressions are covered only through synchronous SSE iteration. Add async coverage for the split leading BOM and non-UTF-8 charset cases so the separate `aiter_text()` path is verified as well.</comment>
<file context>
@@ -253,6 +253,35 @@ def handler(request: httpx2.Request) -> httpx2.Response:
assert [event.data for event in events] == ["hello", "world"]
+def test_leading_bom_split_across_chunks_is_ignored() -> None:
+ def chunks() -> Iterator[bytes]:
+ yield b"\xef"
</file context>
There was a problem hiding this comment.
1 issue found across 2 files
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="tests/httpx2/test_sse.py">
<violation number="1" location="tests/httpx2/test_sse.py:290">
P2: This test never exercises a hook consuming a live SSE stream: `sse_handler` eagerly caches `content=SSE_BODY`, making `response.read()` a no-op before the hook runs. Return a `Response(stream=...)` from this test so the hook actually buffers the response before `EventSource` iterates it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| response.read() | ||
| assert response.text | ||
|
|
||
| with httpx2.Client(transport=httpx2.MockTransport(sse_handler), event_hooks={"response": [read_text]}) as client: |
There was a problem hiding this comment.
P2: This test never exercises a hook consuming a live SSE stream: sse_handler eagerly caches content=SSE_BODY, making response.read() a no-op before the hook runs. Return a Response(stream=...) from this test so the hook actually buffers the response before EventSource iterates it.
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 tests/httpx2/test_sse.py, line 290:
<comment>This test never exercises a hook consuming a live SSE stream: `sse_handler` eagerly caches `content=SSE_BODY`, making `response.read()` a no-op before the hook runs. Return a `Response(stream=...)` from this test so the hook actually buffers the response before `EventSource` iterates it.</comment>
<file context>
@@ -253,6 +253,47 @@ def handler(request: httpx2.Request) -> httpx2.Response:
+ response.read()
+ assert response.text
+
+ with httpx2.Client(transport=httpx2.MockTransport(sse_handler), event_hooks={"response": [read_text]}) as client:
+ with client.sse("http://testserver/sse") as source:
+ events = list(source)
</file context>
|
Closing: a leading BOM or a non-UTF-8 charset on |
Summary
A leading UTF-8 BOM made the first SSE event disappear, and a
charsetother than UTF-8 ontext/event-streamgarbled event data. The SSE spec says event streams are always UTF-8 and one leading BOM is ignored, which is what browsers do.The SSE parser now reads bytes and decodes them with an incremental
TextDecoder("utf-8-sig"), which handles both, including a BOM split across chunks.response.encodingis left untouched.Checklist
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.