fix: surface a clear error on non-JSON API responses instead of crashing - #1093
fix: surface a clear error on non-JSON API responses instead of crashing#1093ralphstodomingo wants to merge 2 commits into
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Verified via E2E repro (Bun)Drove the actual Before — unpatched client on The exact string from telemetry. The JavaScriptCore phrasing confirms it runs in the Bun CLI (not the Node extension), and the throw pins the crash to the JSON success-path parse in After — this PR: Control — valid JSON 200 against the patched client: parses fine ( |
1c24cce to
8249569
Compare
|
Re-verified after the review round: both hunks now wrapped in |
|
@codex review |
![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: 82495695bb
ℹ️ 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".
| case "json": | ||
| try { | ||
| data = await response.json() | ||
| } catch { |
There was a problem hiding this comment.
When a response stream fails after the headers arrive—for example, because the transfer is interrupted or the request is aborted—response.json() rejects with that network/body-read error before parsing JSON. This broad catch replaces it with a misleading proxy/non-JSON message, losing the error needed for diagnosis or retry classification. Keep body consumption outside the parse guard as the v2 implementation does, or only translate actual JSON syntax errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed empirically and fixed in b090fd4a0 — reproduced the exact failure with a raw-TCP mid-body reset (Content-Length promised, socket terminated after a partial body): the previous v1 guard swallowed the socket error and mislabeled it as the proxy/gateway message; v2 was unaffected. v1 now mirrors v2 (body read outside the guard, only JSON.parse inside): the reset case propagates The socket connection was closed unexpectedly, the HTML-body case keeps the actionable error, and the full parse-mode matrix + typecheck still pass.
When a proxy, gateway or CDN returns an HTTP 200 with an HTML body (an error or interstitial page) instead of JSON, the generated SDK client JSON-parses it and throws a raw `JSON Parse error: Unrecognized token '<'`. `parseAs` falls back to "json" whenever Content-Type is missing or unrecognized, so a non-JSON body reaches the parser. The error path was already guarded; the success path was not. Guard the JSON parse in both the v1 and v2 generated clients: on a parse failure, throw an actionable error (non-JSON response, likely a proxy/gateway error page, with HTTP status + content-type) instead of the raw parse crash. In v1, "json" is split out of the shared fall-through group so the other parse modes (arrayBuffer/blob/formData/text) keep dispatching via response[parseAs](). Both hunks are wrapped in `altimate_change start — upstream_fix:` markers, the repo convention for local deviations that should survive upstream bridge merges and eventually land upstream. Surfaced from telemetry as a recurring extension sendMessageError.
8249569 to
b090fd4
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
This PR hardens the generated JavaScript SDK clients to handle non-JSON bodies on successful (2xx) responses by converting raw JSON parse crashes into an actionable error that calls out the received Content-Type and HTTP status.
Changes:
- Add a guarded
JSON.parseon the success path for the v2 generated client when parsing JSON fromresponse.text(). - Split
"json"out of the v1 client’s fall-through parse switch so JSON parsing can be guarded without affecting otherparseAsmodes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/sdk/js/src/v2/gen/client/client.gen.ts | Wraps JSON parsing in a try/catch on 2xx responses to replace raw parse crashes with a clearer error. |
| packages/sdk/js/src/gen/client/client.gen.ts | Separates "json" parsing from the generic response[parseAs]() path to guard JSON parse failures while keeping other parse modes unchanged. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch { | ||
| throw new Error( | ||
| `Expected a JSON response but received ${response.headers.get("content-type") || "an unknown content type"} ` + | ||
| `(HTTP ${response.status}). This is usually a proxy or gateway error page, not the API.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Done in 15a452ae4 — original SyntaxError now attached via new Error(msg, { cause }), matching the SDK's error-interceptor.ts convention. E2E-verified the cause is present ([cause=SyntaxError] on the HTML-body case) and the full parse-mode + interrupted-body matrix still passes on both clients.
| } catch { | ||
| throw new Error( | ||
| `Expected a JSON response but received ${response.headers.get("content-type") || "an unknown content type"} ` + | ||
| `(HTTP ${response.status}). This is usually a proxy or gateway error page, not the API.`, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Done in 15a452ae4 — original SyntaxError now attached via new Error(msg, { cause }), matching the SDK's error-interceptor.ts convention. E2E-verified the cause is present ([cause=SyntaxError] on the HTML-body case) and the full parse-mode + interrupted-body matrix still passes on both clients.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Notes:
Previous Review Summary (commit b090fd4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit b090fd4)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Notes:
Reviewed by glm-5.2 · Input: 20.1K · Output: 3.8K · Cached: 191K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Addresses Copilot review: the guard's actionable message discarded the
underlying SyntaxError (token/position detail). Attach it via
new Error(msg, { cause }) — the SDK's existing convention
(error-interceptor.ts).
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Review summary
Verdict: request changes. One blocking issue, one coverage gap, one missing test. Details on the first three are inline.
The diagnosis is right and the implementation is careful in the places that matter. Reading the body outside the try is the non-obvious call and it's the correct one — a socket reset or abort mid-read keeps its own error identity instead of getting mislabeled a proxy error page, and the inline comment says why. { cause } preserves the original SyntaxError, lib: ["es2022"] in packages/sdk/js/tsconfig.json makes the two-arg Error constructor typecheck, and splitting "json" out of the v1 fall-through is mechanically clean — arrayBuffer/blob/formData/text still dispatch through response[parseAs]() and parseAs resolution is unchanged. The marker format is right too (balanced, upstream_fix: prefix, no redundant nesting).
Major: no regression test
packages/sdk/js has no test suite, but the SDK is exercised from packages/opencode/test/server/ — sdk-error-shape.test.ts, sdk-v1-smoke.test.ts, httpapi-sdk.test.ts all build a client with an injected fetch, which makes faking this a ten-liner:
const sdk = createOpencodeClient({
baseUrl: "http://test",
fetch: (async () =>
new Response("<!DOCTYPE html><html>502</html>", {
status: 200,
headers: { "content-type": "application/json" },
})) as unknown as typeof fetch,
})
await expect(sdk.session.list()).rejects.toThrow(/not JSON/)Two reasons this is more than a box-tick. First, a test is the only mechanism that catches the regeneration wipe. Second, the trigger is counter-intuitive: the instinct is to return Content-Type: text/html, which never reaches the guard — the test has to use application/json with an HTML body. That subtlety belongs in a committed test rather than a PR description. The E2E matrix in the description is real work; it just isn't running anywhere.
REVIEW.md is explicit that CI here covers types and marker presence, not runtime behavior.
Minor
- The throw bypasses
interceptors.errorand thethrowOnError: falsecontract. Both wrappers registerclient.interceptors.error.use(wrapClientError), and those run only on the non-ok branch. A success-paththrowskips them — including any consumer-registered telemetry hook — and escapes regardless ofthrowOnError: false, which otherwise promises a{ data, error }tuple. This is not a regression:JSON.parse(text)threw a rawSyntaxErrorfrom the identical position before, so no caller ever got a result tuple for this failure class. But this was the natural moment to route it through the normal error path, and that's also why this error class is invisible towrapClientError. causeshape diverges from the error-path convention.error-interceptor.ts:31,35,41attachescause: { body, status }; this attaches the rawSyntaxError. Defensible — different failure classes — and the inline suggestion on the message resolves it incidentally.
Nits
gen/client/client.gen.ts:132uses"content-type"; line 110 in the same function uses"Content-Type".Headers.getis case-insensitive so it works, but pick one.- Twelve byte-identical lines across the two clients. A
parseJsonOrThrow(text, request, response)helper inpackages/sdk/js/src/—error-interceptor.tsis the precedent for shared non-generated client logic — would shrink the fork delta to two one-line calls and compose cleanly with the post-gen patch. - The v1 comment runs six lines to v2's three for identical logic.
Test matrix worth committing
- 200 +
application/json+ HTML body → actionable error,causeis aSyntaxError. v1 and v2. (the shipped bug) - 200 +
text/html; charset=utf-8+ HTML body → currently returns a string asdata. - 200 + no
Content-Type+ HTML body → currently returns a stream asdata. - v1 chunked 200, empty body, no
Content-Length→ asserts the new{}rather than the old throw. - 200 +
application/json+ valid JSON, and valid JSON under a wrong content-type → guard must not fire. responseValidator/responseTransformerstill run after a successful parse (v1 regression guard).- Body-read failure mid-stream still surfaces the socket error, not the proxy message — pins the outside-the-
tryplacement against future edits. - Codegen idempotence: run
packages/sdk/js/script/build.ts, assert the guard survives.
| // altimate_change start — upstream_fix: guard JSON parse against non-JSON (HTML) response bodies | ||
| // A 200 whose body is an HTML error page from a proxy/gateway/CDN otherwise crashes with a | ||
| // raw "JSON Parse error: Unrecognized token '<'". Surface an actionable error instead. | ||
| try { | ||
| data = text ? JSON.parse(text) : {} | ||
| } catch (cause) { | ||
| throw new Error( | ||
| `Expected a JSON response but received ${response.headers.get("content-type") || "an unknown content type"} ` + | ||
| `(HTTP ${response.status}). This is usually a proxy or gateway error page, not the API.`, | ||
| { cause }, | ||
| ) | ||
| } | ||
| // altimate_change end |
There was a problem hiding this comment.
Blocking: this hunk is deleted by the release build, so it never ships.
packages/sdk/js/script/build.ts:16-22 regenerates this whole tree:
await createClient({
input: "./openapi.json",
output: { path: "./src/v2/gen", tsConfigPath: ..., clean: true },
...
})clean: true wipes src/v2/gen and regenerates client/client.gen.ts from the @hey-api/client-fetch template — no guard, no markers — and nothing re-applies it afterwards.
This isn't "if someone runs generate". script/publish.ts:19-28 calls ./packages/sdk/js/script/build.ts inside prepareReleaseFiles(), which runs on every release, before the CLI and SDK are packed. So the published @opencode-ai/sdk/v2 — and the altimate binary that bundles it — ships without this fix, and the crash in the telemetry keeps firing.
Two things worth flagging:
-
The correct pattern is twelve lines below the generation call.
build.ts:43-59re-applies theSseFncodegen patch post-generation and throws if the needle stops matching. That's exactly what this needs:const v2ClientPath = "./src/v2/gen/client/client.gen.ts" const v2ClientSource = await Bun.file(v2ClientPath).text() const needle = "data = text ? JSON.parse(text) : {}" const v2ClientPatched = v2ClientSource.replace(needle, guardedBlock) if (v2ClientPatched === v2ClientSource) { throw new Error(`json-guard patch did not apply; @hey-api/client-fetch output may have changed (${v2ClientPath})`) } await Bun.write(v2ClientPath, v2ClientPatched)
Pair it with a codegen-idempotence check (run
build.ts, assert the guard is still there). Thereplaceassertion catches a template change; the test catches someone removing the patch step. -
The markers don't protect these files.
script/upstream/analyze.ts:707-721excludes both gen trees from marker checks outright:const markerExcludePatterns = [ ..., "packages/sdk/js/src/gen/**", "packages/sdk/js/src/v2/gen/**", ... ]
So the PR description's rationale — markers here mean the bridge-merge process sees and carries them — doesn't hold for these two paths. The marker format is right; the file is the problem. This is the first
altimate_changemarker to land insidesrc/v2/gen.
Note the asymmetry the description presents as equivalence: src/gen (v1) isn't regenerated by build.ts (only prettier --write), so the v1 hunk survives — by accident of v1 being a frozen snapshot, not because of the markers. Worth saying so in the v1 comment.
| // A 200 whose body is an HTML error page from a proxy/gateway/CDN otherwise crashes with a | ||
| // raw "JSON Parse error: Unrecognized token '<'". Surface an actionable error instead. | ||
| try { | ||
| data = text ? JSON.parse(text) : {} |
There was a problem hiding this comment.
The guard only fires when parseAs resolves to "json", which leaves the common proxy shape untouched.
Both clients default to parseAs: "auto", so getParseAs() picks the arm (utils.gen.ts:61-90, v1 twin at :59-88):
| Response content-type | resolves to | with an HTML body, after this PR |
|---|---|---|
application/json (proxy lies) |
json |
✅ fixed |
unrecognized, e.g. foo/bar (?? "json") |
json |
✅ fixed |
text/html / text/html; charset=utf-8 |
text |
❌ HTML returned as a string in data, no error |
| absent | stream |
❌ response.body returned as data, no error |
application/octet-stream |
blob |
❌ Blob returned as data, no error |
Driven against a local server, both clients resolve rather than reject for text/html, text/html; charset=utf-8, and no Content-Type, under both throwOnError settings.
So this covers only the mislabeled-as-JSON case. A gateway that labels its error page honestly — most of them — still returns a "successful" result whose data is an HTML string, and fails further downstream with a worse message than the one this replaces.
Also, the description says parseAs falls back to "json" when Content-Type is missing. It doesn't — a missing Content-Type resolves to "stream" (utils.gen.ts:62-66).
The cheapest way to close most of this is one line in code this PR doesn't touch. packages/sdk/js/src/v2/client.ts:84-89 already guards this exact failure:
if (contentType === "text/html")
throw new Error("Request is not supported by this version of OpenCode Server (Server responded with text/html)")Exact equality misses text/html; charset=utf-8 — the form proxies and CDNs actually send. Normalizing it covers strictly more cases than this hunk does:
if (contentType?.split(";")[0]?.trim().toLowerCase() === "text/html")(v1 has no such interceptor at all, so v1 has neither layer.) Pre-existing and outside the diff, raised only because it's load-bearing for the gap above and is a one-liner.
| throw new Error( | ||
| `Expected a JSON response but received ${response.headers.get("content-type") || "an unknown content type"} ` + | ||
| `(HTTP ${response.status}). This is usually a proxy or gateway error page, not the API.`, | ||
| { cause }, | ||
| ) |
There was a problem hiding this comment.
The message names the content-type, which in the only case that fires is application/json.
Because the guard runs only when parseAs resolved to "json", what users actually see is:
Expected a JSON response but received application/json (HTTP 200).
That's the string in the PR's own verification table, and it reads as self-contradictory — the content-type is the one field that isn't discriminating here.
The more concrete loss is that the error carries no request identity. packages/sdk/js/src/error-interceptor.ts (describe()) deliberately puts method + URL + status into every wrapped client error so formatters and telemetry have something traceable. A telemetry event carrying this message can't be traced to an endpoint or a host, and request is in scope at both sites.
Keeping the body out of the message is right — embedding a gateway page risks logging something sensitive — but it can live on cause for anyone debugging:
| throw new Error( | |
| `Expected a JSON response but received ${response.headers.get("content-type") || "an unknown content type"} ` + | |
| `(HTTP ${response.status}). This is usually a proxy or gateway error page, not the API.`, | |
| { cause }, | |
| ) | |
| throw new Error( | |
| `Expected a JSON response from ${request.method} ${request.url} but the body was not JSON ` + | |
| `(HTTP ${response.status}, content-type ${response.headers.get("content-type") ?? "unset"}). ` + | |
| `This is usually a proxy or gateway error page, not the API.`, | |
| { cause: { parseError: cause, status: response.status, body: text.slice(0, 200) } }, | |
| ) |
Same applies to the v1 copy at gen/client/client.gen.ts:131-135.
| case "json": { | ||
| const text = await response.text() | ||
| try { | ||
| data = text ? JSON.parse(text) : {} |
There was a problem hiding this comment.
Undeclared v1 behavior change: an empty body used to throw, now returns {}.
v1's case "json" was await response.json(), which throws SyntaxError: Unexpected end of JSON input on an empty body. text ? JSON.parse(text) : {} returns {} instead.
The early return above only covers status === 204 and Content-Length === "0" (client.gen.ts:100-107), so a chunked 200 with an empty body and no Content-Length reaches this switch and now silently yields {}.
Aligning v1 with v2 is probably the right call, but it's outside the stated scope and the PR body's matrix says v1 already returned {} before — it didn't. One knock-on: responseValidator (client.gen.ts:150) now runs against {} for empty bodies where it was previously never reached.
Either call it out in the description or split it into its own commit.
Closes #1119
What
When a proxy / gateway / CDN returns an HTTP 200 with an HTML body (an error or interstitial page) instead of JSON, the generated SDK client crashes with a raw
JSON Parse error: Unrecognized token '<'.parseAsfalls back to"json"(?? "json") wheneverContent-Typeis missing or unrecognized, so a non-JSON body reaches the parser. The error response path was already guarded; the success path was not.Fix
Guard the JSON parse in the success path of both generated clients. On a parse failure, throw an actionable error naming the received content-type + HTTP status ("…usually a proxy or gateway error page, not the API") instead of the raw parse crash.
packages/sdk/js/src/v2/gen/client/client.gen.ts— the client the CLI imports (@opencode-ai/sdk/v2)packages/sdk/js/src/gen/client/client.gen.ts— v1:jsonis split out of the shared fall-through group soarrayBuffer/blob/formData/textkeep dispatching viaresponse[parseAs]()Both hunks are wrapped in
altimate_change start — upstream_fix:markers — the repo convention for local deviations from upstream, so the bridge-merge process sees and carries them, and they can be retired if/when the fix lands upstream.Verification (E2E under Bun, full parse-mode matrix)
Drove each actual client file against a local server. 7 cases × v1/v2 × before/after:
json+ HTML body (JSON content-type)SyntaxError: JSON Parse error: Unrecognized token '<'(v2) /Failed to parse JSON(v1)Expected a JSON response but received application/json (HTTP 200). This is usually a proxy or gateway error page, not the API.json+ valid JSONjson+ empty body{}{}(unchanged)parseAs: blobBlobBlob(unchanged)parseAs: arrayBufferArrayBufferArrayBuffer(unchanged)parseAs: textparseAs: formData(real multipart)FormDatafield=valueFormDatafield=value (unchanged)The v2 "before" error is the exact string seen in telemetry (JavaScriptCore phrasing → confirms the crash runs in the Bun CLI, not the Node extension).
packages/sdk/jstypecheck (tsgo --noEmit) passes.Where it came from
Surfaced by the extension telemetry-triage bot as a recurring
ChatPanel:chat:sendMessageError(~11 machines / 7d).🤖 Generated with Claude Code
https://claude.ai/code/session_01LKJeLDMhBaYu16LrjGCf25