fix: restore raw BaseModel value contract - #373
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Generator compatibility audit complete: no Castiron PR is required for this fix.
This means integrated future regenerations preserve the implementation and its regression suite as repository-owned runtime code; adding a generator template or compatibility branch would duplicate ownership and is unnecessary. |
apcha-oai
left a comment
apcha-oai
left a comment
There was a problem hiding this comment.
Thanks for putting this together. Restoring the raw-value contract is directionally right, but after comparing this branch against the last published release (v0.78.0), I don't think this is safe to merge as a compatibility fix.
1. Request serialization can silently use stale cached values.
input = [{content: "before", role: "user", type: "message"}]
params = OpenAI::Responses::ResponseCreateParams.new(input: input, model: "gpt-4o")
input[0][:content] = "after"
input << {content: "second", role: "user", type: "message"}
params.to_h[:input]
# => [{content: "after", ...}, {content: "second", ...}]
dumped, = OpenAI::Responses::ResponseCreateParams.dump_request(params)
dumped[:input]
# => [{content: "before", ...}]On v0.78.0, both mutations are included in the serialized request. Here the raw input and converted cache diverge, so the SDK silently sends a different payload from the one exposed by to_h.
2. Parsed response behavior also changes relative to the published SDK. Fields such as response[:type] and response[:role] that were symbols on parsed response models in v0.78.0 become strings here. The change to the existing streaming assertion from c[:type] == :output_text to c[:type] == "output_text" demonstrates the compatibility break directly. Equality and hashing of otherwise equivalent parsed models change as well.
3. This does not undo the broader parsing changes from #295. The changes to union selection, collection coercion, and conversion-error propagation remain, so fixing to_h alone does not restore the previously published parsing behavior.
My honest recommendation is to revert #295, since it has not shipped, and revisit nested model coercion as a separate, deliberately scoped compatibility change. I don't think layering a second global model-conversion/cache rewrite on top is warranted here.
|
One additional point on the original motivation for #295: if a caller wants typed nested models, they can construct them explicitly instead of relying on global implicit coercion: item = Item.new(name: "example")
container = Container.new(item: item)
container.item.name # => "example"Passing a hash and passing a typed model are meaningfully different choices. Automatically converting every nested hash changes existing behavior across model construction, response deserialization, unions, collections, raw accessors, and request serialization. My recommendation remains to revert #295 and revisit this separately. Globally changing deserialization/coercion to address this is too risky, and we should decide much more carefully whether the right answer is explicit typed constructors, a dedicated conversion API, or a narrowly scoped accessor change. |
|
@apcha-oai Addressed all three points in 19d44c1.
I also rewrote the PR description to match the new implementation. The change does not add or redesign discriminator behavior; #370 remains the separate draft for that work. Local validation is green on Ruby 3.3, 3.4, and 4.0 (613 tests / 2,256 assertions each), plus RuboCop, Sorbet, RBS, packaging, and the thermonuclear code-quality review. Replacement CI is running now. Please take another look when convenient. |
![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: 19d44c13c7
ℹ️ 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".
jbeckwith-oai
changed the title
|
Pushed Validation is green locally across Ruby 3.3.12, 3.4.10, and 4.0.6 (613 runs / 2,256 assertions each), plus lint, gem build, and the thermonuclear review. This intentionally does not attempt a replacement union/discriminator design. @apcha-oai, once the replacement CI is green, could you please take a fresh look at the exact-revert version? |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Closing this PR rather than repurposing it. The forward-fix history and review discussion should remain intact. I’ll open a separate, clean PR from current |
jbeckwith-oai
changed the title
|
Cleanup complete: this closed PR’s original forward-fix title/body have been restored, and |
## Summary - Reverts openai#295 as a standalone mechanical revert. - Adds focused characterization tests for the published raw-request / parsed-response `BaseModel` contract. - Does not introduce a replacement union or discriminator design. ## Commit structure 1. `b269a43e` is the literal Git revert of merge commit `e5152a92962251ce76ceb3d89ec0b6b37fa4efdb`. 2. `6e8650e1` adds only the compatibility test suite. Keeping those changes separate makes the rollback itself directly auditable and the additional contract coverage independently reviewable. ## Why openai#295 globally coerced nested request-model values, changing the documented contract that `BaseModel#[]` and `#to_h` expose the raw values callers supplied. The broader conversion changes also interact unsafely with optional discriminators and schemas where multiple variants share the same discriminator. The previous forward-fix PR openai#373 was closed rather than repurposed. Draft openai#370 was also closed; issue openai#287 remains open for a narrower streaming-scoped solution after the model system is redesigned from first principles. ## Compatibility coverage - raw request identity through accessors, `#[]`, `#to_h`, and `#deep_to_h` - idiomatic nested fetch chains such as `params.to_h.fetch(:input).fetch(0).fetch(:type)` - request serialization after in-place mutation of caller-owned hashes - parsed-model symbol access, equality, and hashing - response coercion retaining expected nested model objects ## Generator scope No Castiron change is required. Castiron intentionally excludes the SDK-owned `lib/openai/internal/` runtime, its signatures, and the handwritten test tree, so generated output is unchanged. ## Validation - focused BaseModel suite: 17 runs, 261 assertions - focused compatibility suite: 9 runs, 39 assertions - full suite via the canonical `scripts/test` entry point on Ruby 3.3.12, 3.4.10, and 4.0.6: 613 runs, 2,256 assertions on each - `bundle exec rake lint`: RuboCop, Sorbet, and 1,212 RBS files green - `bundle exec rake build:gem` - thermonuclear code-quality review: no findings
2 participants
Dependency
This PR must land before #370. PR #370 remains a draft and retains the separate union/discriminator work.
Summary
Why
#295 changed global model construction and response parsing. In addition to replacing nested request hashes in the public raw-value APIs, the first version of this PR introduced a second converted-value cache that could silently become stale when callers mutated their original nested values.
This revision restores the last published behavior directly. It deliberately contains no new union/discriminator parsing design; that remains in #370.
Coverage
The regression suite now directly covers:
Generator
No generator change is required. BaseModel and the related runtime tests/signatures are SDK-owned exclusions, and a full generation plus bootstrap overlay preserved these paths.
Validation