From 2096320b057367f1cd038c38d3787ec4f707aa44 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 23:00:24 +0000 Subject: [PATCH] fix(errors): sanitise and unify the Router detail[] validation summary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The models.run() 422 path built its message by joining the raw detail[] entries' bare `msg` values, bypassing the excerpt sanitiser: a server-, proxy- or provider-supplied `msg` carrying newlines, C0/C1 controls, ANSI escapes or bidi overrides reached str(exc), tracebacks and log lines verbatim and unbounded, and many long entries made the string arbitrarily large. It also disagreed with the queued path on `.detail` for one wire body — two `field required` entries collapsed to the unrecoverable `field required; field required` on run() while the queued surface named both fields. - Add comfy_low.summarise_detail(): renders each entry as `: ` and runs the joined line through clean_body_excerpt (unprintable reduction, whitespace collapse, 256-char cap). Both error_from_envelope (awaited run path) and router_exceptions (queued path) now summarise the same array through this one function, so one wire body yields one string; the redundant _summarise is removed. - to_sdk_error: a detail[] array selects a RouterError even under a bucket the v2 envelope also spells (unauthorized/forbidden/insufficient_credits) or the status-derived invalid_workflow guess a stripped X-Comfy-Error-Type falls to — the array only Router sends, so it identifies the surface. The typed entries and summary are no longer dropped, including when the body also carried error.message. - Correct _clean's docstring: the list detail shape still reaches it through the message = _clean(raw_detail) fallback, so its isinstance guard is load-bearing (it keeps a Python list repr out of the caller's message). Regression tests per item; run/queued `.detail` parity asserted. Co-Authored-By: Claude Opus 4.8 --- src/comfy_low/errors.py | 101 ++++++++++++++---- src/comfy_sdk/exceptions.py | 27 ++++- src/comfy_sdk/router_exceptions.py | 28 ++--- tests/test_error_mapping.py | 162 +++++++++++++++++++++++++++-- tests/test_models_run.py | 19 +++- 5 files changed, 282 insertions(+), 55 deletions(-) diff --git a/src/comfy_low/errors.py b/src/comfy_low/errors.py index 6e294d8..4c76a41 100644 --- a/src/comfy_low/errors.py +++ b/src/comfy_low/errors.py @@ -236,12 +236,17 @@ def _clean(value: Any) -> str | None: reads as absent rather than being coerced, so a malformed body degrades to the status-derived default instead of producing a nonsense code. - The list form is still dropped here, and that is still right for the CODE - fields: a list is not a code and stringifying one would put a Python repr - where a caller expects a wire value. It is no longer how the list ``detail`` - is *read*, though — :func:`error_from_envelope` handles that shape itself, - before it reaches this function, because the entries carry both the - per-field data and the messages that summarise it. + The list form is dropped here, and that is right for the CODE fields: a list + is not a code and stringifying one would put a Python repr where a caller + expects a wire value. It is load-bearing for ``detail`` too: a Router + validation array that produced no summary — every member a non-mapping, or + every ``msg`` missing or blank — still reaches this function through the + ``message = _clean(raw_detail)`` fallback in :func:`error_from_envelope`, so + the ``isinstance(value, str)`` guard below is what keeps a Python list repr + out of the caller's message on that path. It is *not* how a populated array + is read: :func:`error_from_envelope` runs it through :func:`summarise_detail` + before that fallback, because the entries carry both the per-field data and + the messages that summarise it. """ if not isinstance(value, str): return None @@ -249,6 +254,64 @@ def _clean(value: Any) -> str | None: return stripped or None +def _location(raw_loc: Any) -> str: + """A ``detail[]`` entry's ``loc`` rendered as a dotted path, or ``""``. + + Kept identical to + :meth:`comfy_sdk.router_exceptions.ValidationErrorDetail.location` so a + per-field failure reads the same whichever surface built the summary — + ``body.images.0`` where an integer indexes into an array. + """ + if isinstance(raw_loc, Sequence) and not isinstance(raw_loc, (str, bytes)): + return ".".join(str(part) for part in raw_loc) + return "" + + +def summarise_detail(entries: Any) -> str | None: + """A bounded, single-line, printable summary of a Router per-field validation + body, or ``None`` when it named nothing. + + ``entries`` is the raw ``detail`` array — a sequence of ``{loc, msg, ...}`` + mappings. Each entry renders as ``: `` (the dotted ``loc`` path and + the field's own message), or whichever one of the two it carries; the parts + join with ``"; "``. Non-mapping members are skipped rather than coerced: this + runs while handling a failure, and one malformed member must not cost the + caller the others. + + Including ``loc`` is what makes two ``field required`` entries read as the + two fields they name rather than collapsing to an unrecoverable ``field + required; field required``. The joined line is then reduced by + :func:`clean_body_excerpt` — the same unprintable-category reduction, + whitespace collapse and :data:`_BODY_EXCERPT_LIMIT` cap every other + server-controlled string headed for a traceback or a log line already gets. + Without it a proxy-, provider- or server-supplied ``msg`` carrying newlines, + C0/C1 controls, ANSI escapes or bidi overrides would reach ``str(exc)`` + verbatim and unbounded — the exact thing :data:`_UNPRINTABLE_CATEGORIES` + exists to prevent — and enough long entries would make the message + arbitrarily large. + + Defined on this lowest layer because both error surfaces summarise the same + array off their own response and must produce one string for one wire body: + :func:`error_from_envelope` on the awaited ``models.run`` path, + ``comfy_sdk.router_exceptions`` on the queued one. + """ + if not isinstance(entries, Sequence) or isinstance(entries, (str, bytes)): + return None + parts: list[str] = [] + for entry in entries: + if not isinstance(entry, Mapping): + continue + location = _location(entry.get("loc")) + message = _clean(entry.get("msg")) + if location and message: + parts.append(f"{location}: {message}") + elif location: + parts.append(location) + elif message: + parts.append(message) + return clean_body_excerpt("; ".join(parts)) + + def error_from_envelope( http_status: int, body: dict[str, Any] | None, @@ -289,11 +352,12 @@ def error_from_envelope( Router also spells ``detail`` two ways, and both are read here. A request-level failure sends a string, which becomes the message. A per-field model-validation failure sends an ARRAY of ``{loc, msg, type, ...}`` entries - instead; those land raw on :attr:`ApiError.validation_errors`, and their - ``msg`` values — joined with ``"; "`` — become the message. Before that the - array read as no message at all, so such a failure reached a caller as a - bare ``HTTP 422`` with the raw JSON glued on as a body excerpt, and the - per-field reasons the array exists to carry were dropped on the floor. + instead; those land raw on :attr:`ApiError.validation_errors`, and + :func:`summarise_detail` renders them — each ``: ``, sanitised and + bounded — into the message. Before that the array read as no message at all, + so such a failure reached a caller as a bare ``HTTP 422`` with the raw JSON + glued on as a body excerpt, and the per-field reasons the array exists to + carry were dropped on the floor. The precedence is: the envelope's ``code`` always wins; then, when the response identifies itself as Router's — the ``X-Comfy-Error-Type`` header @@ -362,15 +426,12 @@ def error_from_envelope( # and one malformed member must not cost the caller the others. validation_errors = tuple(entry for entry in raw_detail if isinstance(entry, Mapping)) if not message: - # The entries' own messages, joined, rather than the status-derived - # default: a body that named every failing field would otherwise - # reach a caller as a bare `HTTP 422`. - summary = "; ".join( - cleaned - for cleaned in (_clean(entry.get("msg")) for entry in validation_errors) - if cleaned - ) - message = summary or message + # The entries summarised — each `: `, sanitised and bounded + # — rather than the status-derived default: a body that named every + # failing field would otherwise reach a caller as a bare `HTTP 422`. + # `comfy_sdk.router_exceptions` summarises the same array the same way + # off the queued surface, so one wire body yields one message. + message = summarise_detail(raw_detail) or message if not message: # Router names its human-readable string `detail`, not `error.message`. message = _clean(raw_detail) diff --git a/src/comfy_sdk/exceptions.py b/src/comfy_sdk/exceptions.py index f3f9b50..485315a 100644 --- a/src/comfy_sdk/exceptions.py +++ b/src/comfy_sdk/exceptions.py @@ -240,6 +240,21 @@ def to_sdk_error(exc: ApiError) -> ComfyError: request_id=exc.request_id, ) router_cls = _router_only_class(exc.code) + if router_cls is None and exc.validation_errors: + # The body carried Router's `detail[]` ARRAY, which only Router sends, so + # it identifies the Router surface even when the bucket collides with a v2 + # envelope code (`unauthorized`/`forbidden`/`insufficient_credits`, spelled + # identically by both) or is the status-derived `invalid_workflow` guess a + # 422 whose `X-Comfy-Error-Type` was stripped falls to. Selecting a + # RouterError keeps both the typed entries and the summary; the plain + # `_BY_CODE` branch below drops `.errors`, and when the body ALSO carried + # `error.message` the summary was lost with them — `message` was already + # set, so `error_from_envelope`'s `if not message:` summary block never + # ran — leaving the per-field data reachable only through `__cause__`, + # against what the `RouterError.errors` docstring promises. + from comfy_sdk.router_exceptions import exception_for + + router_cls = exception_for(exc.code) if router_cls is not None: # Imported here for the same reason `_router_only_class` imports # `_BY_ERROR_TYPE` lazily: `router_exceptions` subclasses `ComfyError` @@ -260,11 +275,13 @@ def to_sdk_error(exc: ApiError) -> ComfyError: errors=tuple(_detail_from(entry) for entry in exc.validation_errors), ) cls = _BY_CODE.get(exc.code, ComfyError) - # No `errors=` here, deliberately: `.errors` is a `RouterError` attribute - # and none of these classes takes the argument. A validation body that - # reaches this branch — a `detail[]` under a v2 `error.code`, or under the - # status-derived guess when no bucket was sent at all — still gets the - # entries' messages, since those became `exc.message` one layer down. + # No `errors=` here, deliberately: `.errors` is a `RouterError` attribute and + # none of these classes takes the argument. Nothing typed is lost by that: a + # body that carried Router's `detail[]` array was already routed to a + # RouterError above (the `exc.validation_errors` guard), so this branch is + # reached only when the response carried no per-field entries at all — a v2 + # envelope, an intermediary's reject, or a `detail[]` whose members were none + # of them mappings — and there is nothing to forward. return cls( str(exc), code=exc.code, diff --git a/src/comfy_sdk/router_exceptions.py b/src/comfy_sdk/router_exceptions.py index ad80772..b0001f6 100644 --- a/src/comfy_sdk/router_exceptions.py +++ b/src/comfy_sdk/router_exceptions.py @@ -85,7 +85,7 @@ class docstrings below reproduce. ``tests/test_router_spec_contract.py`` reads from dataclasses import dataclass from typing import Any -from comfy_low.errors import clean_request_id +from comfy_low.errors import clean_request_id, summarise_detail from .exceptions import ComfyError @@ -626,6 +626,9 @@ def error_from_response( errors = tuple( _detail_from(entry) for entry in raw_detail if isinstance(entry, Mapping) ) + # The same summariser the awaited `models.run` path uses, so one wire + # body produces one `.detail` whichever surface built the exception. + detail = summarise_detail(raw_detail) if error_type is None: error_type = _clean(body.get("error_type")) @@ -633,7 +636,7 @@ def error_from_response( error_type = _ERROR_TYPE_BY_STATUS.get(http_status) if detail is None: - detail = _summarise(errors) or f"HTTP {http_status}" + detail = f"HTTP {http_status}" return exception_for(error_type)( detail, @@ -685,9 +688,12 @@ def error_from_completion( detail = raw_detail or None elif isinstance(raw_detail, Sequence) and not isinstance(raw_detail, (str, bytes)): errors = tuple(_detail_from(entry) for entry in raw_detail if isinstance(entry, Mapping)) + # One summariser across both surfaces (see `error_from_response`), so the + # queued path's `.detail` matches the awaited one for the same body. + detail = summarise_detail(raw_detail) if detail is None: - detail = _summarise(errors) or f"the request completed with error_type {error_type!r}" + detail = f"the request completed with error_type {error_type!r}" return exception_for(error_type)( detail, @@ -759,22 +765,6 @@ def _detail_from(entry: Mapping[str, Any]) -> ValidationErrorDetail: ) -def _summarise(errors: Sequence[ValidationErrorDetail]) -> str: - """A one-line message for a per-field failure. - - This is *in addition to* ``.errors``, never instead of it -- the entries stay - readable as data, and a caller branching on a field reads them rather than - parsing this back apart. - """ - parts: list[str] = [] - for entry in errors: - if entry.location and entry.msg: - parts.append(f"{entry.location}: {entry.msg}") - elif entry.location or entry.msg: - parts.append(entry.location or entry.msg) - return "; ".join(parts) - - __all__ = [ "ERROR_TYPE_HEADER", "REQUEST_ID_HEADER", diff --git a/tests/test_error_mapping.py b/tests/test_error_mapping.py index a1e95c8..0b69603 100644 --- a/tests/test_error_mapping.py +++ b/tests/test_error_mapping.py @@ -195,7 +195,8 @@ def test_a_router_validation_body_is_read_rather_than_coerced_into_the_message() error_type="internal_error", ) assert err.code == "internal_error" - assert err.message == "too large" + # `: `, the same rendering the queued surface uses. + assert err.message == "body.steps: too large" # The intent the old status-derived message protected, unchanged: whatever # reaches `str(exc)` is prose, never a repr of the array. assert "[" not in err.message @@ -204,17 +205,31 @@ def test_a_router_validation_body_is_read_rather_than_coerced_into_the_message() ) -def test_a_validation_body_whose_entries_state_no_message_still_degrades() -> None: - # The entries decoded, so they are carried; none of them named a reason, so - # there is nothing to say but the status. Both halves matter: a caller that - # branches on `.errors` still gets them, and the message never becomes an - # empty string. +def test_a_validation_body_whose_entries_name_nothing_still_degrades() -> None: + # The entries decoded, so they are carried; none of them named a field OR a + # reason, so there is nothing to say but the status. Both halves matter: a + # caller that branches on `.errors` still gets them, and the message never + # becomes an empty string. (An entry that names a field but no message is a + # separate case -- it surfaces the field, see the loc-only test below.) err = error_from_envelope( 422, - {"detail": [{"loc": ["body", "steps"]}, {"msg": " "}]}, + {"detail": [{"type": "missing"}, {"msg": " "}]}, error_type="invalid_input", ) assert err.message == "HTTP 422" + assert err.validation_errors == ({"type": "missing"}, {"msg": " "}) + + +def test_a_validation_entry_with_a_loc_but_no_msg_names_the_field() -> None: + # A field named with no reason still beats the bare status: the loc alone + # tells the caller which field the server rejected. A blank `msg` reads as + # absent, exactly as `_clean` treats every other whitespace-only wire string. + err = error_from_envelope( + 422, + {"detail": [{"loc": ["body", "steps"]}, {"msg": " "}]}, + error_type="invalid_input", + ) + assert err.message == "body.steps" assert err.validation_errors == ({"loc": ["body", "steps"]}, {"msg": " "}) @@ -355,7 +370,7 @@ def test_a_validation_array_that_states_a_message_drops_the_excerpt_too() -> Non error_type="invalid_input", body_excerpt=raw, ) - assert str(err) == "too large" + assert str(err) == "body.steps: too large" assert err.body_excerpt is None @@ -532,3 +547,134 @@ def test_an_undecodable_success_body_keeps_what_was_served_instead() -> None: assert str(err) == ( "Could not decode the 200 response body as JSON: proxy: gateway timeout" ) + + +# --- the per-field summary is sanitised, bounded, and one string per body --- +# +# The `detail[]` summary reaches `str(exc)`, a traceback and a log line, and its +# `msg` is server-, proxy- or provider-controlled text. It gets the same +# treatment every other body-derived string gets (`clean_body_excerpt`), and the +# awaited `models.run` path and the queued surface render it through one +# summariser so a caller's branch cannot work on one and silently not the other. + + +def test_summarise_detail_names_each_field_rather_than_collapsing() -> None: + # Two `field required` entries must not collapse to the unrecoverable + # `field required; field required`: the loc is what tells them apart. + from comfy_low.errors import summarise_detail + + summary = summarise_detail( + [ + {"loc": ["body", "a"], "msg": "field required"}, + {"loc": ["body", "b"], "msg": "field required"}, + ] + ) + assert summary == "body.a: field required; body.b: field required" + # An integer index renders like the queued surface's `.location`. + assert summarise_detail([{"loc": ["body", "images", 0], "msg": "bad"}]) == "body.images.0: bad" + # Nothing to summarise -> None, so the caller falls back to the status. + assert summarise_detail([]) is None + assert summarise_detail([1, "x"]) is None + assert summarise_detail("not a sequence of entries") is None + + +def test_a_validation_summary_is_sanitised_before_it_reaches_str() -> None: + # A newline, a C0 NUL and an ANSI escape in a provider `msg` must not reach + # the terminal reading the error verbatim -- the escape's ESC byte is the + # dangerous part and is reduced to a space, whitespace collapses to one line. + err = error_from_envelope( + 422, + {"detail": [{"loc": ["body", "steps"], "msg": "line one\nline\x1b[2Jtwo\x00three"}]}, + error_type="invalid_input", + ) + assert "\n" not in err.message + assert "\x1b" not in err.message + assert "\x00" not in err.message + assert err.message == "body.steps: line one line [2Jtwo three" + + +def test_a_validation_summary_is_capped_at_the_excerpt_limit() -> None: + # Many long entries could otherwise make the message arbitrarily large; the + # same 256-char cap the excerpt gets applies to the summary. + from comfy_low.errors import _BODY_EXCERPT_LIMIT + + err = error_from_envelope( + 422, + {"detail": [{"msg": "x" * 5000} for _ in range(10)]}, + error_type="invalid_input", + ) + assert len(err.message) == _BODY_EXCERPT_LIMIT + + +def test_a_detail_array_of_non_mappings_never_leaks_a_list_repr() -> None: + # When the array yields no summary -- every member a non-mapping -- the + # message falls through to `_clean(raw_detail)` with the LIST. `_clean`'s + # isinstance guard is load-bearing there: without it the caller's message + # would be a Python list repr. It degrades to the status instead. + err = error_from_envelope(422, {"detail": [1, "x", 2]}, error_type="invalid_input") + assert err.message == "HTTP 422" + assert "[" not in err.message + assert err.validation_errors == () + + +# --- a `detail[]` array identifies the Router surface even under a bucket the +# v2 envelope also spells, or the status-derived guess a stripped header +# falls to: the typed entries must not be dropped by the non-Router branch. + + +def test_a_validation_array_under_a_colliding_bucket_stays_a_router_error() -> None: + import comfy_sdk.router_exceptions as rx + + low = error_from_envelope( + 401, + {"detail": [{"loc": ["body", "key"], "msg": "field required"}]}, + error_type="unauthorized", + ) + err = to_sdk_error(low) + # A `detail[]` array only Router sends, so it resolves the collision. + assert isinstance(err, rx.Unauthorized) + assert isinstance(err, rx.RouterError) + assert not isinstance(err, SdkUnauthorized) + assert [e.msg for e in err.errors] == ["field required"] + assert err.errors[0].location == "body.key" + + +def test_a_stripped_422_header_validation_array_still_carries_its_entries() -> None: + import comfy_sdk.router_exceptions as rx + + # No `error_type`: the 422 falls to the status-derived `invalid_workflow` + # guess, which is not a Router bucket -- but the array is still Router's. + low = error_from_envelope(422, {"detail": [{"loc": ["body", "steps"], "msg": "too large"}]}) + assert low.code == "invalid_workflow" + err = to_sdk_error(low) + assert isinstance(err, rx.RouterError) + assert [e.msg for e in err.errors] == ["too large"] + + +def test_a_colliding_bucket_with_an_envelope_message_keeps_the_entries() -> None: + import comfy_sdk.router_exceptions as rx + + # The exact body test_an_envelope_message_outranks_the_validation_entries + # builds: an `error.message` AND a `detail[]` array. The message being set + # means the summary block never ran, so before the fix both the summary and + # `.errors` were lost -- the per-field data reachable only through + # `__cause__`, against what the RouterError.errors docstring promises. + low = error_from_envelope( + 422, + { + "error": {"code": "invalid_workflow", "message": "the graph is invalid"}, + "detail": [{"loc": ["body", "steps"], "msg": "too large"}], + }, + ) + err = to_sdk_error(low) + assert isinstance(err, rx.RouterError) + assert [e.msg for e in err.errors] == ["too large"] + # The envelope's own message still wins as the human-readable detail. + assert err.detail == "the graph is invalid" + + +def test_a_colliding_bucket_without_an_array_still_keeps_its_v2_class() -> None: + # The array is the discriminator, not the code: a bare `unauthorized` with no + # `detail[]` stays the v2 class every jobs-surface handler catches. + err = to_sdk_error(ApiError("no", code="unauthorized", http_status=401)) + assert isinstance(err, SdkUnauthorized) diff --git a/tests/test_models_run.py b/tests/test_models_run.py index df5fe86..8fc9fd6 100644 --- a/tests/test_models_run.py +++ b/tests/test_models_run.py @@ -581,9 +581,13 @@ def _assert_validation_surface(exc: InvalidInput) -> None: # and which a caller reads to say what the limit actually was. assert exc.errors[0].ctx == {"limit_value": 8} assert exc.errors[0].input == 50 - # The human-readable line is the entries' own messages, joined -- not the - # `HTTP 422` a caller used to get, and not a Python repr of the array. - assert exc.detail == "ensure this value is less than or equal to 8; unknown model variant" + # The human-readable line is the entries summarised -- each `: `, + # the same rendering the queued surface produces -- not the `HTTP 422` a + # caller used to get, and not a Python repr of the array. + assert exc.detail == ( + "body.steps: ensure this value is less than or equal to 8; " + "body.model: unknown model variant" + ) assert str(exc) == exc.detail assert "[" not in exc.detail assert exc.error_type == "invalid_input" @@ -650,6 +654,15 @@ def test_the_run_and_queued_paths_agree_on_one_validation_body(server) -> None: ) assert queued is not None assert excinfo.value.errors == queued.errors + # ...and the human-readable line, too: both surfaces summarise the array + # through one function, so a body that names two fields reads the same on + # `run()` and on the queued surface rather than collapsing to a bare join of + # the messages on one of them. + assert excinfo.value.detail == queued.detail + assert excinfo.value.detail == ( + "body.steps: ensure this value is less than or equal to 8; " + "body.model: unknown model variant" + ) # --- the key survives the failure ----------------------------------------