[PR #139] [MERGED] fix(models): return a Router binary 200 as bytes instead of throwing unexpected_response #141

New issue

Closed

opened 2026-10-31 22:04:59 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:04:59 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/139
Author: @mattmillerai
Created: 9/11/2026
Status: ✅ Merged
Merged: 9/14/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-13347-models-run-binary-result

📝 Commits (2)

ade4930 fix(models): return a Router binary 200 as bytes instead of throwing
fe4566b fix(models): require a 200 with a body, and read the media type properly

📊 Changes

6 files changed (+606 additions, -35 deletions)

View changed files

📝 CHANGELOG.md (+38 -0)
📝 README.md (+45 -7)
📝 src/sdk/index.ts (+3 -0)
📝 src/sdk/models.test.ts (+314 -9)
📝 src/sdk/models.ts (+188 -13)
📝 test/support/router-stub-server.ts (+18 -6)

📄 Description

ELI-5

Some partner models hand back a file — ElevenLabs answers with the MP3 itself, not a link to one. This SDK read every successful response as text and ran JSON.parse on it, so those calls blew up with "body is not JSON" after the server had already generated and billed the audio, and the text decode shredded the bytes on the way so you couldn't even rescue them from the error. Now run looks at the response's Content-Type first: JSON still comes back exactly as before, and a file comes back as the raw bytes you can write straight to disk.

What changed

comfy.models.run's success path used to be "read the whole body with response.text(), JSON.parse it, done". The run route's 200 has had two branches in the contract for a while — application/json, and */* with format: binary — and the ElevenLabs audio models (elevenlabs/eleven_v3, elevenlabs/eleven_sfx_v2) are the first of the second kind in the catalog, so every model whose result is a file was unusable from this SDK.

The body is buffered as bytes, not text (response.arrayBuffer() instead of response.text()), inside the same try as before so the deadline still covers body consumption. Decoding is deferred to the one branch that wants a string, which is what text() did internally anyway. This is the half that makes the bytes recoverable at all: the old lossy UTF-8 decode ran before anything looked at the Content-Type.
finish branches on Content-Type. application/json or a +json suffix → today's result. Anything else → the bytes. A 2xx with no Content-Type at all is parsed as JSON if it parses, and is binary with contentType: "" otherwise — that fallback is deliberate, so a server that omits the header on a JSON body is not silently regressed into bytes.
RunResult is now a discriminated union: RunJsonResult (kind: "json", data the parsed document) and RunBinaryResult (kind: "binary", data a Uint8Array of the exact bytes, contentType the partner's own media type). Both are exported, as is CONTENT_TYPE_HEADER. The bytes are handed back verbatim — not base64, not wrapped in a JSON-shaped object.
Unchanged, and tested to stay that way: the 202 guard, the deadline / AbortSignal handling, the retry loop and the Retry-After collect loop. A binary 200 collected across a paced 409 under a replayed Idempotency-Key resolves identically to a first run. A non-2xx is still the typed error it always was, never bytes. A 2xx that says application/json and then does not parse still raises unexpected_response.

README.md documents both shapes with a worked audio example (writeFile and new Blob([data], { type: contentType })), and there is a CHANGELOG.md entry under [Unreleased].

Breaking change (types only)

The runtime shape of a JSON result gains only kind, so existing JavaScript keeps working unchanged. Existing TypeScript needs a narrowing — if (result.kind === "json") — before data is the supplied TData again, because it is otherwise TData | Uint8Array. Pre-1.0 library, and the behaviour being replaced is a thrown error rather than a contract anyone could rely on.

Judgment calls

Accept broadened from application/json to application/json, */*;q=0.9. This is the riskiest line in the diff, so: git log -S traces the header to the commit that first introduced the hand-bound run route, with no content-negotiation rationale recorded for the exclusivity, and the route's own contract declares a */* 200. JSON stays ranked at q=1, so a server that negotiates still picks JSON wherever a JSON representation exists; the two values can only disagree where the server has only a non-JSON representation, which is exactly the case being fixed. Left as-is, this client would go on telling a binary-only model it rejects the only thing that model can produce — a 406 waiting for the day anything in front of the route honours the header.
text/json and other non-standard JSON spellings are treated as bytes. The contract declares application/json, and the response carries X-Content-Type-Options: nosniff, so the declared type is taken at its word rather than guessed at from the body. Widening the JSON set on a hunch would re-introduce a smaller version of the bug being fixed.
The whole body is buffered in memory. Streaming is explicitly out of scope for this change; the server buffers too.

Scope sweep

Because "which other responses are secretly binary" is the question this bug is really about, both vendored contracts were swept rather than only the route being fixed:

Vendored Router contract: 4 operations, 1 non-application/json response branch — runRouterModel's 200 */*. That is the one this PR fixes; nothing is left over.
Vendored Comfy API v2 contract: 12 operations, 3 non-JSON 2xx branches — getAssetContent 200/206 (application/octet-stream) and getJobEvents 200 (text/event-stream). All three already bypass the JSON parse in parseOrRaise on their success path (src/low/transport.ts:441-444 returns the raw Response for a 200/206 and only enters parseOrRaise on a non-2xx; SSE has its own reader), so 0 remain mis-parsed. The other JSON.parse call sites in src/ are local workflow files and SSE event payloads, neither of which is an HTTP response body.

spec/router-openapi.yaml needed no re-sync: it was parsed and structurally compared against the canonical contract it is vendored from, and the two are identical, */* format: binary branch and X-Content-Type-Options: nosniff header included. pnpm check:spec-drift is green.

Residual

Not fixed or not exercised by this PR, written to stand on its own:

No live call was made against the elevenlabs/eleven_v3 model on the deployed Router. This environment holds no Router credential, so every assertion here is against the repository's own stub server, which was extended to answer with raw bytes and with an omitted Content-Type. The fixture is an ID3v2.4 header plus an MPEG frame sync word (49 44 33 04 … ff fb), i.e. the documented head of an audio/mpeg body including bytes that are not valid UTF-8 — but a stub is not the deployment. Someone with a credential should run comfy.models.run("elevenlabs/eleven_v3", { text: …, output_format: "mp3_44100_128" }) against the deployed route once and confirm the result is kind: "binary" with playable bytes. The published per-model contract for that model was read directly and does declare the 200 as */* / format: binary ("Raw audio bytes… forwarded from ElevenLabs"), so the shape this PR implements is the shape the server documents.
elevenlabs/eleven_sfx_v2 was not exercised at all. It is the second binary model in the catalog and is named in the same report, but it is reported to answer 422 from a server-side binding defect that is not in this repository and is tracked separately. Nothing in this PR is model-specific — the branch is on Content-Type, not on the model ID — so it should work the moment that server-side defect lands, but that is a prediction, not a measurement.
The originating report is a stack from the Python client, not this one. The defect here was identified by reading this repository's code for the same unconditional parse, and it is unambiguously present (JSON.parse on every 2xx body, no Content-Type read anywhere on the success path) — but no one has reproduced this SDK's failure against the deployed route. The sibling SDKs in other languages carry the same defect and are out of scope here.
A 2xx with no Content-Type and an empty body now resolves as a zero-byte binary result instead of raising. That follows from the prescribed no-Content-Type rule and is unreachable against the current contract, which declares no 204 on this route — but a zero-byte "generation" handed to a caller as success is a worse failure than an error if a deployment ever produces one, and a byteLength === 0 guard would be a reasonable follow-up.
Streaming the binary body is not implemented. The whole response is buffered in memory. That was out of scope by design, and is the obvious next piece of work for a large video result.

Provenance

Authored by: agent-work loop
Verified: pnpm test: 486 passed, 0 failed (24 files), including 11 new binary-result cases; pnpm typecheck: clean; pnpm lint (oxlint): clean; pnpm format:check: clean; pnpm build: clean; pnpm check:spec-drift: both checks OK; node scripts/check-public-repo-hygiene.mjs: OK. Vendored Router spec parsed and deep-compared against the canonical contract: identical.
Deviations: none of the acceptance criteria were skipped; the spec re-sync criterion was satisfied by verifying the vendored copy is already current rather than by producing a diff. Live verification against the deployed Router was not possible from this environment — see ## Residual.

Summary by CodeRabbit

New Features
- comfy.models.run now supports both JSON and binary responses.
- Binary responses preserve raw bytes and their content type.
- Results identify their format through a kind field.
- Added exported result types and a content-type header constant.
Documentation
- Added guidance for handling binary media, JSON parsing errors, buffering, and Node.js and browser usage.
Bug Fixes
- Improved response handling for missing or parameterized content types and JSON-compatible media types.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/comfy-typescript-sdk/pull/139 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/11/2026 **Status:** ✅ Merged **Merged:** 9/14/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-13347-models-run-binary-result` --- ### 📝 Commits (2) - [`ade4930`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/ade49305c90fcfecd8af8d30ba4deb99160fdc83) fix(models): return a Router binary 200 as bytes instead of throwing - [`fe4566b`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/fe4566bb8f624f912682a0999cfb7eb5860c05e3) fix(models): require a 200 with a body, and read the media type properly ### 📊 Changes **6 files changed** (+606 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+38 -0) 📝 `README.md` (+45 -7) 📝 `src/sdk/index.ts` (+3 -0) 📝 `src/sdk/models.test.ts` (+314 -9) 📝 `src/sdk/models.ts` (+188 -13) 📝 `test/support/router-stub-server.ts` (+18 -6) </details> ### 📄 Description ## ELI-5 Some partner models hand back a file — ElevenLabs answers with the MP3 itself, not a link to one. This SDK read *every* successful response as text and ran `JSON.parse` on it, so those calls blew up with "body is not JSON" **after** the server had already generated and billed the audio, and the text decode shredded the bytes on the way so you couldn't even rescue them from the error. Now `run` looks at the response's `Content-Type` first: JSON still comes back exactly as before, and a file comes back as the raw bytes you can write straight to disk. ## What changed `comfy.models.run`'s success path used to be "read the whole body with `response.text()`, `JSON.parse` it, done". The run route's `200` has had two branches in the contract for a while — `application/json`, and `*/*` with `format: binary` — and the ElevenLabs audio models (`elevenlabs/eleven_v3`, `elevenlabs/eleven_sfx_v2`) are the first of the second kind in the catalog, so every model whose result is a file was unusable from this SDK. - **The body is buffered as bytes, not text** (`response.arrayBuffer()` instead of `response.text()`), inside the same `try` as before so the deadline still covers body consumption. Decoding is deferred to the one branch that wants a string, which is what `text()` did internally anyway. This is the half that makes the bytes recoverable at all: the old lossy UTF-8 decode ran *before* anything looked at the `Content-Type`. - **`finish` branches on `Content-Type`.** `application/json` or a `+json` suffix → today's result. Anything else → the bytes. A `2xx` with no `Content-Type` at all is parsed as JSON if it parses, and is binary with `contentType: ""` otherwise — that fallback is deliberate, so a server that omits the header on a JSON body is not silently regressed into bytes. - **`RunResult` is now a discriminated union**: `RunJsonResult` (`kind: "json"`, `data` the parsed document) and `RunBinaryResult` (`kind: "binary"`, `data` a `Uint8Array` of the exact bytes, `contentType` the partner's own media type). Both are exported, as is `CONTENT_TYPE_HEADER`. The bytes are handed back verbatim — not base64, not wrapped in a JSON-shaped object. - **Unchanged, and tested to stay that way:** the `202` guard, the deadline / `AbortSignal` handling, the retry loop and the `Retry-After` collect loop. A binary `200` collected across a paced `409` under a replayed `Idempotency-Key` resolves identically to a first run. A non-2xx is still the typed error it always was, never bytes. A `2xx` that says `application/json` and then does not parse still raises `unexpected_response`. `README.md` documents both shapes with a worked audio example (`writeFile` and `new Blob([data], { type: contentType })`), and there is a `CHANGELOG.md` entry under `[Unreleased]`. ## Breaking change (types only) The runtime shape of a JSON result gains only `kind`, so existing JavaScript keeps working unchanged. Existing **TypeScript** needs a narrowing — `if (result.kind === "json")` — before `data` is the supplied `TData` again, because it is otherwise `TData | Uint8Array`. Pre-1.0 library, and the behaviour being replaced is a thrown error rather than a contract anyone could rely on. ## Judgment calls - **`Accept` broadened from `application/json` to `application/json, */*;q=0.9`.** This is the riskiest line in the diff, so: `git log -S` traces the header to the commit that first introduced the hand-bound run route, with no content-negotiation rationale recorded for the exclusivity, and the route's own contract declares a `*/*` `200`. JSON stays ranked at `q=1`, so a server that negotiates still picks JSON wherever a JSON representation exists; the two values can only disagree where the server has *only* a non-JSON representation, which is exactly the case being fixed. Left as-is, this client would go on telling a binary-only model it rejects the only thing that model can produce — a `406` waiting for the day anything in front of the route honours the header. - **`text/json` and other non-standard JSON spellings are treated as bytes.** The contract declares `application/json`, and the response carries `X-Content-Type-Options: nosniff`, so the declared type is taken at its word rather than guessed at from the body. Widening the JSON set on a hunch would re-introduce a smaller version of the bug being fixed. - **The whole body is buffered in memory.** Streaming is explicitly out of scope for this change; the server buffers too. ## Scope sweep Because "which other responses are secretly binary" is the question this bug is really about, both vendored contracts were swept rather than only the route being fixed: - Vendored Router contract: **4 operations, 1 non-`application/json` response branch** — `runRouterModel`'s `200` `*/*`. That is the one this PR fixes; nothing is left over. - Vendored Comfy API v2 contract: **12 operations, 3 non-JSON `2xx` branches** — `getAssetContent` `200`/`206` (`application/octet-stream`) and `getJobEvents` `200` (`text/event-stream`). All three already bypass the JSON parse in `parseOrRaise` on their success path (`src/low/transport.ts:441-444` returns the raw `Response` for a `200`/`206` and only enters `parseOrRaise` on a non-2xx; SSE has its own reader), so **0** remain mis-parsed. The other `JSON.parse` call sites in `src/` are local workflow files and SSE event payloads, neither of which is an HTTP response body. `spec/router-openapi.yaml` needed no re-sync: it was parsed and structurally compared against the canonical contract it is vendored from, and the two are identical, `*/*` `format: binary` branch and `X-Content-Type-Options: nosniff` header included. `pnpm check:spec-drift` is green. ## Residual Not fixed or not exercised by this PR, written to stand on its own: - **No live call was made against the `elevenlabs/eleven_v3` model on the deployed Router.** This environment holds no Router credential, so every assertion here is against the repository's own stub server, which was extended to answer with raw bytes and with an omitted `Content-Type`. The fixture is an ID3v2.4 header plus an MPEG frame sync word (`49 44 33 04 … ff fb`), i.e. the documented head of an `audio/mpeg` body including bytes that are not valid UTF-8 — but a stub is not the deployment. Someone with a credential should run `comfy.models.run("elevenlabs/eleven_v3", { text: …, output_format: "mp3_44100_128" })` against the deployed route once and confirm the result is `kind: "binary"` with playable bytes. The published per-model contract for that model *was* read directly and does declare the `200` as `*/*` / `format: binary` ("Raw audio bytes… forwarded from ElevenLabs"), so the shape this PR implements is the shape the server documents. - **`elevenlabs/eleven_sfx_v2` was not exercised at all.** It is the second binary model in the catalog and is named in the same report, but it is reported to answer `422` from a server-side binding defect that is not in this repository and is tracked separately. Nothing in this PR is model-specific — the branch is on `Content-Type`, not on the model ID — so it should work the moment that server-side defect lands, but that is a prediction, not a measurement. - **The originating report is a stack from the Python client, not this one.** The defect here was identified by reading this repository's code for the same unconditional parse, and it is unambiguously present (`JSON.parse` on every `2xx` body, no `Content-Type` read anywhere on the success path) — but no one has reproduced *this* SDK's failure against the deployed route. The sibling SDKs in other languages carry the same defect and are out of scope here. - **A `2xx` with no `Content-Type` and an empty body now resolves as a zero-byte binary result instead of raising.** That follows from the prescribed no-`Content-Type` rule and is unreachable against the current contract, which declares no `204` on this route — but a zero-byte "generation" handed to a caller as success is a worse failure than an error if a deployment ever produces one, and a `byteLength === 0` guard would be a reasonable follow-up. - **Streaming the binary body is not implemented.** The whole response is buffered in memory. That was out of scope by design, and is the obvious next piece of work for a large video result. ## Provenance - **Authored by:** agent-work loop - **Verified:** `pnpm test`: 486 passed, 0 failed (24 files), including 11 new binary-result cases; `pnpm typecheck`: clean; `pnpm lint` (oxlint): clean; `pnpm format:check`: clean; `pnpm build`: clean; `pnpm check:spec-drift`: both checks OK; `node scripts/check-public-repo-hygiene.mjs`: OK. Vendored Router spec parsed and deep-compared against the canonical contract: identical. - **Deviations:** none of the acceptance criteria were skipped; the spec re-sync criterion was satisfied by verifying the vendored copy is already current rather than by producing a diff. Live verification against the deployed Router was not possible from this environment — see `## Residual`.  ## Summary by CodeRabbit - **New Features** - `comfy.models.run` now supports both JSON and binary responses. - Binary responses preserve raw bytes and their content type. - Results identify their format through a `kind` field. - Added exported result types and a content-type header constant. - **Documentation** - Added guidance for handling binary media, JSON parsing errors, buffering, and Node.js and browser usage. - **Bug Fixes** - Improved response handling for missing or parameterized content types and JSON-compatible media types.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-31 22:04:59 +00:00

bsakaguchi referenced this issue

2026-10-31 22:04:59 +00:00

[PR #141] [MERGED] ci: bump Comfy-Org/github-workflows/.github/workflows/cursor-review.yml from ce203b3861f5b6b6bc00bbfd43710188ccaa8b6f to ebfd9e44588ffc560a063d0d13cf8a9a91d1ffb0 #142

bsakaguchi closed this issue

2026-10-31 22:04:59 +00:00

bsakaguchi referenced this issue

2026-10-31 22:05:06 +00:00

[PR #153] ci(cursor-review): re-sync both pins to 425c154 and stop Dependabot splitting them #155