fix: align structured output readers with Sorbet types - #385
fix: align structured output readers with Sorbet types#385jbeckwith-oai wants to merge 4 commits into
Conversation
OkTest Summary🟡 Running for Ruby SDK PR #385. SDK merge (pending) · head ( |
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".
|
|
||
| case converted | ||
| when target | ||
| return converted if state.fetch(:error).nil? && state.fetch(:exactness).fetch(:no).zero? |
There was a problem hiding this comment.
Clear stale union errors before rejecting valid conversions
When a union contains an incompatible alternative before a valid zero-field model (for example, OpenAI::UnionOf[String, EmptyModel] with {}), Union#coerce leaves the first alternative's state[:error] in place even though the model conversion succeeds exactly. This check therefore raises ConversionError for a value that converted to the generated union type; clear/reset stale branch errors or determine success from the selected conversion rather than the shared error slot.
Useful? React with 👍 / 👎.
| value | ||
| else | ||
| state = OpenAI::Internal::Type::Converter.new_coerce_state(translate_names: false) | ||
| converted = OpenAI::Internal::Type::Converter.coerce(target, value, state: state) |
There was a problem hiding this comment.
Convert JSON strings for supported Symbol fields
When a structured-output field is declared as Symbol, its generated JSON schema is a string, so every value returned by the API reaches this reader as a String. The canonical converter has no String-to-Symbol conversion for the Symbol class, causing this new wrapper to raise ConversionError for every valid response instead of satisfying the generated Symbol reader type; explicitly convert these strings or stop advertising Symbol as a supported structured-output field type.
Useful? React with 👍 / 👎.
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.nil, non-viable boolean, enum, and mutated-container values withOpenAI::Errors::ConversionErrorinstead of violating generated reader types.Why existing tests did not catch this
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, enum conversion, both union branches, nested model inheritance, caller-owned mutations, and conversion errors for invalid booleans, enums, and nested container values.
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.
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,lib/openai/helpers/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 -- ./scripts/test— 639 runs, 2,608 assertions.mise exec ruby@3.4.10 -- ./scripts/test— 639 runs, 2,608 assertions.mise exec ruby@3.3.12 -- ./scripts/test— 639 runs, 2,608 assertions.mise exec ruby@4.0.6 -- bundle exec rake lint— 2,612 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.