[PR #141] [MERGED] feat: name the status on an unrecognised error and keep the body that explains it #141

New issue

Closed

opened 2026-10-31 22:01:44 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 22:01:44 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/comfy-python-sdk/pull/141
Author: @mattmillerai
Created: 9/10/2026
Status: ✅ Merged
Merged: 9/10/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-11430-http-status-code-body-excerpt

📝 Commits (4)

827574e feat: name the status on an unrecognised error and keep the body that explains it
d81da24 fix: address bot review comments
de0c69b Merge origin/main into matt/be-11430-http-status-code-body-excerpt
a707792 docs(changelog): keep this branch's entries under Unreleased after the 0.1.9 cut on main

📊 Changes

6 files changed (+447 additions, -13 deletions)

View changed files

📝 CHANGELOG.md (+35 -0)
📝 README.md (+9 -0)
📝 src/comfy_low/errors.py (+122 -7)
📝 src/comfy_low/transport.py (+34 -1)
📝 src/comfy_sdk/exceptions.py (+8 -3)
📝 tests/test_error_mapping.py (+239 -2)

📄 Description

ELI-5

When something in front of the deployment answers instead of the service — a load balancer with no healthy backend — you got ApiError(code="error", message="HTTP 503"). "error" is the class default any hand-built exception carries, so it told you nothing, and the one line that did name the cause (no healthy upstream) was thrown away with the response. Now you get code="http_503" and HTTP 503: no healthy upstream.

What changed

code for an unrecognised response is now http_<status> instead of "error". This is the last fallback in error_from_envelope, reached only after the envelope's own error.code, Router's X-Comfy-Error-Type bucket, and _CODE_BY_STATUS have all declined. _CODE_BY_STATUS is untouched — a bare 401 with no body still maps to Unauthorized, and every documented code is unaffected. "error" was indistinguishable from "nobody set a code", on the one class of failure where the SDK has nothing else to say; the status is the single fact such a response does carry. The http_ prefix is deliberate: it reads as no service verdict was reached — something in front of the API answered, and it cannot collide with a wire code or a Router bucket, none of which are spelled that way.

ApiError.body_excerpt (keyword-only, defaults to None) keeps the text that response served. Whitespace-collapsed to one line, control characters stripped, capped at 256 characters. It is set only where the body was not the error envelope — where the response has already stated its cause, a second copy of the same text helps nobody — and __str__ shows it only beside the synthetic HTTP <status> message, never spliced into a message the server actually sent.

Retry behaviour is unchanged. RetryPolicy keys on the status plus the two collectable buckets (deadline_exceeded on a 504, concurrency_limit_exceeded on a 409); http_<status> is neither, so such a 5xx remains "a completed 5xx that named no pace" and is not auto-retried. Asserted directly in test_an_unrecognised_status_is_still_not_retried_on_its_own.

http_<status> survives the layer boundary unchanged. to_sdk_error re-types a preserved Router bucket into its typed class; http_503 is not one, so it arrives as a plain ComfyError still carrying code="http_503". No code change was needed for this — it is pinned by a new test so a later widening of the re-typing table cannot silently swallow it.

Judgment calls

Characters, not bytes. The plan said "the first 256 bytes of resp.text"; the implementation caps at 256 characters of the whitespace-collapsed text, which is what the plan's own test criterion ("a 2 KB body is truncated to 256 chars") describes, and slicing raw bytes would have meant either splitting a multi-byte UTF-8 sequence or discarding the response's declared charset.
Control characters are stripped, and the C1 range with them. Not in the plan. The excerpt is server-controlled text headed for a traceback and a log line, and the neighbouring clean_request_id bounds the request id for exactly that reason; an escape sequence in an error page must not repaint the terminal reading it.
One extra raise site in the same function. parse_or_raise also raises code="invalid_response" for a success status whose body will not decode — a proxy interstitial served as a 200. That has the identical defect (the message says the body would not decode; only the body says what answered), so it carries the excerpt too. It is one argument, purely additive, and __str__ is unaffected there because that message is not the synthetic one.
resp.text is read behind a guard. It raises ResponseNotRead on a streaming response nothing has read yet — the same case the existing resp.json() already degrades on — and a failure while composing an error message must not replace the error it was describing. Covered by test_an_unread_streaming_body_degrades_instead_of_raising.
Compatibility. A caller matching exc.code == "error" to detect an unrecognised 5xx would need to match exc.code.startswith("http_") instead. Filed under Changed in the CHANGELOG rather than as a breaking change: "error" was the class default and was documented in the code as the meaningless one.
Negative-claim falsification does not apply. The change adds no deny/dead-end path and no capability-denying string; it is purely additive information on an error that was already being raised.

Verification

uv run --extra dev ruff check . — all checks passed
uv run --extra dev ruff format --check . — 51 files already formatted
uv run --extra dev mypy src — no issues, 19 source files
uv run --extra dev pytest -q — 739 passed, 4 skipped, 0 failed
python3 scripts/check_public_repo_hygiene.py — no internal-only references found
No existing test's expectation was modified; the tests diff is additions plus one import block. Nothing in the repo asserted "error" for a bare 5xx (grep over tests/ for == "error" returns only test_models_run_retry.py:1003, which builds an ApiError by hand and therefore reads the untouched class default).

Residual

ApiError.body_excerpt does not reach the comfy_sdk surface. translating() converts every protocol ApiError into a ComfyError via to_sdk_error, and ComfyError has no body_excerpt, so an integrator catching ComfyError from client.run() / client.models.run() sees code="http_503" but not the text that names the cause — str(exc) there is still the bare HTTP 503. The code half of the fix crosses the boundary; the body half does not. This was left out deliberately: the plan named comfy_low/errors.py, comfy_low/transport.py and the code carry-through only, and ComfyError's attribute surface is an explicitly-reviewed published contract (_BASE_ATTRIBUTES in tests/test_error_contract.py, spelled out precisely so that adding one is a deliberate edit). Completing it means: add body_excerpt to ComfyError.__init__ and its class defaults, forward it in to_sdk_error, add it to _BASE_ATTRIBUTES, and decide whether ComfyError.__str__ should mirror the HTTP <status> gating. The README bullet added here says explicitly that the text lives on the protocol-level exception, so the docs are not overclaiming in the meantime.
The router surface has the same defect and is untouched. comfy_sdk.router_exceptions.error_from_response — a public, exported helper with no caller inside src/ today — degrades a bare load-balancer 503 to RouterError(detail="HTTP 503") with error_type=None and the body dropped. 503 is deliberately absent from its _ERROR_TYPE_BY_STATUS (a gateway's 503 is not the router's service_unavailable), and that decision is correct and unchanged here; what is missing is the same body-excerpt keep-and-display on that path. The ticket scoped itself to the envelope fallback and says so.
Unexercised artifact: the router-sdk e2e harness. The requirement that http_<status> be read as "no server verdict" is stated against a harness that lives in another repository, at a path this worker cannot read from its sandbox. I verified the SDK-side half of that contract instead — that to_sdk_error carries http_503 through unchanged and that no re-typing or flattening happens on the way out — but I did not run, read, or otherwise confirm the harness's own startswith("http_") check, nor that its line reference is still current. If that check has moved or changed shape, this prefix may not be the one it is looking for.
Not reproduced against a live load balancer. The no healthy upstream / upstream connect error or disconnect/reset before headers bodies come from the originating investigation, not from a capture I took. The tests drive real httpx.Response objects carrying those exact bodies through the real raise site, which exercises the code path end to end, but the wire shape itself (in particular the assumption that such a response carries no X-Comfy-Error-Type) is inherited rather than re-observed.

Provenance

Authored by: agent-work loop
Verified: ruff check .: all checks passed; ruff format --check .: 51 files already formatted; mypy src: no issues in 19 source files; pytest -q: 739 passed, 4 skipped, 0 failed; scripts/check_public_repo_hygiene.py: clean
Deviations: the excerpt is bounded in characters rather than bytes (see Judgment calls); body_excerpt is not propagated to the comfy_sdk exception surface (see Residual)

Summary by CodeRabbit

Bug Fixes
- Improved handling of proxy-generated and non-standard HTTP 5xx responses.
- Error messages now preserve a sanitized, bounded excerpt of diagnostic response text.
- Unknown HTTP failures now include status-specific codes such as http_502.
- Invalid JSON and malformed error responses provide clearer context without exposing excessive or unsafe text.
- Existing retry behavior and recognized error mappings remain unchanged.
Documentation
- Added guidance for interpreting fallback HTTP error codes and accessing response-body excerpts.

_{🔄 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-python-sdk/pull/141 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/10/2026 **Status:** ✅ Merged **Merged:** 9/10/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-11430-http-status-code-body-excerpt` --- ### 📝 Commits (4) - [`827574e`](https://github.com/Comfy-Org/comfy-python-sdk/commit/827574e378b5ba8b6431c8a30030436d169c5a9a) feat: name the status on an unrecognised error and keep the body that explains it - [`d81da24`](https://github.com/Comfy-Org/comfy-python-sdk/commit/d81da247f1dbf0f5eb857645087961782dec4397) fix: address bot review comments - [`de0c69b`](https://github.com/Comfy-Org/comfy-python-sdk/commit/de0c69bc8ca866a5a596f68d3f86633c432b13d4) Merge origin/main into matt/be-11430-http-status-code-body-excerpt - [`a707792`](https://github.com/Comfy-Org/comfy-python-sdk/commit/a707792ee1ddbbce32266403e46542a79c7ffe97) docs(changelog): keep this branch's entries under Unreleased after the 0.1.9 cut on main ### 📊 Changes **6 files changed** (+447 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+35 -0) 📝 `README.md` (+9 -0) 📝 `src/comfy_low/errors.py` (+122 -7) 📝 `src/comfy_low/transport.py` (+34 -1) 📝 `src/comfy_sdk/exceptions.py` (+8 -3) 📝 `tests/test_error_mapping.py` (+239 -2) </details> ### 📄 Description ## ELI-5 When something in front of the deployment answers instead of the service — a load balancer with no healthy backend — you got `ApiError(code="error", message="HTTP 503")`. `"error"` is the class default any hand-built exception carries, so it told you nothing, and the one line that *did* name the cause (`no healthy upstream`) was thrown away with the response. Now you get `code="http_503"` and `HTTP 503: no healthy upstream`. ## What changed **`code` for an unrecognised response is now `http_<status>` instead of `"error"`.** This is the last fallback in `error_from_envelope`, reached only after the envelope's own `error.code`, Router's `X-Comfy-Error-Type` bucket, *and* `_CODE_BY_STATUS` have all declined. `_CODE_BY_STATUS` is untouched — a bare `401` with no body still maps to `Unauthorized`, and every documented code is unaffected. `"error"` was indistinguishable from "nobody set a code", on the one class of failure where the SDK has nothing else to say; the status is the single fact such a response does carry. The `http_` prefix is deliberate: it reads as *no service verdict was reached — something in front of the API answered*, and it cannot collide with a wire `code` or a Router bucket, none of which are spelled that way. **`ApiError.body_excerpt`** (keyword-only, defaults to `None`) keeps the text that response served. Whitespace-collapsed to one line, control characters stripped, capped at 256 characters. It is set only where the body was not the error envelope — where the response has already stated its cause, a second copy of the same text helps nobody — and `__str__` shows it only beside the synthetic `HTTP <status>` message, never spliced into a message the server actually sent. **Retry behaviour is unchanged.** `RetryPolicy` keys on the status plus the two collectable buckets (`deadline_exceeded` on a `504`, `concurrency_limit_exceeded` on a `409`); `http_<status>` is neither, so such a 5xx remains "a completed 5xx that named no pace" and is not auto-retried. Asserted directly in `test_an_unrecognised_status_is_still_not_retried_on_its_own`. **`http_<status>` survives the layer boundary unchanged.** `to_sdk_error` re-types a preserved Router bucket into its typed class; `http_503` is not one, so it arrives as a plain `ComfyError` still carrying `code="http_503"`. No code change was needed for this — it is pinned by a new test so a later widening of the re-typing table cannot silently swallow it. ## Judgment calls - **Characters, not bytes.** The plan said "the first 256 bytes of `resp.text`"; the implementation caps at 256 *characters* of the whitespace-collapsed text, which is what the plan's own test criterion ("a 2 KB body is truncated to 256 chars") describes, and slicing raw bytes would have meant either splitting a multi-byte UTF-8 sequence or discarding the response's declared charset. - **Control characters are stripped, and the C1 range with them.** Not in the plan. The excerpt is server-controlled text headed for a traceback and a log line, and the neighbouring `clean_request_id` bounds the request id for exactly that reason; an escape sequence in an error page must not repaint the terminal reading it. - **One extra raise site in the same function.** `parse_or_raise` also raises `code="invalid_response"` for a *success* status whose body will not decode — a proxy interstitial served as a `200`. That has the identical defect (the message says the body would not decode; only the body says what answered), so it carries the excerpt too. It is one argument, purely additive, and `__str__` is unaffected there because that message is not the synthetic one. - **`resp.text` is read behind a guard.** It raises `ResponseNotRead` on a streaming response nothing has read yet — the same case the existing `resp.json()` already degrades on — and a failure while composing an error message must not replace the error it was describing. Covered by `test_an_unread_streaming_body_degrades_instead_of_raising`. - **Compatibility.** A caller matching `exc.code == "error"` to detect an unrecognised 5xx would need to match `exc.code.startswith("http_")` instead. Filed under Changed in the CHANGELOG rather than as a breaking change: `"error"` was the class default and was documented in the code as the meaningless one. - **Negative-claim falsification does not apply.** The change adds no deny/dead-end path and no capability-denying string; it is purely additive information on an error that was already being raised. ## Verification - `uv run --extra dev ruff check .` — all checks passed - `uv run --extra dev ruff format --check .` — 51 files already formatted - `uv run --extra dev mypy src` — no issues, 19 source files - `uv run --extra dev pytest -q` — 739 passed, 4 skipped, 0 failed - `python3 scripts/check_public_repo_hygiene.py` — no internal-only references found - No existing test's expectation was modified; the tests diff is additions plus one import block. Nothing in the repo asserted `"error"` for a bare 5xx (`grep` over `tests/` for `== "error"` returns only `test_models_run_retry.py:1003`, which builds an `ApiError` by hand and therefore reads the untouched *class* default). ## Residual - **`ApiError.body_excerpt` does not reach the `comfy_sdk` surface.** `translating()` converts every protocol `ApiError` into a `ComfyError` via `to_sdk_error`, and `ComfyError` has no `body_excerpt`, so an integrator catching `ComfyError` from `client.run()` / `client.models.run()` sees `code="http_503"` but not the text that names the cause — `str(exc)` there is still the bare `HTTP 503`. The code half of the fix crosses the boundary; the body half does not. This was left out deliberately: the plan named `comfy_low/errors.py`, `comfy_low/transport.py` and the code carry-through only, and `ComfyError`'s attribute surface is an explicitly-reviewed published contract (`_BASE_ATTRIBUTES` in `tests/test_error_contract.py`, spelled out precisely so that adding one is a deliberate edit). Completing it means: add `body_excerpt` to `ComfyError.__init__` and its class defaults, forward it in `to_sdk_error`, add it to `_BASE_ATTRIBUTES`, and decide whether `ComfyError.__str__` should mirror the `HTTP <status>` gating. The README bullet added here says explicitly that the text lives on the protocol-level exception, so the docs are not overclaiming in the meantime. - **The router surface has the same defect and is untouched.** `comfy_sdk.router_exceptions.error_from_response` — a public, exported helper with no caller inside `src/` today — degrades a bare load-balancer `503` to `RouterError(detail="HTTP 503")` with `error_type=None` and the body dropped. `503` is deliberately absent from its `_ERROR_TYPE_BY_STATUS` (a gateway's `503` is not the router's `service_unavailable`), and that decision is correct and unchanged here; what is missing is the same body-excerpt keep-and-display on that path. The ticket scoped itself to the envelope fallback and says so. - **Unexercised artifact: the router-sdk e2e harness.** The requirement that `http_<status>` be read as "no server verdict" is stated against a harness that lives in another repository, at a path this worker cannot read from its sandbox. I verified the SDK-side half of that contract instead — that `to_sdk_error` carries `http_503` through unchanged and that no re-typing or flattening happens on the way out — but I did not run, read, or otherwise confirm the harness's own `startswith("http_")` check, nor that its line reference is still current. If that check has moved or changed shape, this prefix may not be the one it is looking for. - **Not reproduced against a live load balancer.** The `no healthy upstream` / `upstream connect error or disconnect/reset before headers` bodies come from the originating investigation, not from a capture I took. The tests drive real `httpx.Response` objects carrying those exact bodies through the real raise site, which exercises the code path end to end, but the wire shape itself (in particular the assumption that such a response carries no `X-Comfy-Error-Type`) is inherited rather than re-observed. ## Provenance - **Authored by:** agent-work loop - **Verified:** `ruff check .`: all checks passed; `ruff format --check .`: 51 files already formatted; `mypy src`: no issues in 19 source files; `pytest -q`: 739 passed, 4 skipped, 0 failed; `scripts/check_public_repo_hygiene.py`: clean - **Deviations:** the excerpt is bounded in characters rather than bytes (see Judgment calls); `body_excerpt` is not propagated to the `comfy_sdk` exception surface (see Residual)  ## Summary by CodeRabbit - **Bug Fixes** - Improved handling of proxy-generated and non-standard HTTP 5xx responses. - Error messages now preserve a sanitized, bounded excerpt of diagnostic response text. - Unknown HTTP failures now include status-specific codes such as `http_502`. - Invalid JSON and malformed error responses provide clearer context without exposing excessive or unsafe text. - Existing retry behavior and recognized error mappings remain unchanged. - **Documentation** - Added guidance for interpreting fallback HTTP error codes and accessing response-body excerpts.  --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 22:01:44 +00:00