fix(errors): sanitise and unify the Router detail[] validation summary

CHANGELOG.md

@@ -28,6 +28,15 @@ the fuller account of each version, including verification notes.
 - `models.run` now populates `RouterError.errors` from a Router 422's per-field `detail[]` and
   uses the entries' messages as `detail`, instead of `HTTP 422`; `comfy_low.ApiError.validation_errors`
   carries the raw entries.
+- **A Router `detail[]` summary now names its fields, and is sanitised.** Both the awaited
+  (`models.run`) and the queued (`submit`) paths build the human-readable string with one shared
+  function, so a single server response reads the same way whichever surface raised it. Each entry
+  renders as `<loc>: <msg>`, so two `field required` errors now read `body.seed: field required;
+  body.steps: field required` rather than collapsing to an unrecoverable `field required; field
+  required`. The joined line gets the same treatment every other body-derived string already gets:
+  control characters, ANSI escapes and bidi overrides reduced, whitespace collapsed, and a 256-character
+  cap — so a hostile or merely careless `msg` can no longer scribble on a terminal or flood a log line.
+  Only the summary string changes; `.errors` still carries the raw typed entries.
 
 ### Changed
 

src/comfy_low/errors.py

@@ -263,19 +263,107 @@ 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
     stripped = value.strip()
     return stripped or None
 
 
+def _location(raw_loc: Any) -> str:
+    """A ``detail[]`` entry's ``loc`` rendered as a dotted path, or ``""``.
+
+    :meth:`comfy_sdk.router_exceptions.ValidationErrorDetail.location` delegates
+    here rather than restating the join, so a per-field failure reads the same
+    whichever surface built the summary — ``body.images.0`` where an integer
+    indexes into an array — and the two cannot drift apart.
+    """
+    if isinstance(raw_loc, Sequence) and not isinstance(raw_loc, (str, bytes)):
+        # Only the scalar member types a `loc` path is made of. `str(part)` on
+        # anything else would put a Python repr into the caller's message — a
+        # server-controlled `{"loc": [["a", "b"]]}` rendering as `['a', 'b']:
+        # ...` — which is the same leak `_clean`'s isinstance guard exists to
+        # stop, and `clean_body_excerpt` does not strip brackets. `bool` is
+        # excluded despite being an `int`: `True` is not an index or a field
+        # name, and rendering one as a path segment would be nonsense.
+        parts = [
+            str(part)
+            for part in raw_loc
+            if isinstance(part, str) or (isinstance(part, int) and not isinstance(part, bool))
+        ]
+        return ".".join(parts)
+    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 ``<loc>: <msg>`` (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
+    # Stop accumulating once there is provably enough to fill the capped result,
+    # rather than rendering a server-controlled array in full and slicing 256
+    # characters off the end. `clean_body_excerpt` already reads only its first
+    # `_BODY_EXCERPT_LIMIT * _BODY_EXCERPT_WINDOW` characters, so anything past
+    # that budget cannot reach the output — collecting it would only make the
+    # cost of describing a body scale with the body, which is the property
+    # `_BODY_EXCERPT_WINDOW` exists to bound.
+    budget = _BODY_EXCERPT_LIMIT * _BODY_EXCERPT_WINDOW
+    parts: list[str] = []
+    size = 0
+    for entry in entries:
+        if not isinstance(entry, Mapping):
+            continue
+        location = _location(entry.get("loc"))
+        message = _clean(entry.get("msg"))
+        if location and message:
+            part = f"{location}: {message}"
+        elif location or message:
+            part = location or message or ""
+        else:
+            continue
+        parts.append(part)
+        size += len(part) + 2  # the "; " this part will join on
+        if size >= budget:
+            break
+    return clean_body_excerpt("; ".join(parts))
+
+
 #: The status a cancel refusal is answered with.
 _CANCEL_REFUSAL_STATUS = 409
 
@@ -351,11 +439,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 ``<loc>: <msg>``, 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
@@ -431,15 +520,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 `<loc>: <msg>`, 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)

src/comfy_sdk/exceptions.py

@@ -238,7 +238,19 @@ def to_sdk_error(exc: ApiError) -> ComfyError:
     # and none of the remaining 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.
+    # entries' summary, since `summarise_detail` made it `exc.message` one layer
+    # down. The same holds for the `queue_full` early return above.
+    #
+    # The array is deliberately NOT used to reroute these into the Router
+    # hierarchy. `detail[]` is a body shape any server, proxy or gateway can
+    # send (a FastAPI `RequestValidationError` is exactly it), so keying the
+    # class off it would let an intermediary in front of the v2 jobs surface
+    # decide which `except` a caller runs. Provenance is the header, and
+    # `_class_for` already reads it: `error_type` is set only for a response
+    # that identified itself as Router's, and for those this branch is
+    # unreachable — every Router bucket resolves to a `RouterError` subclass,
+    # including the three both surfaces spell alike, so the entries are
+    # forwarded above.
     return cls(
         str(exc),
         code=exc.code,

src/comfy_sdk/router_exceptions.py

@@ -100,7 +100,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 _location, clean_request_id, summarise_detail
 
 from ._errors import ComfyError
 
@@ -181,8 +181,13 @@ class ValidationErrorDetail:
 
     @property
     def location(self) -> str:
-        """``loc`` as a dotted path -- ``body.images.0`` -- for display."""
-        return ".".join(str(part) for part in self.loc)
+        """``loc`` as a dotted path -- ``body.images.0`` -- for display.
+
+        The one renderer :func:`comfy_low.errors.summarise_detail` uses, shared
+        rather than restated so this property and the summary built from the
+        same array can never disagree about what a field is called.
+        """
+        return _location(self.loc)
 
 
 class RouterError(ComfyError):
@@ -709,14 +714,17 @@ 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"))
 
     if error_type is None:
         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,
@@ -768,9 +776,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,
@@ -830,7 +841,17 @@ def _detail_from(entry: Mapping[str, Any]) -> ValidationErrorDetail:
     raw_loc = entry.get("loc")
     loc: tuple[str | int, ...] = ()
     if isinstance(raw_loc, Sequence) and not isinstance(raw_loc, (str, bytes)):
-        loc = tuple(part if isinstance(part, (str, int)) else str(part) for part in raw_loc)
+        # A member that is not a path segment is DROPPED, not stringified. The
+        # field is typed as a path -- a field name or an array index -- and
+        # `str(part)` on a server-controlled nested value put a Python repr in
+        # there instead (`('body', "['a', 'b']")`), which then reached the user
+        # through `.location`. `bool` is excluded despite being an `int`: `True`
+        # is neither a field name nor an index.
+        loc = tuple(
+            part
+            for part in raw_loc
+            if isinstance(part, str) or (isinstance(part, int) and not isinstance(part, bool))
+        )
 
     msg, reason, ctx = entry.get("msg"), entry.get("type"), entry.get("ctx")
     return ValidationErrorDetail(
@@ -842,22 +863,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__ = [
     "CANCEL_REFUSALS",
     "ERROR_TYPE_HEADER",