Conversation
Contributor
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 34648238979 --repo openai/openai-java \
--name castiron-custom-code-34648238979-1 --dir /tmp/castiron-custom-code-34648238979-1
git apply --stat /tmp/castiron-custom-code-34648238979-1/custom-code.patch
cat /tmp/castiron-custom-code-34648238979-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 6e773e774e8e324ddfb94437f471ebd7d22035bf cced0727e6166aa9f2441acdede9316737bf4ff5
python3 scripts/castiron/custom_code_report.py report \
--base 6e773e774e8e324ddfb94437f471ebd7d22035bf \
--head cced0727e6166aa9f2441acdede9316737bf4ff5 --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-cced0727e616
cat /tmp/castiron-custom-code-cced0727e616/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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Alternative to draft PR #997 for issue #886. Starting from current
main, this moves the default transport's OkHttp URL construction intoHttpRequest.url()and has OkHttp use that method. Core now declares the existing OkHttp 4.12.0 dependency (and its Okio transitives). NoHttpClientinterface or wrapper changes are required.This fixes the reported log mismatch and changes the public
HttpRequest.url()output. It deliberately goes beyond #886's narrower suggestion: query spaces change from+to%20too. Compare this tradeoff with #997, which keeps core independent of OkHttp and logs each transport's prepared URL, but leavesHttpRequest.url()unchanged and requires observer support from custom clients and wrappers.Representative behavior with the default client
The initial native request target is unchanged; the public URL and SDK log now agree with the default transport. “Target” means HTTP path and query, excluding the fragment.
https://example.comunless shown)/v1, patha bhttps://example.com/v1/a+bhttps://example.com/v1/a%20b/v1/a%20b/v1, patha+bhttps://example.com/v1/a%2Bbhttps://example.com/v1/a+b/v1/a+b/v1, querya b=c dhttps://example.com/v1?a+b=c+dhttps://example.com/v1?a%20b=c%20d/v1?a%20b=c%20d/v1, pathsa,..,bhttps://example.com/v1/a/../bhttps://example.com/v1/b/v1/b/v1?x=1, pathusershttps://example.com/v1?x=1/usershttps://example.com/v1/users?x=1/v1/users?x=1/v1#section, pathusershttps://example.com/v1#section/usershttps://example.com/v1/users#section/v1/usershttps://example.comhttps://example.comhttps://example.com//HTTPS://EXAMPLE.COM:443/v1https://example.com/v1/v1https://example.com:65536/v1url()returns that text; log prints it, then native construction rejectsurl()throwsIllegalArgumentException; logging fails before dispatchFor newly added path segments, the full measured character-level difference set is space (
+versus%20) plus!,$,&,',(,),+,,,:,;,=,@,[,],~(core previously escaped these 15; OkHttp leaves them literal). For newly added query names/values, only space differs; a literal query+is%2Bin both. Existing base text has additional normalization of raw spaces, Unicode, controls, userinfo, host/IP/port, dot segments, and query/fragment placement. These are observable URL and validation changes, not only alternate spellings.Custom clients and compatibility
A custom
HttpClientstill implements the same methods; no observer forwarding is needed. A core-only consumer gains OkHttp and Okio runtime dependencies.HttpRequest.url()can now throw on a base the custom transport would accept. Because SDK logging callsurl()before dispatch, enabled INFO/ERROR/DEBUG logging can block that transport, while OFF logging still dispatches. A focused test usescustom://logical-endpointto show both paths. If a custom client rewrites the destination internally, the SDK log still shows the shared core URL, which may differ from the actual destination. Later interceptors and redirects can also change it.Review release classification and migration impact for callers comparing, storing, or signing public URL strings, and for core-only and custom-transport users. The change should not be described as universally backward compatible. Please include security-focused review of URL/path validation and transport behavior.
Verification
HttpRequest.url()directly: 117 accepted URLs and 30 rejected bases; all pass. It covers the full added-character set, dot segments, existing queries/fragments, base encoding and controls, normalization, and rejection. Existing URL tests and local sync/async WireMock checks compare the public URL, log, and sent target.SKIP_MOCK_TESTS=true(external OpenAPI mock-server cases skipped); the example Java compilation passes. The isolated core runtime probe passes on JDK 21 and the core runtime classpath contains OkHttp 4.12.0 and Okio 3.6.0. JDK 8 runtime execution was not checked locally because that toolchain is unavailable.