Repository navigation
feat(api): support audio fixture playback in Live sessions - #1185
Conversation
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 113 mixed files remain; 0 existing customizations changed; 3 generated baselines changed. Compared
110 existing customizations unchanged
70 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 38821281267 --repo openai/openai-java \
--name castiron-custom-code-38821281267-1 --dir /tmp/castiron-custom-code-38821281267-1
git apply --stat /tmp/castiron-custom-code-38821281267-1/custom-code.patch
cat /tmp/castiron-custom-code-38821281267-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 75b46c0b774cadd323b9b31d24ce150a56700d91 8dae95545d93d5d77cee16d81235fbb10bc1cae0
python3 scripts/castiron/custom_code_report.py report \
--base 75b46c0b774cadd323b9b31d24ce150a56700d91 \
--head 8dae95545d93d5d77cee16d81235fbb10bc1cae0 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-8dae95545d93
cat /tmp/castiron-custom-code-8dae95545d93/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
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. |
775ac8f to
7eb7e69
Compare
![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: 7eb7e69d68
ℹ️ 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".
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the full 18-file diff at 7eb7e69, including the generated models/unions, sideband send path, and all changed tests.
Requesting changes for the existing visitor compatibility finding, which I independently confirmed. The new fixture methods in ServerEvent.Visitor, ClientEvent.Visitor, and ForkClientEvent.Visitor are abstract. Existing implementations therefore fail to recompile, and an already-compiled visitor can throw AbstractMethodError when accept() dispatches a newly typed fixture event. Please give these added methods a default that delegates to unknown(...), as ForkServerEvent.Visitor already does, and cover an old-style visitor consumer. No duplicate inline comment added.
The maximum-size outbound fixture regression suggested in the other existing comment would also be useful, but I did not establish a production size-rejection bug: the default message limit is Int.MAX_VALUE. The blocking issue above is the visitor API break.
Static source review only; no local code/tests/builds were executed. All current non-skipped hosted checks are green. I inspected the successful test job and the API-compatibility log; the latter reports structured-output API probes, not an old Live visitor consumer test. No additional security finding in the reviewed diff.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete 18-file change at 7eb7e69d6851e40c0f3c29e20267632c5a44af65.
The existing visitor compatibility finding still needs correction. ClientEvent.Visitor, ServerEvent.Visitor, and ForkClientEvent.Visitor add abstract methods. Existing implementations fail to recompile, and an already compiled visitor can throw AbstractMethodError when a new fixture event is dispatched instead of reaching its unknown(...) handler. Generate default methods delegating to unknown(JsonValue.from(event)), as ForkServerEvent.Visitor already does, and add an old-style visitor consumer regression. The project enables -Xjvm-default=all. I independently confirmed the existing finding and have not duplicated its inline thread.
The fixture models, union mappings, and sideband command guard otherwise look consistent with the schema. The maximum-size fixture test suggested in the other thread would strengthen coverage; I did not establish a production size-rejection defect because the default message limit is Int.MAX_VALUE.
All 14 non-skipped current-head hosted checks passed; one queue signal was skipped. Review covered source, checked-in tests, and transport/serialization trust boundaries. No local tests, builds, generation, or live API calls were run.
Castiron-Internal-PR: openai/openai-java-internal#269 Castiron-Source-SHA: 5efa70326247e2eaedff12cafbcc5067257e6f2a Castiron-Public-Base-SHA: 75b46c0
7eb7e69 to
8dae955
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete 20-file change at 8dae95545d93d5d77cee16d81235fbb10bc1cae0. No actionable findings remain.
The new fixture visitor methods now delegate to unknown(JsonValue.from(event)). Together with the existing -Xjvm-default=all setting, this resolves the earlier abstract-method compatibility defect. Existing overrides retain their dispatch behavior. The whole-union setting also makes 44 existing methods optional: new visitors lose compile-time exhaustiveness, while the default unknown still throws unless the consumer overrides it. That scope is deliberate and consistent across the three selected unions.
The compatibility test exercises a visitor with the old method surface and checks that all four new methods are JVM defaults. The sequential payload regression sends all 1,920,000 Base64 characters for a synthetic 30-second PCM fixture through the public sideband connection to a local WebSocket peer, without raising the default limit. These address the two existing review threads. They do not execute a separately compiled old consumer or verify remote audio playback.
At the latest check, hosted API compatibility and every other completed, non-skipped check passed; the test job was still running. Review covered source, existing tests, and transport/serialization trust boundaries. No local tests, builds, generation, or live API calls were run.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed at 8dae95545d93d5d77cee16d81235fbb10bc1cae0. The visitor compatibility blocker from my previous review is fixed; approving this revision.
I reviewed the complete six-file update against my prior full-diff review. The four added fixture visitor methods now delegate to unknown(JsonValue.from(event)), and the existing -Xjvm-default=all setting makes them actual JVM default methods. Existing implementations keep their overrides and no longer need to implement the new methods. The whole-union configuration also makes 44 pre-existing methods optional: this intentionally relaxes compile-time exhaustiveness, while the default unknown still throws unless the caller overrides it.
The new compatibility test exercises an old-shaped visitor and verifies all four new methods are JVM defaults. The added public-entrypoint sideband test sends the full 1,920,000-character Base64 fixture through OkHttp to a local WebSocket peer without raising the default limit. These tests cover the reported regression; they do not constitute a separately compiled old-consumer test or remote playback validation.
All 14 current-head non-skipped hosted checks now pass, including tests, API compatibility, Jackson compatibility, Java 8/25 runtime compatibility, and the trusted custom-code checks. I also reviewed the companion configuration Delta #2543303; it has an exact-head peer approval, so I am not duplicating that approval. Its CI is still running.
Static source/security-focused review only; no local code, tests, builds, generation, or live calls were executed. No new findings. No budget, ownership, workflow, or publication policy changes are included.
3 participants
Summary
Send an audio fixture in a Live session and handle the events for fixture acceptance and server playback completion.
Changes
session.output_audio.fixture.play.session.output_audio.fixture.playingwhen the server accepts the audio, and correlate it with the play event usingclient_event_id.session.output_audio.fixture.playedwhen the server mixer drains. Audio buffered by the transport may still be playing.