feat(responses): parse structured outputs on retrieval - #532
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: b8e9ee7797
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # Retrieves a model response with the given ID. | ||
| # | ||
| # @overload retrieve(response_id, include: nil, include_obfuscation: nil, starting_after: nil, request_options: {}) | ||
| # @overload retrieve(response_id, include: nil, include_obfuscation: nil, starting_after: nil, text: nil, tools: nil, request_options: {}) |
There was a problem hiding this comment.
For Sorbet-typed consumers, the newly documented responses.retrieve(..., text: ..., tools: ...) call is rejected because rbi/openai/resources/responses.rbi still declares only include, include_obfuscation, starting_after, stream, and request_options for this method. Since the RBI files are shipped with the gem and the existing create signature already models JsonSchemaConverter, add text and tools to the retrieve signature so typed applications can use this feature.
Useful? React with 👍 / 👎.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Thanks for tackling #308! This is a thoughtful improvement, and I especially appreciate the clear background-response example, the focused completed/in-progress coverage, and the quick follow-up adding both RBI and RBS signatures.
I reviewed the updated head and confirmed the type-signature concern is already addressed. Before merging, please address the four remaining compatibility/correctness issues in the inline comments:
- String-keyed local parsing hints currently leak into the HTTP query string.
- Existing
ResponseRetrieveParamsobjects now fail before request conversion. - Supported
ResponseTextConfiginstances that parse correctly during creation silently fail to parse during retrieval. - Retrieval without parsing hints now runs the structured-output parser and can reject previously accepted partial function-call output.
The existing focused suites still pass (114 tests, 627 assertions), but targeted regression cases reproduce each of these four issues. Please add coverage for these cases, preserve the existing no-hints/request-model behavior, and request another review once they are fixed. Thanks again for moving this feature forward!
| # @see OpenAI::Models::Responses::ResponseRetrieveParams | ||
| def retrieve(response_id, params = {}) | ||
| parsed, options = OpenAI::Responses::ResponseRetrieveParams.dump_request(params) | ||
| structured_output_params = duplicate_structured_output_params(params.slice(:text, :tools)) |
There was a problem hiding this comment.
[P1] Keep string-keyed parsing hints local.
The SDK normally accepts string-keyed parameter hashes and normalizes them in dump_request, but this extraction/deletion handles only symbol keys. I reproduced responses.retrieve(id, {"text" => CalendarEvent}) issuing GET /responses/<id>?text=CalendarEvent, which both leaks the supposedly local model name into the URL and sends an unsupported retrieval query parameter. Please normalize parameter keys before extracting/removing text and tools, and add regression coverage for both string-keyed hints.
| # @see OpenAI::Models::Responses::ResponseRetrieveParams | ||
| def retrieve(response_id, params = {}) | ||
| parsed, options = OpenAI::Responses::ResponseRetrieveParams.dump_request(params) | ||
| structured_output_params = duplicate_structured_output_params(params.slice(:text, :tools)) |
There was a problem hiding this comment.
[P2] Preserve existing request-parameter model arguments.
responses.retrieve(id, OpenAI::Responses::ResponseRetrieveParams.new) succeeds on the current base branch because dump_request accepts SDK request-model instances. This new unconditional params.slice call instead raises NoMethodError: undefined method 'slice' before any request is made. Please coerce supported parameter objects to a hash before inspecting local hints, and add a backward-compatibility regression test.
| raise ArgumentError.new(message) | ||
| end | ||
|
|
||
| model, tool_models = get_structured_output_models(structured_output_params) |
There was a problem hiding this comment.
[P2] Support the same typed text configurations accepted by creation.
config = OpenAI::Responses::ResponseTextConfig.new(format_: CalendarEvent) correctly populates content.parsed when supplied to responses.create(..., text: config), but responses.retrieve(id, text: config) silently returns content.parsed == nil. Creation normalizes the model and translates format_ to format before extracting structured-output models; retrieval passes the unnormalized object straight into the parser. Please normalize supported typed configurations before model extraction and add a create/retrieve parity test.
|
|
||
| model, tool_models = get_structured_output_models(structured_output_params) | ||
|
|
||
| unwrap = -> (raw) do |
There was a problem hiding this comment.
[P2] Preserve the no-hints retrieval path.
unwrap now invokes ResponseParser.parse! on every retrieval, including calls without text or tools. An in-progress response containing a partially populated {type: "function_call"} item succeeds on the base branch but now raises KeyError: key not found: :name, even though no tool parsing was requested. Please install the parser only when a usable text/tool model is present, and add a regression test showing ordinary retrieval remains backward-compatible.
|
Addressed the four requested compatibility issues in 8b44e9c:
Added focused regressions for each case. Fresh verification: 26 runs, 171 assertions, 0 failures/errors; rubyfmt 0.14.1 clean; RuboCop clean. Ready for another review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b44e9cfcb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| include: T::Array[OpenAI::Responses::ResponseIncludable::OrSymbol], | ||
| include_obfuscation: T::Boolean, | ||
| starting_after: Integer, | ||
| text: T.nilable(OpenAI::StructuredOutput::JsonSchemaConverter::Input), |
There was a problem hiding this comment.
When a Sorbet-typed caller passes an OpenAI::Responses::ResponseTextConfig to retrieve, the runtime supports it—as the new test_typed_text_config_has_create_and_retrieve_parity test demonstrates—but this signature accepts only JsonSchemaConverter::Input, so Sorbet rejects the call. Although retrieval hints were added to the RBI, the fresh typed-config test exposes this remaining mismatch; mirror the create signature by including ResponseTextConfig::OrHash alongside the converter type.
Useful? React with 👍 / 👎.
2 participants
Summary
responses.retrieveto accept the same localtextandtoolsstructured-output models used when creating a background responseCloses #308.
Tests
rubyfmt 0.14.1 --checkon the 2 modified Ruby filesrubocopon the 2 modified Ruby files: no offenses