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". |
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