Conversation
Castiron custom code✅ No new custom-code files detected. 70 mixed files remain; 0 existing customizations changed. Compared 70 existing customizations unchanged
30 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 34873949779 --repo openai/openai-java \
--name castiron-custom-code-34873949779-1 --dir /tmp/castiron-custom-code-34873949779-1
git apply --stat /tmp/castiron-custom-code-34873949779-1/custom-code.patch
cat /tmp/castiron-custom-code-34873949779-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 6e773e774e8e324ddfb94437f471ebd7d22035bf e308317ad628dee4375c506d17334217a0987219
python3 scripts/castiron/custom_code_report.py report \
--base 6e773e774e8e324ddfb94437f471ebd7d22035bf \
--head e308317ad628dee4375c506d17334217a0987219 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-e308317ad628
cat /tmp/castiron-custom-code-e308317ad628/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.
Requesting changes for one confirmed behavioral compatibility regression: recompiling an existing Kotlin HttpClient by delegate wrapper can bypass its original execution overrides. Details and validation are in the inline comment.
Review conclusions:
- API contract:
HttpRequest.url()and default OkHttp wire construction remain unchanged, but the delegated-wrapper regression must be fixed. Legacy custom clients also intentionally lose URL logging in favor of<URL unavailable>; that remains an explicit compatibility tradeoff. - Generator ownership: None of the 16 changed files belongs to the verified generated snapshot
518b0539b6c7380ca65ceaedae379187c4545226. Generation metadata is unchanged. The custom-code budget passes at 2,157 / 3,000, unchanged from base. - Support matrix: No JVM/compiler floors or runtime dependencies change. Java 8 remains supported; exact-head CI passes its Java 8/25 runtime checks and build/Jackson/API checks. Those checks do not cover this delegated-wrapper behavior.
- API design: Reading the prepared URL from the transport is the right boundary and keeps core independent of OkHttp. Observation should be an explicit opt-in capability that preserves existing wrapper dispatch. A separate optional interface is one possible design; please agree on that API shape before expanding the implementation.
Validation: two independent read-only reviewers identified the same blocker; a separate offline counting-wrapper fixture confirmed it on Java 8 and 25, including against the complete Gradle-built PR JAR. Focused core HTTP and OkHttp tests passed locally (509 passed, 1 skipped), along with Kotlin lint, git diff --check, and the custom-code budget check. Security-focused inspection found no additional supported blocker.
Reviewed head eae6187e5414474dfe3c745ab426eee73467a345 against base 41bdd89bce7d88c14ce09f891b11bb7eb61ba534. No source changes were made.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed the full 16-file diff at eae6187e5414474dfe3c745ab426eee73467a345. No additional actionable findings beyond the existing delegated-wrapper regression.
I independently traced that regression through the SDK call chain and Kotlin delegation rules: adding execution overloads causes a recompiled delegated wrapper to forward those methods around its existing overrides. The logging wrapper invokes that path even with logging off. Preserve original dispatch unless observation is explicitly supported; the existing finding remains unresolved.
The transport remains the single source of the prepared URL, and retry/authentication forwarding preserves the existing lifecycle. Exact-head CI passed, including build, tests, API compatibility, and Java 8/25 checks. This was a read-only review; I did not install dependencies or execute the SDK locally.
eae6187 to
e308317
Compare
dpiet-oai
changed the title
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed the full 11-file change at e308317ad628dee4375c506d17334217a0987219. The previous delegated-wrapper finding is addressed: HttpClient remains unchanged, and both logging paths call the original two-argument methods. The new source-compiled Kotlin delegation regression covers sync/async under OFF and INFO, including wrapper URL changes and unchanged OFF request options.
No actionable findings after independent correctness, structural and security review. Passing observation through existing immutable RequestOptions avoids overload bridges and a second URL-construction implementation. I traced observer forwarding through authentication/retries, callback failure containment, request-body lifetime, userinfo redaction, and the documented custom-transport fallback. The URL is explicitly the initial prepared URL, before later interceptors or redirects.
Exact-head CI is green for tests, API compatibility, Java 8/25, lint and the Castiron budget checks. Hosted logs confirm the core and OkHttp test tasks ran and the external/proposed API compatibility compilation passed. The 147-case URL fixture contains 117 accepted and 30 rejected cases. I did not build or run the SDK locally.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete 11-file diff at e308317ad628dee4375c506d17334217a0987219 against 6e773e774e8e324ddfb94437f471ebd7d22035bf. No blocking findings.
The prior delegated-wrapper regression is fixed: the new three-argument HttpClient overloads are gone, LoggingHttpClient dispatches through the original two-argument sync/async methods, and OFF/no-observer calls preserve the original request/options objects. The new source-compiled HttpClient by delegate coverage exercises both overrides at OFF and INFO and confirms wrapper request mutation reaches OkHttp and the prepared-URL log.
I also traced observer propagation through retries, authentication, workload-identity refresh, nested loggers, custom transports, cancellation, and body cleanup. The transport reports call.request().url before dispatch; caller observer failures are isolated; userinfo is redacted before logging; core gains no OkHttp/Okio dependency; and the existing HttpRequest.url() contract remains unchanged. The documented <URL unavailable> fallback for transports that do not opt in is consistent with the implementation.
Verification: exact-head CI is green for build/Jackson compatibility, tests, lint, API compatibility, Java 8/25 runtime compatibility, required checks, and Castiron budget. I independently ran the focused core and OkHttp test suites under Java 21; all passed. git diff --check is clean.
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: e308317ad6
ℹ️ 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".
| return | ||
| } | ||
| val loggedUrl = | ||
| url?.replace(urlUserInfo) { "${it.groupValues[1]}██@" } ?: "<URL unavailable>" |
There was a problem hiding this comment.
When a custom transport follows the new README guidance and reports a prepared signed URL containing query credentials such as access_token, X-Amz-Signature, or X-Amz-Security-Token, INFO-or-higher logging writes those values verbatim to stderr because this replacement masks only URL userinfo. Sanitize credential-bearing query parameters before printing the prepared URL so transport authentication material cannot leak into collected logs.
AGENTS.md reference: AGENTS.md:L29-L33
Useful? React with 👍 / 👎.
| private fun prepareLoggingRequest(request: HttpRequest): HttpRequest { | ||
| val body = request.body | ||
| return if (level.shouldLog(LogLevel.DEBUG) && body != null) { | ||
| request.toBuilder().body(LoggingHttpRequestBody(request.method, body)).build() |
There was a problem hiding this comment.
When a downstream wrapper changes the HTTP method after LoggingHttpClient and the original request has a body, the observer now makes the request line use the prepared method (for example, --> POST), but this wrapper captures the original method and later emits --> END GET. Use the observer-reported method for the body marker, or otherwise avoid labeling it with the stale method, so a single DEBUG trace does not contradict itself.
Useful? React with 👍 / 👎.
3 participants
Summary
Log the initial URL prepared by the HTTP transport instead of the SDK-rendered
HttpRequest.url()string. The default OkHttp transport reportscall.request().urlbefore sync or async dispatch, so INFO logs reflect its URL normalization, path encoding, and query construction. This change does not alter the outgoing request or the publicHttpRequest.url()method.Addressing Justin's review of the earlier draft
Justin identified that the earlier three-argument
HttpClientoverloads could bypass a freshly recompiled KotlinHttpClient by delegatewrapper's existing two-argument overrides, even when logging was OFF. That could skip request customization or authentication. The replacement removes those overloads and always dispatches through the original two-argumentexecuteandexecuteAsync. Observation travels in per-callRequestOptions; at OFF with no caller observer, the original request and options are forwarded unchanged. A source-compiled regression exercises both delegated overrides at OFF and INFO and verifies that the wrapper's path change reaches OkHttp and the log. The same dispatch correction addresses Mark's confirmation.Behavior and compatibility
RequestOptions.requestObserver. SDK wrappers forward the per-call options, including across retries. An outer logger forwards and logs every attempt. Observer runtime exceptions cannot stop default OkHttp dispatch.<URL unavailable>after a successful call; failures before observation do not invent a request line. The README includes a Java sync/async example for custom transports to opt in.HttpRequest.url()retains its existing behavior, so custom transports that construct outgoing requests from that method still need a separate fix for the public-method part of #886.Verification
The 147-case URL matrix covers 117 accepted URLs and 30 rejected bases; rejected bases yield no dispatch or request URL line. Tests also cover actual WireMock request targets, sync/async calls, delegated wrappers, retries, custom-client fallback, cancellation, body cleanup, authentication and X.509 paths, userinfo redaction, and Java consumer compilation. The public PR's CI must pass its full test, lint, API compatibility, Java 8/25, and Castiron budget checks. A security-focused review of URL/path and transport behavior is required before merge.