fix: align structured output readers with Sorbet types - #385
fix: align structured output readers with Sorbet types#385jbeckwith-oai wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1290cf5e9d
ℹ️ 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".
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed e01f430b4572b8eeebf5d64ac3625dd5f0a0b9c0. I found two remaining reader-contract gaps in the new wrapper; details are inline. I also agree with the existing unresolved @api private documentation note.
f658366 to
2bebdfd
Compare
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed the new head 2bebdfda62cdece88e476bb831f1028bd402b264. The previous conversion-viability, nullable-array identity, and private-API findings are addressed. One remaining non-nil reader-contract gap is inline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bebdfda62
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc0c6a1647
ℹ️ 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".
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed current head bc0c6a1. The required-nil and stale-union-error fixes address their reported cases. One remaining recursive Symbol-normalization gap is inline.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ad3490c5b
ℹ️ 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".
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Re-reviewed current head 9ad3490. Direct Symbol arrays and unions are fixed, but one remaining model-boundary recursion gap is inline.
ec0f2be to
b592d72
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b592d72ccc
ℹ️ 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".
Summary
Fix the contract mismatch between the Tapioca structured-output compiler introduced in #364 and the raw-value compatibility rollback in #375.
OpenAI::BaseModelstructured-output classes.[]andto_h, including on directly constructed and reassigned structured-output models.Symbolfailures once in the SDK-owned primitive converter;String → Symbolnow works naturally through arbitrary arrays, unions, and nested models without structured-output-specific normalization branches.nil, non-viable boolean, enum, and mutated-container values withOpenAI::Errors::ConversionErrorinstead of violating generated reader types.message.parsedworkflow from Beta Feedback and Feature Requests! #126/Provide supported Sorbet/Tapioca typing for user-defined structured-output models #309 and the Responsescontent.parsedworkflow through real public client calls.Why existing tests did not catch this
Symbolas a class target, even though structured-output schemas explicitly advertiseSymbol; successive container-specific fixes therefore masked the missing canonical scalar behavior.The new compiler test closes the gap by checking every advertised field shape against runtime readers for direct construction, explicit assignment, and parsed responses in the same subprocess that generates and typechecks the RBI. It additionally checks raw identity through both accessors, nullable array elements and parsed array identity, direct/nested Symbol arrays and unions, including Symbol-bearing models inside arrays and unions, enum conversion, both union branches, nested model inheritance, caller-owned mutations, and conversion errors for invalid booleans, enums, and nested container values.
A 108-assertion scalar/container matrix systematically combines every supported scalar (
String,Symbol,Integer,Float, boolean, enum) with nested model, array, and union boundaries across construction, assignment, and parsed conversion. Public-client integration tests exercise bothchat.completions.create(response_format: Model)andresponses.create(text: Model)using Minitest's stub on the SDK's actual HTTP-client protocol. Primitive-converter regressions separately verify valid Symbols, JSON strings, invalid input, and exactness/error accounting. A separate nine-shape nullable matrix verifies explicitly constructed, assigned, and parsednilacross String, Symbol, Integer, Float, Boolean, model, array, enum, and union readers; the generated-RBI subprocess independently checks nullable constructor and assignment behavior.Design tradeoff
Globally restoring nested coercion would regress the published generated-request model contract. Widening generated readers to model/hash unions would make signatures truthful but would also require narrowing every nested field in normal parsed-response code. Documenting the signatures as parsed-only would leave direct construction and assignment unsound.
Instead, the structured-output subclass prepends one small reader module per application-defined model. Each wrapper delegates to the original generated reader for existing validation, returns already-materialized values unchanged, and uses the existing canonical converter only when the raw value does not already match its declared target. Raw storage remains unchanged, and generated SDK request models never enter this path.
Symbolconversion belongs alongside the converter's existingString,Integer, andFloatprimitive branches, not in separate model/array/union pre-processing hooks. Fixing that canonical scalar branch deletes 41 lines of workaround logic and automatically handles every recursive composition. An audit of all 2,308 generated/internal model classes found zero directSymboldeclarations, and the generated-request raw-value compatibility suite remains unchanged and green. This changes scalar parsing only; it does not restore global nested-model coercion.Setters are intentionally not added to generated RBIs: the documented compiler contract promises readers.
Castiron ownership
No upstream Castiron compiler or renderer change is required.
The SDK contributor guide explicitly says the generator never modifies
lib/openai/helpers/. Castiron's authoritativegenerated-file-excludes.yamlindependently excludesREADME.mdglobally, bothlib/openai/helpers/andlib/openai/internal/for Ruby, and the handwrittentest/tree except generated resource tests and namespace scaffolding. I evaluated that policy against every changed PR path:Castiron's Ruby renderer generates package/client/model/resource/signature files and resource tests; it does not emit the handwritten structured-output helper or these tests. The existing Tapioca compiler file is unchanged.
Verification
mise exec ruby@4.0.6 -- env TEST_API_BASE_URL=http://127.0.0.1:4520 ./scripts/test— 648 runs, 2,813 assertions.mise exec ruby@3.4.10 -- env TEST_API_BASE_URL=http://127.0.0.1:4520 ./scripts/test— 648 runs, 2,813 assertions.mise exec ruby@3.3.12 -- env TEST_API_BASE_URL=http://127.0.0.1:4520 ./scripts/test— 648 runs, 2,813 assertions.mise exec ruby@4.0.6 -- bundle exec rake linton current main — 1,389 RuboCop files clean, Sorbet clean, and 1,212 RBS files valid.mise exec ruby@4.0.6 -- bundle exec rake build:gem; package contains the handwritten structured-output runtime and compiler with nosorbet-runtimeruntime dependency.