Repository navigation
feat(api): support Live sideband Decision commands - #1193
Conversation
Castiron custom codeEvaluated main: Trusted main policy: ✅ No new custom-code files detected. 98 mixed files remain; 0 existing customizations changed. Compared 98 existing customizations unchanged
58 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 39023795984 --repo openai/openai-java \
--name castiron-custom-code-39023795984-1 --dir /tmp/castiron-custom-code-39023795984-1
git apply --stat /tmp/castiron-custom-code-39023795984-1/custom-code.patch
cat /tmp/castiron-custom-code-39023795984-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 71916f9f8a456f3055b56b6b12d4334479fdc280 2e5b9b2ae12d4e89513fd6c99b9c7031dde01f4c
python3 scripts/castiron/custom_code_report.py report \
--base 71916f9f8a456f3055b56b6b12d4334479fdc280 \
--head 2e5b9b2ae12d4e89513fd6c99b9c7031dde01f4c --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-2e5b9b2ae12d
cat /tmp/castiron-custom-code-2e5b9b2ae12d/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. |
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
The generated public visitor surface has one blocking compatibility regression; see the inline finding.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed 9f3b319febfd3b283b8b0da9fe9668428a8a7c76 against 812a4fb74885f69b130c82d024f44541f73b8f5e.
One compatibility issue needs fixing: the new Decisions branch adds an abstract method to five existing public Delegation.Visitor interfaces. Existing client/responses visitors consequently stop compiling on an SDK upgrade, even when the application does not use Decisions. I independently confirmed the same regression that Dan posted during this review: #1193 (comment). Please use the existing finding to track the fix; I am not adding a duplicate inline comment.
I checked the schema/model mapping, nullable correlation IDs, native Decision payload, trigger omission versus empty-list handling, normal/fork event dispatch, typed sideband overloads, generated metadata, and test changes. The manual transport change delegates to the existing send path. No other actionable issue found.
Static review only; no candidate code, tests, builds, or generation were executed locally. git diff --check passed. Hosted build, lint, Jackson/API/runtime compatibility, and Castiron checks are successful at this head; the main test job and OkTest are still running.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed all 53 changed files at 9f3b319febfd3b283b8b0da9fe9668428a8a7c76 against 812a4fb74885f69b130c82d024f44541f73b8f5e. I found two compatibility defects that need fixes: existing delegation visitors gain an abstract method, and copied configs with omitted trigger or question lists throw on the first append. Dan has already reported the visitor issue in #1193 (comment); I independently confirmed it and am using that thread to track the fix. The distinct copied-list issue and its regression cases are inline.
The event unions and sideband overloads otherwise preserve the existing transport and unknown-event behavior. The new tests cover populated models and wire roundtrips, but miss the two cases above. I accepted the larger model/test files because their nested schema types and per-variant tests remain clearly separated; an extra runtime abstraction is not warranted.
Validation: immutable source and blob-identity review, including an independent full-diff pass. I did not execute the candidate. Hosted build, lint, Jackson, API compatibility, Java 8/25 runtime compatibility, and Castiron checks passed at the final check; the main tests and OkTest were still running. Those passing checks do not cover the two demonstrated source paths.
8f16f07 to
098e21d
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed 098e21d9c50f9fe77b450b946e3342955bffec60. The five delegation visitor defaults and six copied-list corrections fix the earlier findings. One compatibility issue remains: SidebandClientEvent.Visitor adds three abstract Decision methods, which breaks existing visitor implementations. I left one inline finding for that interface.
I reconciled the 59-file scope against the earlier review, checked the changed source and regression tests, and verified the generated checkpoint and disclosed customizations. Seventeen hosted checks passed and two skipped; the separate OkTest status still reports running. I did not execute candidate code or independently run the author's compiled-consumer probe.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 098e21d9c50f9fe77b450b946e3342955bffec60. The five delegation visitor defaults fix my earlier finding, and the six copied-list corrections correctly distinguish omission from explicit null/malformed values.
One P2 compatibility issue remains: SidebandClientEvent.Visitor gains three abstract Decision methods. Existing visitors implementing the prior ten variants stop compiling after an upgrade; previously compiled visitors can hit AbstractMethodError when a new variant is dispatched. I independently verified Mark's existing finding at #1193 (comment) and am using that thread rather than adding a duplicate. Please give those three methods the established unknown(JsonValue.from(...)) defaults and extend the old-visitor regression to this interface.
Coverage: reconciled all 59 changed paths with the prior review, reused 43 identical blobs, and inspected the 16 changed/new paths and relevant current-base context. The sideband send overloads preserve the existing transport. The six documented list customizations are the only current changed generated sources differing from their manifest hashes. git diff --check passed; GitHub's current-head checks and Castiron statuses are green. Static review only: I did not run candidate code, tests, builds, generation, or the author's compiled-consumer probe.
098e21d to
daaf0c2
Compare
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed daaf0c293e893b298492a49a87e99d3d85db0fbd. The earlier compatibility findings are fixed: all six affected visitor interfaces retain their existing signatures and route unhandled typed variants through unknown, including the three new sideband Decision methods. The JVM-default setting is unchanged, and the new sideband regression checks the legacy visitor shape and default methods. The documented loss of compile-time exhaustiveness is the tradeoff for that fallback behavior.
I reconciled all 180 changed paths. The broader generated-list change preserves existing guards and changes only missing-list handling; explicit null and malformed values still fail on append. The sideband overloads keep the existing transport path. The generated manifest, remaining customizations, schema changes, and regression tests are consistent. No actionable findings.
All 17 current-head repository checks passed; two queue signals were skipped. This includes SDK tests, API/Jackson/runtime compatibility, and Castiron baseline and budget checks. The separate OkTest run is still running. I reviewed immutable source and hosted results without executing candidate code or independently running the author's precompiled-consumer probe.
daaf0c2 to
78f9326
Compare
78f9326 to
0747a5c
Compare
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the compatibility findings on 0747a5c17268fa366487526a34ab807e8bac1de7; the previously requested changes are addressed.
The five delegation visitors and all three new SidebandClientEvent visitor callbacks now default to unknown(JsonValue.from(...)). The regression tests retain the old visitor implementation shape and assert JVM default methods. The copied-list append path initializes omitted values while preserving omission on an untouched copy; explicit null and malformed values still fail, with focused regression coverage.
I compared this head with Mark's approved daaf0c2: the Live models, helper and regression tests are byte-for-byte unchanged. The intervening baseline/model notices are retained. This is a static follow-up review; I did not rerun the tests locally. Hosted CI on the fresh head is still running and remains a merge gate. No remaining findings from my earlier reviews.
Castiron-Internal-PR: openai/openai-java-internal#275 Castiron-Source-SHA: 081bde9862a8522787beec632c71662ff19bce03 Castiron-Public-Base-SHA: 71916f9
0747a5c to
2e5b9b2
Compare
yoni-oai
changed the title
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the narrowed six-file change at 2e5b9b2ae12d4e89513fd6c99b9c7031dde01f4c against current main 71916f9. The remaining production change is three typed sideband send overloads delegating through the existing sendCommand path; the transport test verifies all three wire messages and a nullable-correlated Decision result. The other changes are the six API-navigation entries, generation metadata, and visitor/copied-list regression tests. No remaining findings. Current-head hosted CI is green and all review threads are resolved. Static review; I did not rerun the tests locally.
4 participants
Live Decision models are already present in the Java baseline, but
LiveSidebandConnectionhas no typedsendoverloads for the three Decision commands. Add overloads fordecision.create,decision.item.create, anddecision.item.delete, and regenerate the API-reference input to include three client and three server Decision events in the Live reference groups.The residual change preserves the generated Java APIs and existing service customizations from openai/openai-java#1213. The generated-file inventory and generated SDK source are unchanged. Regression tests cover sideband transport, omitted list handling, and delegation visitor defaults.
Validation on the promotion source: 15 focused tests passed; the trusted baseline custom-code check passed at 2,021 / 3,000 lines against the verified generated snapshot. See the checks on this PR for hosted results.