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. |
![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: 74e50c3212
ℹ️ 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".
| * The size of the file, in bytes. In a completed file upload response, this can be null when | ||
| * the file size is not yet available. |
There was a problem hiding this comment.
When a completed upload returns the newly documented "bytes": null, bytes() still calls getRequired and throws OpenAIInvalidDataException; enabling response validation can reject the entire otherwise-valid FileObject. Update the authoritative schema to make bytes nullable and regenerate an optional accessor rather than changing only the generated documentation.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: this is a real existing limitation. This PR changes the description without changing the accessor, so we can handle it as a nonblocking follow-up. Update the source API schema for completed-upload files, regenerate, and test null byte counts. Before changing Java's Long accessor to Optional<Long>, check whether nullability can be scoped to the upload-specific file shape and plan compatibility for existing callers.
There was a problem hiding this comment.
This bug predates this work and can be resolved in a follow-up PR. The follow-up should correct the completed-upload file schema and regenerate the affected SDK types, with null and non-null regression coverage. It is separate from the turn-item pagination fix in this PR.
| items() | ||
| .last() | ||
| .accept( |
There was a problem hiding this comment.
If a page with has_more=true contains a future AgentSessionItem variant unknown to this SDK, deriving after through this visitor invokes its default unknown implementation and throws, so auto-pagination stops even though the response supplies a forward-compatible last_id cursor. Use response.lastId() for the next request (and apply the same generated fix to the async page).
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A fix for this is prepared and is awaiting review. It switches this root-turn items endpoint to the existing response-cursor paginator: send the response's last_id as after, and continue to stop when has_more is false.
This bypasses the union visitor when calculating the next cursor, so an unknown item variant cannot throw in either sync or async pagination. Java renderer pagination tests passed, including response-cursor and terminal-page handling.
The fix is not in this SDK PR yet; the SDK still needs to be regenerated and this PR updated after the fix lands.
| * A chat completion delta generated by streamed model responses. Streamed audio can arrive | ||
| * in partial updates with an ID, base64 data, or transcript text. The final audio update | ||
| * contains only its expiry timestamp. |
There was a problem hiding this comment.
For Chat Completions with audio output, the newly documented chunks carry delta.audio fields such as ID, base64 data, transcript text, and expires_at, but Delta still models only content, function calls, refusal, role, and tool calls. Consequently audio is relegated to raw additional properties and cannot be consumed through the SDK's typed streaming API; add the audio object to the authoritative schema and regenerate the model.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Missing typed streamed audio is an existing SDK gap, so we can take it as a nonblocking follow-up. Add a chunk-specific audio shape in the source API schema with optional fragment fields, regenerate the SDK models, and cover partial and expiry-only chunks. Include any finish_reason optionality in the compatibility review. Raw additional properties remain available in the meantime.
| * To turn off input audio noise reduction, send this Realtime event: | ||
| * ```json | ||
| * {"type":"session.update","session":{"type":"realtime","audio":{"input":{"noise_reduction":null}}}} | ||
| * ``` | ||
| * | ||
| * For a transcription session, use `"type":"transcription"` inside `session`. Omitting | ||
| * `audio.input.noise_reduction` from an update leaves its current setting unchanged. |
There was a problem hiding this comment.
When noise reduction is already enabled, omitting the field leaves it unchanged, but the newly documented noise_reduction: null update cannot be constructed through the normal typed builders: both RealtimeAudioConfigInput.Builder.noiseReduction and the transcription equivalent accept only a non-null NoiseReduction. The current transformed schema's not construct did not produce a nullable overload, forcing callers onto the raw JsonField escape hatch; represent object-or-null explicitly in the authoritative schema and regenerate the models.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed: this is a real existing typed-builder limitation and a nonblocking follow-up for this PR. Represent object-or-null explicitly in the source schema, regenerate, preserve existing type names, and test that omission leaves the setting unchanged while explicit null disables it. Check compatibility for current builder callers; JsonField remains the workaround until then.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 74e50c3. Two existing findings block approval:
- Pagination cursor: the new blocking and async turn-item pages derive
afterfrom the last item's ID. The schema already permits a legacy message withid: null. For that supported case, the visitor returns an empty Optional and the next request omitsafter, repeating or restarting the page sequence. Use the response'slast_idcursor and test this case withhas_more: true. - Nullable file size: the new completed-upload docs permit null bytes, but
bytes()still callsgetRequired, andvalidate()calls it unconditionally. Align the schema and generated model with that contract, or correct the new documentation. The runtime restriction itself predates this patch.
I assessed all four current threads. Chat audio remains available as raw additional properties, and the Realtime JSON null example can be expressed through JsonField; I did not establish separate new payload failures in those two cases. No duplicate inline comments added.
Source review only. Two hosted checks passed, three are queued, and one was skipped. No local builds, tests, or live API calls.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed 74e50c32129f19216400d3bb3aa6331ac4bbec83 against 362468ef61b3605dda827c2b0492258eaf9edb50.
P2: the new turn-item paginator is not forward-compatible with unknown item variants. I independently confirmed the existing cursor finding: a nonempty page with has_more: true, a valid last_id, and a final item whose type is not yet known deserializes into the SDK's supported unknown union variant. nextPageParams() then visits that item without overriding unknown, which throws OpenAIInvalidDataException before the second request. Both blocking and async pages have this path. Use the response's documented last_id cursor in the generator and add a two-page regression test with an unknown final item. The added terminal-page tests set has_more: false, so they do not exercise this path. No duplicate inline comment added.
The remaining endpoint wiring, path/query separation, usage/grouping additions, Live variants/voices, error enums, and string-property helpers follow the schema and existing conventions. The new turn-items, usage, and Live artifacts match the recorded generation snapshot. The audio/upload/noise-reduction type limitations mentioned in the other comments predate this patch; I am not treating those as newly introduced runtime regressions.
Validation: source review and git diff --check only. Hosted CI currently has two successful checks, three queued, and one skipped. I did not execute local tests or make live API calls.
74e50c3 to
b492bd8
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the update at b492bd8. Both turn-item pagers now use the response's last_id. This fixes cursor selection for null-ID messages and unknown item variants, removes the duplicated visitors, and retains the has_more=false stop.
The completed-upload byte-count issue remains open: the new docs permit null, while bytes() and validate() still reject it. The author's compatibility concern is valid. Qualifying the new SDK documentation to describe this limitation is also an option until the upload-specific schema and model are fixed. The runtime limitation predates this PR; the documentation mismatch remains in this patch. No duplicate inline comment added.
Source review only, including all current threads and the five changed files since my last review. The four exact-head hosted checks are queued. I did not run local builds, tests, or live API calls.
Castiron-Internal-PR: openai/openai-java-internal#212 Castiron-Source-SHA: fea40a116aa5064b489a98155a3298c83c5b4cc7 Castiron-Public-Base-SHA: 362468e
b492bd8 to
fcc7767
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc77675ee
ℹ️ 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".
| environment: | ||
| type: object | ||
| additionalProperties: {} |
There was a problem hiding this comment.
When a caller configures the newly added Live shell tool, environment must be either container_auto or container_reference, but this unconstrained object generates a Shell.Environment that exposes only arbitrary additional properties; even {} builds and validates, while valid container configurations require hand-written JSON keys with no discriminator or required-field checking. Define this as an anyOf over ContainerAutoParam and ContainerReferenceParam and regenerate both initial and update delegation configs so the typed builder cannot silently create requests the server rejects.
AGENTS.md reference: AGENTS.md:L5-L8
Useful? React with 👍 / 👎.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the update at fcc77675eebfac5345debfcebd015adf213649f9. The new sync/async public-client test checks pagination past an unknown item variant. It also checks query/header options and stopping on the final page.
Two existing threads still need changes:
- Hosted-shell environment: the new shell tool documents two allowed container forms, but its generated
Environmentaccepts an arbitrary object and validates{}. Model the allowed forms in the source schema and regenerate both delegation configs. Reuse compatible container shapes while excluding the domain-secret policy that Live explicitly disallows. - Completed-upload byte counts: the new docs permit null, but
bytes()still callsgetRequired. The runtime limitation predates this PR. If its schema fix stays separate, qualify the new SDK docs with the current accessor/validation limitation.
No duplicate inline comments added. Source review only; I did not run local tests or live API calls. All reported current-head hosted checks passed, apart from the skipped queue signal.
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 91 mixed files remain; 0 existing customizations changed; 13 generated baselines changed. Compared
78 existing customizations unchanged
38 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 37380322708 --repo openai/openai-java \
--name castiron-custom-code-37380322708-1 --dir /tmp/castiron-custom-code-37380322708-1
git apply --stat /tmp/castiron-custom-code-37380322708-1/custom-code.patch
cat /tmp/castiron-custom-code-37380322708-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 93841aaf115ddd93b19edeaff0679ada9ea1f2b1 fcc77675eebfac5345debfcebd015adf213649f9
python3 scripts/castiron/custom_code_report.py report \
--base 93841aaf115ddd93b19edeaff0679ada9ea1f2b1 \
--head fcc77675eebfac5345debfcebd015adf213649f9 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-fcc77675eebf
cat /tmp/castiron-custom-code-fcc77675eebf/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
This branch has not been deployed
3 participants
Summary
Adds agent turn-item listing and typed grouping options for usage and costs.
Changes