[PR #205] fix(errors): keep the message of a bare {message} error body #204

New issue

Open

opened 2026-10-31 22:05:39 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-typescript-sdk/pull/205
Author: @mattmillerai
Created: 10/9/2026
Status: 🔄 Open

Base: main ← Head: be-19945-bare-message-fallback

📝 Commits (3)

afbcbb2 fix(errors): keep the message of a bare {message} error body
c0af5f4 fix(errors): guard the BY_CODE lookup and treat a non-string envelope message as absent
4d2c7b7 fix(errors): leave the BY_CODE own-property guard to #196

📊 Changes

5 files changed (+82 additions, -1 deletions)

View changed files

📝 CHANGELOG.md (+1 -0)
📝 src/low/errors.test.ts (+48 -0)
📝 src/low/errors.ts (+12 -1)
📝 src/sdk/client.test.ts (+12 -0)
📝 test/support/stub-server.ts (+9 -0)

📄 Description

ELI-5

When the server rejects a job before it reaches the API's own error handling (for example, a body that isn't valid JSON, a workflow that isn't an object, or a workflow over the size limit), the web framework answers with a plain {"message": "..."} body instead of the usual {"error": {...}} envelope. The SDK only looked inside error, so the useful explanation was thrown away and you just saw HTTP 400. Now the SDK keeps that message.

Summary

src/low/errors.ts: ErrorEnvelopeBody gains an optional top-level message (documented as echo's HTTPError shape, not part of the ErrorEnvelope contract). In errorFromEnvelope, an empty or non-string envelope message is treated as absent, and then a non-empty string body.message is used. The code stays status-derived (CODE_BY_STATUS[status] ?? "error"), details stays null, and a non-empty envelope message still wins when a body carries both. With no usable message on either side, the result is HTTP <status> (a non-string envelope message used to be stringified, e.g. [object Object]).
test/support/stub-server.ts: a new jobBareError: { status, message } | null scenario makes POST /jobs answer with a bare {message} body.
Tests: unit cases in src/low/errors.test.ts for the decoder 400, the 413, envelope-wins-over-top-level, ignoring a non-string or empty message at either level (42, "", {x:1}), and an empty envelope message falling back to the top-level one. There's also an end-to-end submit() case in src/sdk/client.test.ts against the stub: it rejects with a ComfyError, httpStatus 400, code "error" and the server's message.
CHANGELOG.md: one Fixed entry.

CODE_BY_STATUS, BY_CODE and the exported classes are unchanged, and every existing envelope test passes unmodified.

Judgment calls

The fallback is inserted right after the details read, with the existing blank line left in place before if (!code). That keeps it off the lines the open #196 rewrites. git merge-tree against #196's head merges src/low/errors.ts cleanly. CHANGELOG.md and src/low/errors.test.ts do conflict, but only because both PRs append at the same spot, so whichever lands second keeps both sides.
The fallback uses its own typeof === "string" guard on the envelope message, so it behaves the same whether #196 lands before it or after.
One consequence of "no usable envelope message": an envelope whose error.message is empty or a non-string and that also has a non-empty top-level string message now takes the top-level one. Before this change it got HTTP <status> (or the stringified non-string).
The prototype-safe BY_CODE lookup a reviewer asked for is left to #196, which makes exactly that change on the same lines. Duplicating it here would make src/low/errors.ts conflict.

Provenance

Authored by: agent-work loop
Verified:
- Tests: all green, 796 passed / 5 skipped (vitest, local Node)
- Lint, format check, typecheck and spec drift: clean
- Mutation check: with the errors.ts change reverted, the two bare-body unit cases and the submit() stub case fail
- Merge check against open #196: src/low/errors.ts merges cleanly; append-only conflicts in CHANGELOG.md and src/low/errors.test.ts
- Public-repo hygiene: clean (run with the previously vendored copy of the script; CI now uses the shared reusable workflow)
- Not run locally: Node 22/24 CI matrix, sdk-parity (network), build-check
Deviations: none. The ticket's acceptance names npm test; the repo runs pnpm test.

Residual

The servers' behaviour wasn't exercised against a live service. The {message} shapes for the decoder 400, header-bind 400, BodyLimit 413 and gateway 500 rewrite come from a prior read-only investigation. Here they're only reproduced through the stub server.
The spec documentation of this bare shape lives in a separate, still-open spec-sync PR and isn't on main yet. This change doesn't depend on it.
A bare-body 400 still gets the generic code: "error". Mapping 400 to a named code was deliberately left out because another in-flight change owns the CODE_BY_STATUS table.

Summary by CodeRabbit

Bug Fixes
- Error responses without a standard error envelope now retain a top-level message, making the server’s explanation available in the resulting error.
- The HTTP status continues to determine the error code. When an error envelope contains a non-empty message, that message remains preferred over the top-level message.

_{🔄 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/205 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `be-19945-bare-message-fallback` --- ### 📝 Commits (3) - [`afbcbb2`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/afbcbb2f03b4e08c71efcd62f60e0f1212314ca0) fix(errors): keep the message of a bare {message} error body - [`c0af5f4`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/c0af5f4dfa230d2217abc103f838982463b2aae1) fix(errors): guard the BY_CODE lookup and treat a non-string envelope message as absent - [`4d2c7b7`](https://github.com/Comfy-Org/comfy-typescript-sdk/commit/4d2c7b7855e671fdb6c5dc3f9d549eed5d3c183c) fix(errors): leave the BY_CODE own-property guard to #196 ### 📊 Changes **5 files changed** (+82 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+1 -0) 📝 `src/low/errors.test.ts` (+48 -0) 📝 `src/low/errors.ts` (+12 -1) 📝 `src/sdk/client.test.ts` (+12 -0) 📝 `test/support/stub-server.ts` (+9 -0) </details> ### 📄 Description ## ELI-5 When the server rejects a job before it reaches the API's own error handling (for example, a body that isn't valid JSON, a `workflow` that isn't an object, or a workflow over the size limit), the web framework answers with a plain `{"message": "..."}` body instead of the usual `{"error": {...}}` envelope. The SDK only looked inside `error`, so the useful explanation was thrown away and you just saw `HTTP 400`. Now the SDK keeps that message. ## Summary - `src/low/errors.ts`: `ErrorEnvelopeBody` gains an optional top-level `message` (documented as echo's `HTTPError` shape, not part of the ErrorEnvelope contract). In `errorFromEnvelope`, an empty or non-string envelope message is treated as absent, and then a non-empty string `body.message` is used. The code stays status-derived (`CODE_BY_STATUS[status] ?? "error"`), `details` stays `null`, and a non-empty envelope message still wins when a body carries both. With no usable message on either side, the result is `HTTP <status>` (a non-string envelope message used to be stringified, e.g. `[object Object]`). - `test/support/stub-server.ts`: a new `jobBareError: { status, message } | null` scenario makes `POST /jobs` answer with a bare `{message}` body. - Tests: unit cases in `src/low/errors.test.ts` for the decoder 400, the 413, envelope-wins-over-top-level, ignoring a non-string or empty `message` at either level (`42`, `""`, `{x:1}`), and an empty envelope message falling back to the top-level one. There's also an end-to-end `submit()` case in `src/sdk/client.test.ts` against the stub: it rejects with a `ComfyError`, `httpStatus 400`, `code "error"` and the server's message. - `CHANGELOG.md`: one Fixed entry. `CODE_BY_STATUS`, `BY_CODE` and the exported classes are unchanged, and every existing envelope test passes unmodified. ## Judgment calls - The fallback is inserted right after the `details` read, with the existing blank line left in place before `if (!code)`. That keeps it off the lines the open #196 rewrites. `git merge-tree` against #196's head merges `src/low/errors.ts` cleanly. `CHANGELOG.md` and `src/low/errors.test.ts` do conflict, but only because both PRs append at the same spot, so whichever lands second keeps both sides. - The fallback uses its own `typeof === "string"` guard on the envelope message, so it behaves the same whether #196 lands before it or after. - One consequence of "no usable envelope message": an envelope whose `error.message` is empty or a non-string and that also has a non-empty top-level string `message` now takes the top-level one. Before this change it got `HTTP <status>` (or the stringified non-string). - The prototype-safe `BY_CODE` lookup a reviewer asked for is left to #196, which makes exactly that change on the same lines. Duplicating it here would make `src/low/errors.ts` conflict. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: all green, 796 passed / 5 skipped (vitest, local Node) - Lint, format check, typecheck and spec drift: clean - Mutation check: with the `errors.ts` change reverted, the two bare-body unit cases and the `submit()` stub case fail - Merge check against open #196: `src/low/errors.ts` merges cleanly; append-only conflicts in `CHANGELOG.md` and `src/low/errors.test.ts` - Public-repo hygiene: clean (run with the previously vendored copy of the script; CI now uses the shared reusable workflow) - Not run locally: Node 22/24 CI matrix, `sdk-parity` (network), `build-check` - **Deviations:** none. The ticket's acceptance names `npm test`; the repo runs `pnpm test`. ## Residual - The servers' behaviour wasn't exercised against a live service. The `{message}` shapes for the decoder 400, header-bind 400, BodyLimit 413 and gateway 500 rewrite come from a prior read-only investigation. Here they're only reproduced through the stub server. - The spec documentation of this bare shape lives in a separate, still-open spec-sync PR and isn't on `main` yet. This change doesn't depend on it. - A bare-body 400 still gets the generic `code: "error"`. Mapping 400 to a named code was deliberately left out because another in-flight change owns the `CODE_BY_STATUS` table.  ## Summary by CodeRabbit * **Bug Fixes** * Error responses without a standard error envelope now retain a top-level message, making the server’s explanation available in the resulting error. * The HTTP status continues to determine the error code. When an error envelope contains a non-empty message, that message remains preferred over the top-level message.  --- <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:05:39 +00:00