[PR #167] [MERGED] fix(errors): sanitise and unify the Router detail[] validation summary #167

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15633-run-422-sanitise-detail

📝 Commits (2)

2096320 fix(errors): sanitise and unify the Router detail[] validation summary
9525758 merge main, and drop the detail[] reroute the panel flagged

📊 Changes

7 files changed (+377 additions, -54 deletions)

View changed files

📝 CHANGELOG.md (+9 -0)
📝 src/comfy_low/errors.py (+106 -20)
📝 src/comfy_sdk/exceptions.py (+13 -1)
📝 src/comfy_sdk/router_exceptions.py (+27 -22)
📝 tests/test_error_mapping.py (+195 -8)
📝 tests/test_models_run.py (+16 -3)
📝 tests/test_router_exceptions.py (+11 -0)

📄 Description

ELI-5

When a model call is rejected for a bad field, the server sends back a little list of what went wrong. The SDK turns that list into the human-readable error message.

Two problems: (1) it pasted the server's text straight in without cleaning it, so a message containing a newline, a hidden control byte, or an ANSI escape could scribble on your terminal or flood a log line — and a big list could make the message enormous. (2) The message you got from models.run() looked different from the one you got on the queued/submit() path for the exact same server response: run() dropped the field names, so two field required errors collapsed to a useless field required; field required, while the queued path told you which two fields.

This change cleans the text (same 256-char, single-line, no-control-chars treatment every other body-derived string already gets) and makes both paths build the message with one shared function, so one server response always reads the same way.

What changed

New comfy_low.summarise_detail() renders each detail[] entry as <loc>: <msg> and runs the joined line through clean_body_excerpt (unprintable-category reduction, whitespace collapse, _BODY_EXCERPT_LIMIT/256 cap). Both error_from_envelope (awaited run() path) and router_exceptions (queued path) now summarise the array through this one function, so a single wire body yields a single .detail. The now-redundant _summarise in router_exceptions is removed.
Bounded accumulation. The summariser stops appending once it has reached the _BODY_EXCERPT_LIMIT * _BODY_EXCERPT_WINDOW prefix clean_body_excerpt actually reads, rather than rendering a server-controlled array in full and slicing 256 characters off the end — so describing a body costs the same whether the body is small or huge.
loc members that are not path segments are dropped, not coerced. loc is typed tuple[str | int, ...] — field names and array indices — so a nested, server-controlled member is skipped instead of being str()-ed into a Python repr in the user-visible message. This applied in two places: the summariser, and ValidationErrorDetail, whose _detail_from was storing the repr and whose .location re-rendered it. .location now delegates to the one shared renderer, so the summary and .errors[i].location cannot disagree about what a field is called.
_clean docstring corrected: the list detail shape still reaches _clean through the message = _clean(raw_detail) fallback (a validation array that produced no summary), so its isinstance(value, str) guard is load-bearing — it keeps a Python list repr out of the caller's message. The old docstring claimed the shape never reaches the function, which invited deleting the guard.

Each item has a regression test; the run-vs-queued .detail parity is asserted (assert excinfo.value.detail == queued.detail).

Dropped after review: the `to_sdk_error` reroute

An earlier revision made a detail[] array select a RouterError even when the code collided with a v2 envelope bucket. That is removed, for two independent reasons:

main solved it properly. #157 merged the two exception hierarchies, so unauthorized/forbidden/insufficient_credits are one RouterError subclass each. For any response carrying real Router provenance, _class_for already resolves to a RouterError subclass — the only _BY_CODE/bucket overlaps are exactly those three, every other bucket hits _BY_ERROR_TYPE, and an unknown bucket falls back to RouterError — so the typed entries are forwarded with no help from this branch. Verified across invalid_input, the three shared buckets and an unknown future bucket: all yield a RouterError with .errors populated.
What it still did beyond that was a regression, flagged by 4 of 6 panel reviewers. detail[] is a body shape any server, proxy or gateway can send (a FastAPI RequestValidationError is exactly it), so keying the class off it let an intermediary decide which except a caller runs: a bucket-less 422 stopped raising InvalidWorkflow, the elected RouterError dropped exc.details, and a v2-vocabulary code got stamped onto .error_type. It also contradicted the rule error_from_envelope already documents, learned three times on live traffic — only a response that carries a bucket gets retyped.

The tests that pinned the reroute now pin the opposite: a bucket-less array keeps its v2 class (not isinstance(err, RouterError)), with details intact and the per-field reasons still reaching the caller through the summary.

Scope note / judgment call

The fix chose the "one summariser" option (over merely aligning the low-level format). A side effect is that the queued path's summary is now sanitised and bounded too — the same latent gap the awaited path had. This is deliberate and consistent, and it makes the run/queued .detail parity exact for every body rather than only clean ones. .errors still carries the raw typed entries; only the human-readable summary string is cleaned and field-qualified. This also changes err.message/.detail on the run() path to include the field path (body.steps: too large rather than bare too large) — the pre-existing per-field tests were updated to the corrected value, and there is a CHANGELOG entry for it.

Testing

ruff check . — clean
ruff format --check . — clean (57 files)
mypy src — no issues (21 files)
pytest — 987 passed, 9 skipped
Merged origin/main at e4773c7 (which landed #157) into the branch; the above was run on the merged tree.

Provenance

Authored by: agent-work loop
Verified: on the merged tree (merge commit 9525758): ruff check + ruff format --check + mypy src all clean; pytest 987 passed, 9 skipped. The removal of the to_sdk_error reroute was checked empirically — every Router-provenance bucket still yields a RouterError with .errors populated, and a bucket-less body keeps InvalidWorkflow with its details.
Deviations: the to_sdk_error reroute described in the original revision is removed, not shipped — subsumed by #157 and flagged as a regression by 4 of 6 panel reviewers (see the section above). The sanitise-and-unify core is unchanged. One panel finding is deferred to a follow-up rather than fixed here (below).

Residual

Deferred to a follow-up: the STRING detail/error.message forms are still passed through with only a .strip() (four sites across comfy_low.errors and router_exceptions), so a server-supplied string with control characters or ANSI escapes still reaches str(exc) unbounded. Out of scope here — this PR is scoped to the array shape, and fixing the string forms changes .detail for every non-validation error on both surfaces. Recorded for filing; the docstring no longer implies the string shape is covered.
Unexercisable artifact: the original findings came from an automated review panel whose Post review step failed twice and left them only in an expired GitHub Actions run artifact. That artifact is gone and cannot be fetched; it was not needed, as each finding was re-derived and verified against merged main.

_{🔄 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/167 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15633-run-422-sanitise-detail` --- ### 📝 Commits (2) - [`2096320`](https://github.com/Comfy-Org/comfy-python-sdk/commit/2096320b057367f1cd038c38d3787ec4f707aa44) fix(errors): sanitise and unify the Router detail[] validation summary - [`9525758`](https://github.com/Comfy-Org/comfy-python-sdk/commit/9525758506a914a54e12416feafe5a8d7c096b3d) merge main, and drop the detail[] reroute the panel flagged ### 📊 Changes **7 files changed** (+377 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `CHANGELOG.md` (+9 -0) 📝 `src/comfy_low/errors.py` (+106 -20) 📝 `src/comfy_sdk/exceptions.py` (+13 -1) 📝 `src/comfy_sdk/router_exceptions.py` (+27 -22) 📝 `tests/test_error_mapping.py` (+195 -8) 📝 `tests/test_models_run.py` (+16 -3) 📝 `tests/test_router_exceptions.py` (+11 -0) </details> ### 📄 Description ## ELI-5 When a model call is rejected for a bad field, the server sends back a little list of what went wrong. The SDK turns that list into the human-readable error message. Two problems: (1) it pasted the server's text straight in without cleaning it, so a message containing a newline, a hidden control byte, or an ANSI escape could scribble on your terminal or flood a log line — and a big list could make the message enormous. (2) The message you got from `models.run()` looked *different* from the one you got on the queued/`submit()` path for the exact same server response: `run()` dropped the field names, so two `field required` errors collapsed to a useless `field required; field required`, while the queued path told you *which two fields*. This change cleans the text (same 256-char, single-line, no-control-chars treatment every other body-derived string already gets) and makes both paths build the message with one shared function, so one server response always reads the same way. ## What changed - **New `comfy_low.summarise_detail()`** renders each `detail[]` entry as `<loc>: <msg>` and runs the joined line through `clean_body_excerpt` (unprintable-category reduction, whitespace collapse, `_BODY_EXCERPT_LIMIT`/256 cap). Both `error_from_envelope` (awaited `run()` path) and `router_exceptions` (queued path) now summarise the array through this one function, so a single wire body yields a single `.detail`. The now-redundant `_summarise` in `router_exceptions` is removed. - **Bounded accumulation.** The summariser stops appending once it has reached the `_BODY_EXCERPT_LIMIT * _BODY_EXCERPT_WINDOW` prefix `clean_body_excerpt` actually reads, rather than rendering a server-controlled array in full and slicing 256 characters off the end — so describing a body costs the same whether the body is small or huge. - **`loc` members that are not path segments are dropped, not coerced.** `loc` is typed `tuple[str | int, ...]` — field names and array indices — so a nested, server-controlled member is skipped instead of being `str()`-ed into a Python repr in the user-visible message. This applied in two places: the summariser, and `ValidationErrorDetail`, whose `_detail_from` was *storing* the repr and whose `.location` re-rendered it. `.location` now delegates to the one shared renderer, so the summary and `.errors[i].location` cannot disagree about what a field is called. - **`_clean` docstring corrected**: the list `detail` shape still reaches `_clean` through the `message = _clean(raw_detail)` fallback (a validation array that produced no summary), so its `isinstance(value, str)` guard is load-bearing — it keeps a Python list repr out of the caller's message. The old docstring claimed the shape never reaches the function, which invited deleting the guard. Each item has a regression test; the run-vs-queued `.detail` parity is asserted (`assert excinfo.value.detail == queued.detail`). ## Dropped after review: the `to_sdk_error` reroute An earlier revision made a `detail[]` array select a `RouterError` even when the code collided with a v2 envelope bucket. **That is removed**, for two independent reasons: 1. **`main` solved it properly.** #157 merged the two exception hierarchies, so `unauthorized`/`forbidden`/`insufficient_credits` are one `RouterError` subclass each. For any response carrying real Router provenance, `_class_for` already resolves to a `RouterError` subclass — the only `_BY_CODE`/bucket overlaps are exactly those three, every other bucket hits `_BY_ERROR_TYPE`, and an unknown bucket falls back to `RouterError` — so the typed entries are forwarded with no help from this branch. Verified across `invalid_input`, the three shared buckets and an unknown future bucket: all yield a `RouterError` with `.errors` populated. 2. **What it still did beyond that was a regression**, flagged by 4 of 6 panel reviewers. `detail[]` is a body shape any server, proxy or gateway can send (a FastAPI `RequestValidationError` is exactly it), so keying the class off it let an intermediary decide which `except` a caller runs: a bucket-less 422 stopped raising `InvalidWorkflow`, the elected `RouterError` dropped `exc.details`, and a v2-vocabulary code got stamped onto `.error_type`. It also contradicted the rule `error_from_envelope` already documents, learned three times on live traffic — only a response that carries a *bucket* gets retyped. The tests that pinned the reroute now pin the opposite: a bucket-less array keeps its v2 class (`not isinstance(err, RouterError)`), with `details` intact and the per-field reasons still reaching the caller through the summary. ## Scope note / judgment call The fix chose the "one summariser" option (over merely aligning the low-level format). A side effect is that the **queued** path's summary is now sanitised and bounded too — the same latent gap the awaited path had. This is deliberate and consistent, and it makes the run/queued `.detail` parity exact for *every* body rather than only clean ones. `.errors` still carries the raw typed entries; only the human-readable summary string is cleaned and field-qualified. This also changes `err.message`/`.detail` on the `run()` path to include the field path (`body.steps: too large` rather than bare `too large`) — the pre-existing per-field tests were updated to the corrected value, and there is a CHANGELOG entry for it. ## Testing - `ruff check .` — clean - `ruff format --check .` — clean (57 files) - `mypy src` — no issues (21 files) - `pytest` — **987 passed, 9 skipped** - Merged `origin/main` at `e4773c7` (which landed #157) into the branch; the above was run on the merged tree. ## Provenance - **Authored by:** agent-work loop - **Verified:** on the merged tree (merge commit `9525758`): ruff check + ruff format --check + mypy src all clean; pytest 987 passed, 9 skipped. The removal of the `to_sdk_error` reroute was checked empirically — every Router-provenance bucket still yields a `RouterError` with `.errors` populated, and a bucket-less body keeps `InvalidWorkflow` with its `details`. - **Deviations:** the `to_sdk_error` reroute described in the original revision is **removed**, not shipped — subsumed by #157 and flagged as a regression by 4 of 6 panel reviewers (see the section above). The sanitise-and-unify core is unchanged. One panel finding is deferred to a follow-up rather than fixed here (below). ## Residual - **Deferred to a follow-up:** the STRING `detail`/`error.message` forms are still passed through with only a `.strip()` (four sites across `comfy_low.errors` and `router_exceptions`), so a server-supplied string with control characters or ANSI escapes still reaches `str(exc)` unbounded. Out of scope here — this PR is scoped to the array shape, and fixing the string forms changes `.detail` for every non-validation error on both surfaces. Recorded for filing; the docstring no longer implies the string shape is covered. - **Unexercisable artifact:** the original findings came from an automated review panel whose `Post review` step failed twice and left them only in an expired GitHub Actions run artifact. That artifact is gone and cannot be fetched; it was not needed, as each finding was re-derived and verified against merged `main`. --- <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:58 +00:00