fix(workflow): surface unparseable cloud 200 body as an error instead of empty/success (BE-3334)

comfy_cli/command/workflow.py

@@ -453,6 +453,12 @@ class _ResponseTooLarge(Exception):
 }
 
 
+class _ResponseUnparseable(Exception):
+    """A non-empty 200 body could not be decoded as JSON — surface it as a loud
+    error instead of a misleading empty/success result (an empty body is still a
+    legitimate ``None`` and is *not* this)."""
+
+
 # Map the cloud ``--sort`` fields onto local FileInfo keys (client-side sort;
 # ComfyUI's /userdata listing has no server-side sort/limit/filter).
 _LOCAL_SORT_KEYS = {"create_time": "created", "update_time": "modified", "name": "path"}
@@ -638,27 +644,44 @@ def _http_request(
     if not raw:
         return status, None
     try:
-        return status, json.loads(raw)
-    except (json.JSONDecodeError, UnicodeDecodeError):
-        # UnicodeDecodeError is a ValueError but *not* a JSONDecodeError, so a
-        # body that isn't valid UTF-8 needs naming here or it escapes uncaught.
-        return status, None
+        return status, json.loads(raw.decode("utf-8"))
+    except (ValueError, RecursionError) as e:
+        # A non-empty body that won't decode as JSON is a *malformed* response, not
+        # "no data": returning ``None`` here would let callers report an empty list
+        # or a null id as success. Raise instead so it surfaces as a loud, mapped
+        # error. Decode as UTF-8 *explicitly* first: handed raw bytes, ``json.loads``
+        # auto-detects UTF-16/32 (RFC 4627) and would silently accept a non-UTF-8 body
+        # the contract treats as malformed. Non-UTF-8 bytes -> ``UnicodeDecodeError``;
+        # valid-UTF-8 non-JSON text -> ``JSONDecodeError``; both subclass ``ValueError``.
+        # Catch the ``ValueError`` base to also map the parser's *other* rejections that
+        # aren't ``JSONDecodeError`` — e.g. a JSON integer past CPython's 4300-digit
+        # int/str limit raises a bare ``ValueError`` — plus ``RecursionError`` from
+        # pathologically nested input (the 64 MiB cap permits deep nesting). Otherwise
+        # those escape the mapping and crash the CLI with a raw traceback.
+        raise _ResponseUnparseable() from e
 
 
 def _handle_cloud_http_error(renderer, e, *, operation: str, workflow_id: str | None = None) -> typer.Exit:
     """Map HTTP failures to envelope codes. Returns an Exit to ``raise from``."""
     import urllib.error
 
-    if isinstance(e, _ResponseTooLarge):
+    if isinstance(e, _ResponseUnparseable):
+        renderer.error(
+            code="workflow_unparseable",
+            message=f"cloud returned a non-empty but unparseable (non-JSON) response during {operation}",
+            hint="the server sent a malformed body; retry, and report it if it persists",
+            details={"operation": operation, "workflow_id": workflow_id},
+        )
+    elif isinstance(e, _ResponseTooLarge):
         renderer.error(
             code="workflow_too_large",
             message=f"cloud API response during {operation} exceeded the {_HTTP_MAX_BYTES // (1024 * 1024)} MiB cap",
             hint=_TOO_LARGE_HINTS.get(operation, "the cloud response was unexpectedly large"),
             details={"operation": operation, "workflow_id": workflow_id, "limit_bytes": _HTTP_MAX_BYTES},
         )
     elif isinstance(e, urllib.error.HTTPError):
-        # Bound the read itself; slicing after an unbounded read would still
-        # have pulled an arbitrarily large error body into memory first.
+        # Cap the read itself — ``[:1000]`` after a full ``read()`` would still pull an
+        # arbitrarily large (or malicious) error page into memory before slicing.
         body = (e.read(1000) or b"").decode("utf-8", "replace")
         if e.code == 404:
             renderer.error(
@@ -966,10 +989,42 @@ def list_cmd(
 
     try:
         _, body = _http_request(url, target)
-    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge) as e:
+    except (
+        urllib.error.HTTPError,
+        urllib.error.URLError,
+        OSError,
+        _ResponseUnparseable,
+        _ResponseTooLarge,
+    ) as e:
         raise _handle_cloud_http_error(renderer, e, operation="list") from e
 
-    rows = (body or {}).get("data") or []
+    # A non-empty body decodes here only if it was valid JSON; guard the shape the
+    # way ``get``/``save`` do. An empty body is a legitimate ``None`` (→ no rows), but
+    # a valid-JSON *non-dict* 200 (an array like ``[1, 2, 3]`` or a scalar) is malformed:
+    # ``(body).get("data")`` would raise a raw ``AttributeError``, and coercing it to an
+    # empty list would masquerade the malformed shape as a genuinely-empty listing.
+    if body is not None and not isinstance(body, dict):
+        renderer.error(
+            code="cloud_http_error",
+            message="unexpected response shape from /api/workflows (expected a JSON object)",
+            details={"got_type": type(body).__name__},
+        )
+        raise typer.Exit(code=1)
+
+    # A missing/empty ``data`` is a legitimately-empty listing, but a present non-list
+    # ``data`` is malformed the same way a non-dict body is: a scalar (``{"data": 42}``)
+    # would raise a raw ``TypeError`` in the comprehension below, and a str/dict would
+    # iterate silently and masquerade as an empty listing. Reject it with the same envelope.
+    rows = (body or {}).get("data")
+    if rows is None:
+        rows = []
+    elif not isinstance(rows, list):
+        renderer.error(
+            code="cloud_http_error",
+            message="unexpected response shape from /api/workflows (data must be a JSON array)",
+            details={"got_type": type(rows).__name__},
+        )
+        raise typer.Exit(code=1)
     payload = {
         "count": len(rows),
         "workflows": [
@@ -1037,7 +1092,13 @@ def get_cmd(
     url = target.url("workflows", _up.quote(workflow_id, safe=""), "content")
     try:
         _, body = _http_request(url, target)
-    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge) as e:
+    except (
+        urllib.error.HTTPError,
+        urllib.error.URLError,
+        OSError,
+        _ResponseUnparseable,
+        _ResponseTooLarge,
+    ) as e:
         raise _handle_cloud_http_error(renderer, e, operation="get", workflow_id=workflow_id) from e
 
     if not isinstance(body, dict) or "workflow_json" not in body:
@@ -1132,7 +1193,13 @@ def save_cmd(
     url = target.url("workflows")
     try:
         _, resp = _http_request(url, target, method="POST", body=body)
-    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge) as e:
+    except (
+        urllib.error.HTTPError,
+        urllib.error.URLError,
+        OSError,
+        _ResponseUnparseable,
+        _ResponseTooLarge,
+    ) as e:
         raise _handle_cloud_http_error(renderer, e, operation="save") from e
 
     workflow_id = (resp or {}).get("id") if isinstance(resp, dict) else None
@@ -1168,7 +1235,13 @@ def delete_cmd(
     url = target.url("workflows", _up.quote(workflow_id, safe=""))
     try:
         _, _body = _http_request(url, target, method="DELETE")
-    except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseTooLarge) as e:
+    except (
+        urllib.error.HTTPError,
+        urllib.error.URLError,
+        OSError,
+        _ResponseUnparseable,
+        _ResponseTooLarge,
+    ) as e:
         raise _handle_cloud_http_error(renderer, e, operation="delete", workflow_id=workflow_id) from e
 
     payload = {"workflow_id": workflow_id, "deleted": True}

comfy_cli/error_codes.py

@@ -80,6 +80,14 @@ class ErrorCode:
         "truncate it into a corrupt/partial file. `details.limit_bytes` carries the cap.",
         "the saved workflow is unexpectedly large; inspect it directly on the server",
     ),
+    ErrorCode(
+        "workflow_unparseable",
+        "A cloud `/api/workflows` call returned a non-empty 200 body that couldn't be decoded as JSON "
+        "(non-UTF-8 bytes or a non-JSON body such as an HTML proxy/error page). Distinct from an empty "
+        "body (legitimately no data): the malformed body is surfaced as a hard error rather than a "
+        "misleading empty list / null id. `details.operation` carries the verb.",
+        "the server sent a malformed body; retry, and report it if it persists",
+    ),
     ErrorCode(
         "workflow_content_not_json",
         "`workflow get` fetched content that isn't parseable JSON (non-UTF-8 bytes or a non-JSON body such "

tests/comfy_cli/command/test_workflow_saved.py

@@ -502,12 +502,15 @@ def test_empty_body_still_returns_none(self, cloud_target, monkeypatch):
         target = workflow_cmd._resolve_where_target("cloud")
         assert workflow_cmd._http_request(target.url("workflows"), target) == (200, None)
 
-    def test_non_utf8_body_returns_none_rather_than_raising(self, cloud_target, monkeypatch):
-        # UnicodeDecodeError is a ValueError but not a JSONDecodeError; if it is
-        # not caught it escapes _http_request and no call site handles it.
+    def test_non_utf8_body_raises_unparseable(self, cloud_target, monkeypatch):
+        # UnicodeDecodeError is a ValueError but not a JSONDecodeError. A non-empty
+        # body that won't decode must not masquerade as "no data" (200, None); it is
+        # malformed and must raise _ResponseUnparseable so a call site maps it to a
+        # loud envelope rather than reporting empty success.
         _patch_urlopen(monkeypatch, {"/api/workflows": (b"\xff\xfe\x00not json", 200)})
         target = workflow_cmd._resolve_where_target("cloud")
-        assert workflow_cmd._http_request(target.url("workflows"), target) == (200, None)
+        with pytest.raises(workflow_cmd._ResponseUnparseable):
+            workflow_cmd._http_request(target.url("workflows"), target)
 
     def test_non_utf8_body_surfaces_envelope_not_traceback(self, cloud_target, monkeypatch, capsys):
         # End-to-end: the undecodable body must reach the user as an envelope.
@@ -610,3 +613,102 @@ def test_404_surfaces_workflow_not_found(self, cloud_target, monkeypatch, capsys
         env = _run(["delete", "ghost", "--where", "cloud"], capsys)
         assert env["ok"] is False
         assert env["error"]["code"] == "workflow_not_found"
+
+
+# ---------------------------------------------------------------------------
+# unparseable 200 body — must surface a loud error, not empty/success (BE-3334)
+# ---------------------------------------------------------------------------
+
+# A non-empty 200 body the client can't decode as JSON. Four shapes, all malformed:
+#   - a valid-UTF-8 non-JSON page (e.g. an HTML proxy/error page returned 200) -> JSONDecodeError;
+#   - a non-UTF-8 byte string that isn't valid UTF-8 at all -> UnicodeDecodeError;
+#   - valid JSON encoded as UTF-16, which the *contract* treats as malformed. ``json.loads``
+#     auto-detects UTF-16/32 byte input (RFC 4627), so handed raw bytes it would silently
+#     accept this; decoding as UTF-8 first surfaces it as UnicodeDecodeError. Locks that in.
+#   - a JSON integer past CPython's 4300-digit int/str conversion limit: ``json.loads`` raises
+#     a *bare* ``ValueError`` (not ``JSONDecodeError``), which the narrow catch would have let
+#     escape as a raw traceback. Locks in the broadened ``ValueError``-base catch.
+# ``JSONDecodeError`` and ``UnicodeDecodeError`` both subclass ``ValueError``; catching the base
+# maps all four. None may be reported as "no data" (empty list / null id).
+_UNPARSEABLE_BODIES = [
+    pytest.param(b"<html>502 Bad Gateway</html>", id="non-json"),
+    pytest.param(b"\xff\xfe\x00bad", id="non-utf8"),
+    pytest.param(json.dumps({"data": [], "id": "x"}).encode("utf-16"), id="utf16-json"),
+    pytest.param(b"1" + b"0" * 4400, id="bigint-valueerror"),
+]
+
+
+class TestUnparseableResponse:
+    @pytest.mark.parametrize("raw", _UNPARSEABLE_BODIES)
+    def test_list_surfaces_error_not_empty(self, cloud_target, monkeypatch, capsys, raw):
+        _patch_urlopen(monkeypatch, {"/api/workflows": (raw, 200)})
+        env = _run(["list", "--where", "cloud"], capsys)
+        # Must NOT masquerade as a successful, genuinely-empty list.
+        assert env["ok"] is False
+        assert env["error"]["code"] == "workflow_unparseable"
+        assert env["error"]["details"]["operation"] == "list"
+
+    @pytest.mark.parametrize("raw", _UNPARSEABLE_BODIES)
+    def test_save_surfaces_error_not_null_id(self, cloud_target, tmp_path, monkeypatch, capsys, raw):
+        wf_path = tmp_path / "wf.json"
+        wf_path.write_text(json.dumps({"1": {"class_type": "KSampler", "inputs": {}}}))
+        _patch_urlopen(monkeypatch, {"/api/workflows": (raw, 200)})
+        env = _run(["save", str(wf_path), "--name", "x", "--where", "cloud"], capsys)
+        # Must NOT claim success with a null workflow_id.
+        assert env["ok"] is False
+        assert env["error"]["code"] == "workflow_unparseable"
+        assert env["error"]["details"]["operation"] == "save"
+
+    @pytest.mark.parametrize("raw", _UNPARSEABLE_BODIES)
+    def test_get_surfaces_error_not_missing_content(self, cloud_target, monkeypatch, capsys, raw):
+        _patch_urlopen(monkeypatch, {"/api/workflows/wf-uuid/content": (raw, 200)})
+        env = _run(["get", "wf-uuid", "--where", "cloud"], capsys)
+        # Must NOT be reported as missing/invalid content — it's a malformed transport body.
+        assert env["ok"] is False
+        assert env["error"]["code"] == "workflow_unparseable"
+        assert env["error"]["details"]["operation"] == "get"
+
+    @pytest.mark.parametrize("raw", _UNPARSEABLE_BODIES)
+    def test_delete_surfaces_error_not_success(self, cloud_target, monkeypatch, capsys, raw):
+        _patch_urlopen(monkeypatch, {"/api/workflows/wf-uuid": (raw, 200)})
+        env = _run(["delete", "wf-uuid", "--where", "cloud"], capsys)
+        # Must NOT claim a successful delete off an unparseable body.
+        assert env["ok"] is False
+        assert env["error"]["code"] == "workflow_unparseable"
+        assert env["error"]["details"]["operation"] == "delete"
+
+
+class TestListMalformedShape:
+    """A valid-JSON but wrongly-shaped 200 body reaches ``list`` (``get``/``save`` already
+    guard with ``isinstance``). It must map to an error, not raise a raw ``AttributeError``
+    and not coerce to a masquerading empty list."""
+
+    @pytest.mark.parametrize("raw", [pytest.param(b"[1, 2, 3]", id="array"), pytest.param(b"42", id="scalar")])
+    def test_list_non_dict_body_surfaces_error(self, cloud_target, monkeypatch, capsys, raw):
+        _patch_urlopen(monkeypatch, {"/api/workflows": (raw, 200)})
+        env = _run(["list", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    @pytest.mark.parametrize(
+        "raw",
+        [
+            pytest.param(b'{"data": 42}', id="scalar"),
+            pytest.param(b'{"data": "oops"}', id="string"),
+            pytest.param(b'{"data": {"a": 1}}', id="object"),
+        ],
+    )
+    def test_list_non_list_data_surfaces_error(self, cloud_target, monkeypatch, capsys, raw):
+        # A dict body with a non-list ``data`` must not raise a raw TypeError (scalar)
+        # nor iterate silently into a masquerading empty listing (str/dict).
+        _patch_urlopen(monkeypatch, {"/api/workflows": (raw, 200)})
+        env = _run(["list", "--where", "cloud"], capsys)
+        assert env["ok"] is False
+        assert env["error"]["code"] == "cloud_http_error"
+
+    def test_list_missing_data_is_empty(self, cloud_target, monkeypatch, capsys):
+        # A dict body with no ``data`` key is a legitimately-empty listing, not an error.
+        _patch_urlopen(monkeypatch, {"/api/workflows": (b"{}", 200)})
+        env = _run(["list", "--where", "cloud"], capsys)
+        assert env["ok"] is True
+        assert env["data"]["count"] == 0