Conversation
Castiron-Internal-PR: openai/openai-java-internal#181 Castiron-Source-SHA: 99d7b3139cef86814ebf1f3f98f1965333cbae2e Castiron-Public-Base-SHA: 362468e
Castiron custom codeEvaluated main: ✅ No new custom-code files detected. 91 mixed files remain; 0 existing customizations changed. Compared 91 existing customizations unchanged
51 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 37361400022 --repo openai/openai-java \
--name castiron-custom-code-37361400022-1 --dir /tmp/castiron-custom-code-37361400022-1
git apply --stat /tmp/castiron-custom-code-37361400022-1/custom-code.patch
cat /tmp/castiron-custom-code-37361400022-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 362468ef61b3605dda827c2b0492258eaf9edb50 73a523eecf12b3654033b95bfd5c2011eb94643f
python3 scripts/castiron/custom_code_report.py report \
--base 362468ef61b3605dda827c2b0492258eaf9edb50 \
--head 73a523eecf12b3654033b95bfd5c2011eb94643f --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-73a523eecf12
cat /tmp/castiron-custom-code-73a523eecf12/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed all three changed files at eaa076913ab1f7597f0932d6286d573295778811.
The cleanup fix itself looks correct: both resources are attempted, the first failure is preserved, a distinct second failure is suppressed, and identical exceptions avoid self-suppression. The added tests cover those cases; request authentication and token handling are unchanged.
Requesting changes for the unrelated generation metadata retargeting. This handwritten cleanup does not include the generated-source changes represented by the new snapshot, so it relabels a large set of unchanged model/service files as custom or no longer generated. Please retain main's existing metadata for this fix; details are inline.
Source review and git diff --check completed. Hosted baseline integrity, lint and support-matrix checks passed; the broader CI run is still in progress. I did not execute the SDK or tests locally.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the full three-file change at eaa076913ab1f7597f0932d6286d573295778811. The cleanup code and added tests look sound. The existing generation metadata finding still needs correction.
I independently confirmed that ChatModel.kt is unchanged from base and matches the old generated snapshot. The new checkpoint selects a different version. The trusted report now counts 144 untouched files as newly customized. Please retain the base version of .castiron.stats.yml for this cleanup fix. I am referencing the existing finding to avoid a duplicate inline comment.
The close path preserves the first failure and still attempts transport cleanup. It handles distinct and shared exception objects and follows the existing retry-client pattern. I found no additional issue in the authentication cleanup boundary.
Source review only; no local workloads were run. Hosted lint, baseline consistency, and version-support checks pass. The custom-code budget check fails; the build and downstream checks remain pending.
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. |
This branch has not been deployed
3 participants
When workload-identity authentication cleanup throws, the HTTP transport is now still closed. The first failure is preserved, with a distinct transport failure attached as a suppressed exception.
Fixes #882
Adapted from #883 by @sylvesterkaczmarek, with additional tests for delegate-only failure and identical exceptions.
Validation: 12 focused cleanup tests passed; security-focused and adversarial reviews found no blocking issues.