Repository navigation
fix(ts-client): recognize mixed-case JSON media types - #6077
Open
BlueRaddish wants to merge 2 commits into
Open
BlueRaddish wants to merge 2 commits into
BlueRaddish wants to merge 2 commits into
Conversation
Compare JSON media type tokens without case sensitivity in both success and error paths for fetch and GET-with-body transports. Preserve payload case, explicit response types and non-JSON responses. AI-assisted with OpenAI Codex. Co-authored-by: openhands <openhands@all-hands.dev>
Apply the same case-insensitive JSON media type classification to CloudClient direct and proxy success/error responses. Preserve payload case, explicit response types and non-JSON bodies. AI-assisted with OpenAI Codex. Co-authored-by: openhands <openhands@all-hands.dev>
Collaborator
⏳ Review waiting on checksThe current head
No action is needed. The scheduled scan retries once every check on the head reports a conclusion. This is an automated check - no AI was used to generate this comment. |
all-hands-bot
approved these changes
Oct 20, 2026
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Assessment
The change is correct, minimal, and matches the repository's ownership boundary. clients/typescript/ owns the browser-compatible typed client, and the fix stays entirely inside src/client/ plus new src/__tests__/ coverage.
What I verified on this head (4dd548e)
- All six JSON classification sites are now case-insensitive. A repo-wide sweep for
includes('application/json')underclients/typescript/src/returns exactly the six changed comparisons (http-client.tslines 264, 371, 437, 462;cloud-client.tslines 764, 792) and no remaining case-sensitive site.device-flow-client.tsis correctly untouched because it parses JSON unconditionally (await response.json()) rather than branching on media type, so it has no equivalent defect. - Before/after reproduction, real transport. I ran the two new test files against the base revision
3f26a057and against this head in the workspace. Base: 8 failures, all mixed-case (Application/JSON; charset=utf-8) assertions, with the lowercase-JSON andtext/plaincontrols passing. Head: 24/24 pass. This matches the PR description and exercises real loopback HTTP/fetch, not a mocked transport, so the linked issue's reported symptom is genuinely reproduced and fixed. - Required repository commands.
npm run lint(0 errors; only pre-existing warnings in unrelated files),npm run build,npm run test:coverage(29 files, 418 tests pass), andnpm run format:checkall pass. No tracked files were modified. - Behavior outside the reported bug is unchanged.
text/plainstill yields a string and a stringHttpError.responsewithdetailundefined; explicitresponseType(json/text/blob/arrayBuffer) branches are untouched. The widened predicate only folds case, so a mixed-case JSON response with an unparseable body now fails into the samecatch/nullpath that lowercase JSON already used — consistent, not a regression. Optional chaining still short-circuits whencontent-typeis absent.
Non-blocking notes (no action required)
- The linked issue #6074 lists the separate
CloudClientas an explicit non-goal of the report, and the second commit extends the identical fix there. This is a consistent widening of the same one-line predicate in the same package, it adds no production behavior beyond case folding, and it comes with its own real-transport tests (direct + proxy, success + error), so I do not consider it a defect. Reviewers should simply be aware the delivered scope is slightly broader than the issue's stated bounded scope. Issue #6074 currently carriesbug,priority:low, andready-for-dev. - GitHub shows the code checks on this head as
action_required(fork-PR workflows awaiting maintainer approval to run), so the CI signal for this exact head is not yet available from Actions. I substituted the repository's documented local equivalents (lint, build,test:coverage,format:check) plus the focused content-type tests, and all pass. The PR description's per-commit human-attestation wording is a process question the repository's approval-drift security scan owns at merge time, not a code defect.
No material correctness, security, compatibility, or acceptance-criterion defect found on this head.
✅ APPROVED
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
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.
HUMAN:
I want valid JSON responses to behave consistently regardless of media-type casing. I reviewed this transport patch, its real HTTP regression tests and the PR description.
AGENT:
Why
The TypeScript client treats valid mixed-case JSON media types as plain text. With
Application/JSON; charset=utf-8, successful calls return a string, and HTTP errors lose their structured response andHttpError.detail. Media type type/subtype tokens are case-insensitive (RFC9110 §8.3.1).Summary
Compare media-type casing consistently in all six existing JSON checks: HttpClient fetch and Node GET-with-body success/error, plus CloudClient direct/proxy success/error. Only classification is folded; payload casing, explicit response types and plain-text behavior stay unchanged. The CloudClient follow-up adds two production expressions and real direct/proxy regression tests. No dependency, schema, Python server or additional
+jsonmedia-type support is introduced.Issue Number
Fixes #6074
How to Test
Actual combined-source Ubuntu / Node 24 verification, with locked dependencies:
394db779fb399b2469005aaebadc9bdc7615d131. All 12 pass after the follow-up, across public direct and proxy request paths, success and error responses. Request method/path are checked as well as payload casing andHttpError.detail.npm run lint,npm run build,npm run test:coverageandnpm run format:checkpass.4dd548e72f6b360103d3bd872128a713f365a252, whose parent is the original PR head. Exact source/configuration/patch/lock and raw native author/committer/tree/message identity are checked.3f26a0570e96e92c6af0b75388e31c9bf4f23efc, then all 12 fixed and 406 client tests passed. This is a separate baseline from the new CloudClient test above.From
clients/typescript, runnpx vitest run --config vitest.config.ts src/__tests__/http-client-content-type.test.ts src/__tests__/cloud-client-content-type.test.ts, then the four full npm commands above. The tests use real loopback HTTP/fetch, with no mocked HTTP transport.The first CloudClient run passed the same focused cases, full 418-test suite and build, then stopped at test formatting. The final run uses locked Prettier 3.9.9 output; an actual TypeScript scanner confirmed all 345 test tokens were unchanged before the complete checks were rerun.
Video/Screenshots
Nonvisual client parsing fix; the linked HTTP outcomes and tests are evidence. No live Cloud service, deployed proxy, Agent Server, browser UI, model or provider call is claimed. The loopback proxy test exercises the public client envelope endpoint, not a deployed forwarding server.
Type
Notes
Both CloudClient request branches share
fetchAndParse, whose success/error classifiers areparseResponseandbuildHttpError. HttpClient's original four corrected comparisons and its tests are preserved. A complete 384-open-PR changed-path audit found two other CloudClient patches: #4999 adds conversation metadata and #5461 serializes proxy request bodies; neither changes response media-type classification.The HUMAN note above was supplied on October 18 for the original HttpClient patch at
394db779. This October 18 evening CloudClient follow-up was implemented and verified with OpenAI Codex assistance; no additional personal review of the new lines is claimed. The existing HUMAN section is preserved exactly. Current repository policy protects that section and does not require a renewed personal attestation for each commit on an existing PR.