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
![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: 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
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
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
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
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".
|
Closing after architecture review. OpenAI::BaseModel is an SDK transport model with permissive construction and raw-storage compatibility; retrofitting it into a strongly typed modeling framework creates conflicting reader/storage semantics and unnecessary coercion complexity. The customer need is useful, but the proposed abstraction is not. A future design should evaluate purpose-built Ruby typed models such as Sorbet T::Struct and derive schemas/hydration at the integration boundary rather than extending this PR. |
## Summary - Generate structured-output JSON Schema properties, required names, and reused-definition paths from each field's canonical `api_name`. - Reject duplicate API aliases explicitly instead of silently collapsing schema properties. - Encode each definition path segment separately to prevent direct-alias/nested-path collisions, escape complete `$defs` names as RFC 6901 JSON Pointer tokens, and percent-encode URI fragments correctly. - Support aliases containing `/`, `~`, `#`, spaces, `%`, reserved URI characters, and Unicode while preserving valid `$defs` references. - Keep internal `api_name` metadata out of emitted schemas while preserving descriptions, nullable fields, Ruby readers, API parsing, and request serialization. - Remove the redundant converter dispatch branch and reuse the existing canonical JSON Schema converter. - Add isolated regression coverage for aliases, duplicate aliases, pointer/URI escaping, deep nested/reused definitions, colliding direct/nested paths, Ruby/API-name collisions, converter round trips, and real WebMock-backed Chat Completions and Responses request/response round trips. ## Castiron / generation ownership **No upstream Castiron generator, compiler, renderer, template, OpenAPI schema, or companion regeneration change is required.** Verified against the actual Castiron generation workflow, not just file appearance: 1. `docs/stainless-sdk-json-fixtures/release-primary/generated-file-excludes.yaml` explicitly excludes Ruby `lib/openai/helpers/` and broad `test/`, while explicitly re-including only generated resource tests and namespace scaffolding. 2. Castiron's `crates/castiron-verify/src/generated_files.rs` implements the exact prefix/include-prefix precedence used by generated-tree verification. 3. Evaluating that real policy against every changed path and a generated control path gives: ``` EXCLUDED lib/openai/helpers/structured_output/base_model.rb EXCLUDED lib/openai/helpers/structured_output/json_schema_converter.rb EXCLUDED test/openai/helpers/structured_output_api_names_test.rb GENERATED test/openai/resources/chat/completions_test.rb ``` 4. Castiron's Ruby renderer (`crates/castiron-render-ruby/src/output.rs` and `src/lib/render_views.rs`) emits package/client/model/resource/signature files and resource tests, not structured-output helper code or handwritten helper tests. 5. The SDK's `CONTRIBUTING.md` independently states that the generator never modifies `lib/openai/helpers/`. The overlapping openai#385 changes unrelated Tapioca/reader behavior; this PR does not incorporate or modify any of that work. ## Regression evidence The initial suite failed against unmodified `origin/main` with **5 failures in 6 tests**: invalid property names, leaked `api_name`, lost nested `$defs`, and incorrect Chat Completions/Responses request schemas. Subsequent reviewer regressions independently failed before their fixes for duplicate aliases, JSON Pointer escaping, URI-fragment escaping, deeply nested definitions, and direct-alias/nested-path collisions. ## Verification - `mise exec ruby@4.0.6 -- env TEST_API_BASE_URL=http://127.0.0.1:4510 ./scripts/test` — **637 runs, 2,577 assertions, 0 failures**. - `mise exec ruby@4.0.6 -- bundle exec rake lint` — **1,390 RuboCop files clean, Sorbet clean, and 1,212 RBS files valid**. - `mise exec ruby@4.0.6 -- bundle exec rake build:gem` — gem builds successfully. - New alias/pointer/URI/collision regressions pass on Ruby **3.3.12**, **3.4.10**, and **4.0.6**. - Existing structured-output suite remains green, with no new Ruby 4 URI deprecation warnings. - Explicit thermo-nuclear code-quality review completed before every push; schema dispatch and complete reference encoding stay in their canonical owning layers.
## Summary - Remove the unreleased `OpenAI::BaseModel` Tapioca DSL compiler, its compiler-specific README section, and the compiler-only tests. - Preserve the independently useful Rails/Tapioca load-order handling and nullable-array Sorbet conversion fixes from openai#364. - Add focused compatibility regressions proving application-defined structured-output models preserve raw nested hashes and arrays on construction/assignment, while Chat Completions and Responses both hydrate nested models in parsed API responses. - Keep the nullable-array Sorbet regression in an isolated subprocess so `sorbet-runtime` remains optional for normal SDK usage. ## Why this is the correct architectural rollback openai#364 addressed the customer report in openai#309 by adding a compiler that generated readers such as `Participant` and `T::Array[Participant]`. Those signatures are unsound for the SDK's supported transport-model contract: ```ruby event = Event.new(participant: {name: "Ada"}, participants: [{name: "Grace"}]) event.participant # => Hash event.participants.first # => Hash ``` Parsed API responses correctly hydrate those same fields into application-defined model objects. The original compiler test only type-checked synthetic RBI usage; it never compared generated reader signatures with direct model construction, assignment, or both public parsed-response boundaries. The proposed runtime workaround in openai#385 introduced additional coercion and conflicting accessor/storage semantics instead of addressing the incorrect abstraction. Architecture review concluded that SDK transport models should remain permissive transport models. Sound application-owned typed structured outputs should instead be designed at the structured-output integration boundary. openai#309 has therefore been reopened and rewritten around the original customer outcome rather than the rejected implementation. The removed compiler has not appeared in a released gem, so removing it now avoids publishing an unsound API contract. The actual live Castiron generator also emitted this compiler; its coordinated upstream removal is openai/openai#1291001. ## Why retain the other openai#364 changes - `lib/openai.rb` loader detection independently fixes Rails/Tapioca load order and retains its existing regression coverage. - `ArrayOf#to_sorbet_type` correctly preserves nullable array elements independently of the deleted compiler; the new isolated subprocess test preserves coverage without making Sorbet a runtime dependency. - Existing SDK request-model raw-value contract tests remain unchanged, and new public `OpenAI::BaseModel` tests cover both individual nested fields and nested arrays. ## Validation - Ruby 4.0.6: **695 tests, 3,134 assertions, zero failures/errors/skips**. - Ruby 3.4.10: **695 tests, 3,134 assertions, zero failures/errors/skips**. - Ruby 3.3.12: **695 tests, 3,134 assertions, zero failures/errors/skips**. - `bundle exec rake lint`: **2,618 RuboCop files clean**, Sorbet clean, **1,212 RBS files valid**. - `bundle exec rake build:gem`; inspected the built gem to verify it contains no Tapioca compiler and does not add `sorbet-runtime` as a runtime dependency. - Castiron companion: **574 Ruby renderer tests passed**, including assertions that both generation profiles omit the compiler while preserving their intended loader behavior. Related customer issue: openai#309. Do not auto-close it: the underlying typed structured-output design remains open.
2 participants
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 sound Sorbet typing for application-owned 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.