Skip to content

fix: surface a clear error on non-JSON API responses instead of crashing #1093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ralphstodomingo wants to merge 2 commits into
base: main
Choose a base branch
from
+33 −2
Open

fix: surface a clear error on non-JSON API responses instead of crashing #1093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b090fd4
fix: surface a clear error on non-JSON API responses instead of crashing
Aug 12, 2026
15a452a
fix: preserve the original parse error as the Error cause
Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/sdk/js/src/gen/client/client.gen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,29 @@ export const createClient = (config: Config = {}): Client => {
case "arrayBuffer":
case "blob":
case "formData":
case "json":
case "text":
data = await response[parseAs]()
break
// altimate_change start — upstream_fix: guard JSON parse against non-JSON (HTML) response bodies
// "json" is split out of the fall-through group above so its parse can be guarded: a 200
// whose body is an HTML error page from a proxy/gateway/CDN otherwise crashes with a raw
// "JSON Parse error: Unrecognized token '<'". The body is read OUTSIDE the guard so a
// network/body-read failure (socket reset, abort) keeps its own error; only an actual
// JSON syntax failure gets the actionable message (mirrors the v2 client).
case "json": {
const text = await response.text()
try {
data = text ? JSON.parse(text) : {}

@sahrizvi sahrizvi Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} 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 },
)
}
break
}
// altimate_change end
case "stream":
return opts.responseStyle === "data"
? response.body
Expand Down
14 changes: 13 additions & 1 deletion packages/sdk/js/src/v2/gen/client/client.gen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,19 @@ export const createClient = (config: Config = {}): Client => {
// Some servers return 200 with no Content-Length and empty body.
// response.json() would throw; read as text and parse if non-empty.
const text = await response.text()
data = text ? JSON.parse(text) : {}
// 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) : {}

@sahrizvi sahrizvi Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} 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 },
)
Comment on lines +178 to +182

@sahrizvi sahrizvi Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
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.

}
// altimate_change end
Comment on lines +172 to +184

@sahrizvi sahrizvi Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. The correct pattern is twelve lines below the generation call. build.ts:43-59 re-applies the SseFn codegen 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). The replace assertion catches a template change; the test catches someone removing the patch step.

  2. The markers don't protect these files. script/upstream/analyze.ts:707-721 excludes 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_change marker to land inside src/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.

break
}
case "stream":
Expand Down
Loading