feat: add Responses WebSocket mode - #562
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 47 mixed files remain; 1 existing customization changed. Compared
46 existing customizations unchanged
6 more in the full report. A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 33861286491 --repo openai/openai-ruby \
--name castiron-custom-code-33861286491-1 --dir /tmp/castiron-custom-code-33861286491-1
git apply --stat /tmp/castiron-custom-code-33861286491-1/custom-code.patch
cat /tmp/castiron-custom-code-33861286491-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 32989f4b2d45edb4b2104a40f7561fb06c3341ee 180f75fb4f1843159668183b7cf349999e232bed
python3 scripts/castiron/custom_code_report.py report \
--base 32989f4b2d45edb4b2104a40f7561fb06c3341ee \
--head 180f75fb4f1843159668183b7cf349999e232bed --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-180f75fb4f18
cat /tmp/castiron-custom-code-180f75fb4f18/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed d1bedef. Found two actionable correctness issues in nested-model serialization and application-exception propagation; details inline. Static source review only.
HAYDEN-OAI
left a comment
HAYDEN-OAI
left a comment
There was a problem hiding this comment.
Reviewed 3e2a2a1. The previously reported nested-model serialization and callback-error issues are fixed. One high-priority correctness issue remains: valid function-call wire items are rejected because validation requires the SDK-computed parsed attribute. Details inline. Static source review only.
There was a problem hiding this comment.
Semantic notes
Blocking: this client is enforcing too much policy instead of acting as a permissive, faithful transport. It validates IDs, imposes arbitrary caps, rejects beta/newer fields, treats model coercion as wire validation, and permanently poisons the socket on ordinary shape mismatches.
We should match the SDK’s existing semantics here rather than inventing a stricter WebSocket-specific layer: faithfully pass requests through, parse responses flexibly, preserve unknown data, and avoid permanently poisoning the connection for model-shape mismatches. This should reuse the same established behavior as normal HTTP and streaming paths.
Design notes
I don’t think we should introduce another largely independent WebSocket stack. Responses is unlikely to be our last WebSocket API, and this causes some scaling issues for both consumers and maintainers.
At a high level, we should reuse and generalize the existing WebSocket machinery rather than fork it into another product-specific implementation.
Detailed design concerns:
- blocking:
OpenAI::Responses::Transports::AsyncWebSocket** delegates toOpenAI::Realtime::Transports::AsyncWebSocketand then translatesRealtimeConnectionErrorintoResponsesConnectionError. I consider this blocking: generic WebSocket transport infrastructure should not be owned by one product API and adapted by every other product. The underlying transport and socket wrapper should move into a shared WebSocket namespace before we add another public WebSocket surface. OpenAI::Realtime::ConnectionManagerandOpenAI::Responses::ConnectionManagerduplicate transport validation, opening, handshake completion, connection construction, yielding, closing, aborting, and cleanup behavior. That lifecycle machinery should be shared.OpenAI::Helpers::Realtime::ClientExtensionandOpenAI::Helpers::ResponsesWebSocket::ClientExtensionduplicate WebSocket base-URL parsing, scheme conversion, request-option validation, header filtering, deadline handling, and workload-identity refresh behavior. That request/handshake construction should be shared.OpenAI::Realtime::ConnectionandOpenAI::Responses::Connectionboth implement event iteration, reading, writing, closing, aborting, discriminator lookup, coercion validation, and unknown-event fallback. The common connection and protocol machinery should be shared, even if the public product connection classes remain thin separate facades.OpenAI::Errors::RealtimeProtocolError,OpenAI::Errors::ResponsesProtocolError,OpenAI::Errors::ResponsesClientEventError, andOpenAI::Errors::ResponsesSendError: consider changing t part of a shared WebSocket error hierarchy rather than establishing another product-specific family.- Consumers shouldn’t need to rescue both
OpenAI::Errors::RealtimeConnectionErrorandOpenAI::Errors::ResponsesConnectionError, plus additional equivalent errors for every future WebSocket API. These seem like protocol errors and not specific to the APIs
I feel less strongly about whether Realtime and Responses expose the exact same public connection class. Python and TypeScript retain product-specific connection surfaces, which may make sense given their different resource APIs. But those classes should ideally be thin product facades over one shared WebSocket core—not separate implementations or adapters that translate one product’s transport errors into another’s.
Realtime is new enough that we could either correct its public error surface in a breaking revision or preserve the existing names through compatibility inheritance. This is worth doing imo as we have several WS APIs already (e.g: responses beta)
Meta notes
Stepping back, most of the concerns above fall out of two guiding principles that I think Codex missed:
- SDKs should be thin, permissive clients. They should generally pass inputs through faithfully, return what the API sends, and avoid imposing additional validation or policy beyond what is minimally necessary
- Generic protocol infrastructure should not be owned by a product API. For something like WebSockets, we should build a shared abstraction that specific API surfaces can use, rather than reusing one product’s implementation from another or rebuilding the same stack for every API.
![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: 8135e8fd82
ℹ️ 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: 9697b901c6
ℹ️ 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".
9697b90 to
33bd3a3
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 33bd3a3d9e6f3a1c8f8adc3eb464016a80296062. The transport, connection lifecycle, protocol, and error extraction address most of the earlier structural duplication, and the prior correctness issues are resolved. One maintainability concern remains in handshake construction; details inline. Exact-head CI is green; local runtime tests were unavailable because this leased box has neither Ruby nor mise.
06ba854 to
180f75f
Compare
There was a problem hiding this comment.
The runtime changes address much of my earlier feedback: permissive parsing and forwarding, plus shared WebSocket infrastructure, are the right direction. However, the public typing remains blocking. We cannot ship this API with T.anything replacing its request and event types. Providing consumers with useful, accurate types is a core purpose of this SDK, and Sorbet is a first-class typechecker we support.
P0 — Restore the public request/event typing contract.
ServerEvent and ClientEvent are currently T.anything, with corresponding RBS top aliases. This removes the typing contract from the new API. Decoding events into generated classes at runtime does not substitute for static typing.
This has concrete consequences: Sorbet accepts connection.send_event(123) and connection.send_event(Object.new), while a README-style event loop fails at event.type because the received value is T.anything.
We need a properly typed surface before shipping. Either generate it, which I would prefer, or handwrite complete declarations referencing the generated types and maintain them accordingly. Erasing the public types is not an acceptable workaround for incorrect generated declarations.
P1 — Accurately type the accepted request parameters and support normal Sorbet usage.
response.create needs to describe the parameters it accepts—model, input, reasoning, tools, stream_id, etc.—with their schema-defined types and optionality. The distinction is whether those parameters are accurately typed, not whether the language represents them as keywords or an object.
The existing Ruby Responses#create RBI demonstrates the intended pattern: individually typed keyword parameters referencing generated types. Python similarly exposes typed keywords; Node exposes an object with explicitly typed properties. Please apply that pattern using the WebSocket schema.
The current create(**params) RBI instead declares each keyword value as T::Hash[Symbol, T.anything]. Ordinary string-valued model and input fail Sorbet, while invalid hash-valued versions pass.
Replacing that declaration with T.anything would remove the immediate error without providing the required parameter contract. Please add consumer typechecks demonstrating that normal calls succeed, incorrect known parameter types fail, and generated nested inputs work correctly.
Clarification on stream_id — resolved, not blocking.
I am withdrawing the earlier P2 concern. This is the known Sorbet hash-shape limitation around omitted keys, not a Ruby runtime or WebSocket-specific model regression. The direct stream_id accessor is correctly typed as T.nilable(String), and the RBS declarations already mark the hash key optional. Sorbet cannot express that same optional-key contract precisely in an RBI heterogeneous hash shape; in the supported version, indexing the shape with to_hash[:stream_id] is inferred as T.untyped. The earlier passing hash-indexing example therefore did not demonstrate that Sorbet had established a non-null String.
I consider this concern resolved within the SDK’s existing, documented Sorbet limitation. It is not an additional prerequisite for this PR, and it does not justify replacing the generated event types with T.anything; the P0 and P1 requirements above remain.
The live tests provide useful evidence that the runtime works—including function-call events, generated-model replay, and sequential requests—but green runtime tests and CI do not establish a correctly typed public API.
|
Addressed the remaining review feedback in 80ee595:
The earlier optional Verification: Responses WebSocket runtime/errors/Sorbet tests, Realtime connection/transport/auth/sideband/abort/network-invariant tests, Sorbet examples, RBS validation (1,285 files), focused RuboCop, and two consecutive clean fresh-context adversarial-review rounds. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80ee59546d
ℹ️ 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".
| def send_raw(data) | ||
| raise connection_error(closed_send_message) if closed? |
There was a problem hiding this comment.
When a Responses connection is poisoned by an ambiguous send_event failure, it remains logically open until cleanup, but this inherited send_raw only checks closed?; calling the now-public internal helper can therefore issue another write, and calling it from another thread also bypasses assert_owner!. This defeats the terminal-write and single-owner guarantees and can duplicate response creation, so keep the helper public but override or wrap it for Responses to enforce ownership and poison state before delegating.
AGENTS.md reference: AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| ServerEvent = T.type_alias do | ||
| T.any( | ||
| OpenAI::Responses::ResponsesServerEvent::Variants, | ||
| OpenAI::Responses::UnknownServerEvent | ||
| ) |
There was a problem hiding this comment.
The fresh evidence is that this revision changes ServerEvent from T.anything back to the generated union while Connection#parse_event still intentionally ignores coercion errors and exactness. When a known event is partial—for example, a response.output_text.delta payload missing delta—the converter still returns ResponseTextWsDelta, so Sorbet accepts event.delta.upcase because delta is declared as String, but the accessor returns nil at runtime and the call fails; either reject/fallback for inexact known events or retain a type that reflects best-effort decoding.
Useful? React with 👍 / 👎.
apcha-oai
left a comment
apcha-oai
left a comment
There was a problem hiding this comment.
This is much closer and addresses the immediate typing issues. My remaining concern is that the pieces that evolve with the API are still handwritten.
For example, suppose we add a foo parameter to response.create, backed by a generated FooParam type. Codegen will update the request model, but nothing ensures foo: is added to these handwritten RBI/RBS signatures with the correct type and optionality. The runtime may forward it through **params, while the typed interface falls behind.
Similarly, adding a tool variant can update the generated tool union while leaving the manually enumerated tools: signature stale. The existing consumer tests can continue passing because they only exercise older parameters and tools.
We could adopt a “keep the signatures in sync” pattern, but I consider that an antipattern here. It creates a second copy of the API contract that a person, Codex, or a separate synchronization script must continually reconcile. A check that detects drift would help catch mistakes, but we should avoid introducing that duplication in the first place.
Please generate those schema-dependent pieces through the existing SDK generation pipeline, so adding a parameter or tool updates the WebSocket declarations in the same run.
The transport and connection lifecycle can remain handwritten. The parts that change when the API changes should be generated.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
The transport/lifecycle refactor is much cleaner now, but one API-evolution boundary still looks fragile on the current head: connection_resources.rbi hand-enumerates the response.create keyword set and tool union while the runtime accepts forward-compatible **params. A schema/codegen update can therefore make runtime accept a new field or tool while Sorbet/RBS remains stale with no automatic coupling. I would generate the schema-dependent WebSocket request/event declarations from the same API model pipeline and keep only the transport/lifecycle layer handwritten; otherwise this becomes a second API contract that has to be manually synchronized.
5 participants
Summary
client.responses.connectWebSocket surfaceresponse.createkeyword typing with consumer Sorbet coverageVerification
mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/responses_websocket/connection_test.rb(39 runs, 170 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/responses_websocket/errors_test.rb(1 run, 7 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/responses_websocket/sorbet_test.rb(2 runs, 6 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/connection_test.rb(32 runs, 227 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/async_websocket_transport_test.rb(13 runs, 63 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/network_invariants_test.rb(11 runs, 296 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/auth_retry_test.rb(8 runs, 44 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/sideband_connection_test.rb(13 runs, 47 assertions)mise exec ruby@4.0.6 -- bundle exec ruby -Itest test/openai/realtime/async_websocket_abort_test.rb(1 run, 7 assertions)mise exec ruby@4.0.6 -- bundle exec rake typecheck:sorbet validate:rbsgit diff --checkNotes