Conversation
![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: 7cb43a2af3
ℹ️ 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".
| # of ASCII digits; anything else (a sign, whitespace, a decimal point) | ||
| # must be ignored rather than parsed leniently by int(). | ||
| if value.isascii() and value.isdigit(): | ||
| self._retry = int(value) |
There was a problem hiding this comment.
On the default supported CPython configuration, an otherwise valid ASCII-digit retry value of 4,301+ digits passes this predicate but int(value) raises ValueError because of Python’s integer-string conversion limit, aborting both sync and async SSE iteration. The prior try/except safely ignored this field, so preserve that handling after validation rather than letting a large streaming line terminate the response.
AGENTS.md reference: AGENTS.md:L114-L121
Useful? React with 👍 / 👎.
…y int()-ing it int() accepts -1, +1000, leading/trailing whitespace, and other forms the SSE spec doesn't allow for the retry field, only ASCII digits are valid. retry: -1 was silently accepted as a negative reconnection time instead of being ignored. Validate with value.isascii() and value.isdigit() before assigning, matching the spec's [0-9]+ grammar exactly.
7cb43a2 to
3c1adb7
Compare
|
rebased onto main, #3827 landed in the meantime and touched the same file (different section, error-wrapping around reading the stream, not the retry field), had one test-file import conflict to resolve. retry validation was still needed after #3827, confirmed on current main before rebasing. full test_streaming.py suite: 49 passed, 12 skipped (legacy httpx lane, correctly gated). |
|
Thanks for putting this together! We’re closing this alongside #3834: the validation mismatch is real, but the SDK doesn’t use this field to control retries, and we don’t currently have an example of it causing an application failure. If we’ve missed an affected use case, please share a minimal repro on the issue—we’re happy to revisit both the issue and this change. |
2 participants
Description
SSEDecoder.decode() used int(value) for the retry field, which accepts -1, +1000, leading/trailing whitespace, and other forms the SSE spec doesn't allow. Per the spec, the retry field is only valid when it consists entirely of ASCII digits.
retry: -1 was silently accepted as retry == -1 instead of the field being ignored.
Fix: validate with value.isascii() and value.isdigit() before assigning, matching the spec's [0-9]+ grammar exactly instead of parsing leniently.
Closes #3834
Verification
Added test_sse_decoder_ignores_invalid_retry_value (parametrized over -1, +1000, 1.5, leading/trailing whitespace, 1e3, and empty) and test_sse_decoder_accepts_valid_retry_value directly against SSEDecoder.decode(). Confirmed 4 of the invalid-value cases genuinely fail on unfixed main (-1, +1000, and the two whitespace variants all currently parse successfully) and pass with the fix. Full tests/test_streaming.py suite: 28 passed.