fix: honor raw StringIO request cursors - #621
Conversation
jbeckwith-oai
added
codex-maintenance
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. |
Castiron custom code✅ No new custom-code files detected. 47 mixed files remain; 0 existing customizations changed. Compared 47 existing customizations unchanged
7 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 33904252106 --repo openai/openai-ruby \
--name castiron-custom-code-33904252106-1 --dir /tmp/castiron-custom-code-33904252106-1
git apply --stat /tmp/castiron-custom-code-33904252106-1/custom-code.patch
cat /tmp/castiron-custom-code-33904252106-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 4322e076e57b769d619175006307763e3c630cfb 38f956abb1dc7518367b4c376b5a6239897ab9d2
python3 scripts/castiron/custom_code_report.py report \
--base 4322e076e57b769d619175006307763e3c630cfb \
--head 38f956abb1dc7518367b4c376b5a6239897ab9d2 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-38f956abb1dc
cat /tmp/castiron-custom-code-38f956abb1dc/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Snapshotting the remaining bytes at encode_content is the right replay boundary: retries need an immutable body without consuming, rewinding, or closing caller-owned StringIO. Using byteslice also correctly treats StringIO#pos as a byte offset, which the multibyte regression pins. Handling FilePart(StringIO) at the same dispatch point avoids the first transport attempt consuming state that a retry would otherwise inherit.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the raw StringIO normalization and public-client regression coverage. The change preserves the caller’s cursor and ownership while allowing both raw forms to replay without transport consumption. Verified passing Ruby 3.3, 3.4, and 4.0 CI suites on the exact head/base merge, plus lint and type checks.
3 participants
Problem
The generic
OpenAI::Client#requestpath did not honor a caller-ownedStringIOcursor for raw request bodies. A directStringIOat byte position 3 sent the fullabcdefbuffer, while aStringIOwrapped inOpenAI::FilePartcould be consumed by the first transport attempt and send an empty retry body.Fix
At the existing raw
encode_contentdispatch boundary, snapshot only the bytes remaining from the currentStringIO#posfor both direct rawStringIOand rawFilePart(StringIO)bodies. The snapshot uses byte slicing and does not read, rewind, close, or otherwise mutate the caller-owned object. The guardedFilePartbranch keeps non-StringIOpayloads on their existing path and deliberately does not recursively re-encode raw string content as JSON.User-visible behavior
For a buffer containing
abcdef:abcdefdefThe regression also checks a UTF-8 byte cursor (
éxpositioned afteré) and preserves caller content, position, encoding, and open ownership.Compatibility and scope
This is limited to raw
StringIOnormalization inlib/openai/internal/util.rbplus a focused public-client regression. It does not change retry eligibility or callbacks, IO stream behavior, multipart encoding, headers,Pathname,String, JSON, JSONL, non-StringIOFilePart, generated resources, signatures, dependencies, or public APIs.Verification
mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/raw_stringio_request_test.rb— 3 runs, 70 assertionsmise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/file_part_serialization_cursor_test.rb— 2 runs, 30 assertionsmise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/net_http_client_stringio_length_test.rb— 3 runs, 38 assertionsmise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/internal/util_test.rb— 63 runs, 287 assertionsmise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/http_client_test.rb— 36 runs, 158 assertionsmise exec ruby@4.0.6 -- bundle exec rake lint— 2,875 files, no offenses; RBS validation and Sorbet examples typecheck passedTEST_API_BASE_URL=<coordinator mock> mise exec ruby@4.0.6 -- bundle exec rake test— 1,637 runs, 14,583 assertions, 0 failures, 0 errors, 1 skipReview and security
Two consecutive fresh adversarial-review rounds completed with two independent read-only reviewers per round and no unresolved in-scope findings. A bounded transport/file security review found no new disclosure, mutation, logging, header, redirect, retry-eligibility, or multipart risk; the change reduces accidental prefix transmission for cursor-positioned raw bodies.
Limitations
The full suite retains its existing single skipped test and emits existing dependency warnings; neither is introduced by this change.