From 9d274fa87178e4441d398a2d862b9471839126ed Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 10:07:41 -0700 Subject: [PATCH 1/3] fix(workflow): surface unparseable cloud 200 body as an error instead of empty/success (BE-3334) --- comfy_cli/command/workflow.py | 32 +++++++++++++---- comfy_cli/error_codes.py | 8 +++++ .../comfy_cli/command/test_workflow_saved.py | 34 +++++++++++++++++++ 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index d953c10e..5b37ca20 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -438,6 +438,12 @@ class _ResponseTooLarge(Exception): """A ``/userdata`` response exceeded ``_USERDATA_MAX_BYTES`` — refuse to truncate.""" +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"} @@ -619,15 +625,27 @@ def _http_request( return status, None try: return status, json.loads(raw) - except json.JSONDecodeError: - return status, None + except (json.JSONDecodeError, UnicodeDecodeError) 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. (``json.loads`` decodes bytes itself and raises ``UnicodeDecodeError`` + # — not a ``JSONDecodeError`` — on non-UTF-8 input, so catch both.) + 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, urllib.error.HTTPError): + 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, urllib.error.HTTPError): body = (e.read() or b"")[:1000].decode("utf-8", "replace") if e.code == 404: renderer.error( @@ -935,7 +953,7 @@ def list_cmd( try: _, body = _http_request(url, target) - except (urllib.error.HTTPError, urllib.error.URLError, OSError) as e: + except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseUnparseable) as e: raise _handle_cloud_http_error(renderer, e, operation="list") from e rows = (body or {}).get("data") or [] @@ -1006,7 +1024,7 @@ 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) as e: + except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseUnparseable) 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: @@ -1101,7 +1119,7 @@ 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) as e: + except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseUnparseable) 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 @@ -1137,7 +1155,7 @@ 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) as e: + except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseUnparseable) as e: raise _handle_cloud_http_error(renderer, e, operation="delete", workflow_id=workflow_id) from e payload = {"workflow_id": workflow_id, "deleted": True} diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 4cba21d3..201ca277 100644 --- a/comfy_cli/error_codes.py +++ b/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 " diff --git a/tests/comfy_cli/command/test_workflow_saved.py b/tests/comfy_cli/command/test_workflow_saved.py index 2328480b..5ccbcdf0 100644 --- a/tests/comfy_cli/command/test_workflow_saved.py +++ b/tests/comfy_cli/command/test_workflow_saved.py @@ -531,3 +531,37 @@ 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: a valid-UTF-8 non-JSON page +# (e.g. an HTML proxy/error page returned 200) and a non-UTF-8 byte string. The +# latter makes ``json.loads`` raise ``UnicodeDecodeError`` (not ``JSONDecodeError``), +# so both must be caught. Neither may be reported as "no data" (empty list / null id). +_UNPARSEABLE_BODIES = [ + pytest.param(b"502 Bad Gateway", id="non-json"), + pytest.param(b"\xff\xfe\x00bad", id="non-utf8"), +] + + +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" + + @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" From d43ec2e5f4045f79283ba67a7cc851c90a61e90f Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 10:18:35 -0700 Subject: [PATCH 2/3] fix(workflow): decode cloud bodies as UTF-8 before json.loads (BE-3334) Handed raw bytes, json.loads auto-detects UTF-16/32 (RFC 4627) and would silently accept a non-UTF-8 body the workflow_unparseable contract treats as malformed. Decode UTF-8 explicitly first so such bodies raise UnicodeDecodeError -> _ResponseUnparseable, matching the contract. Also expand TestUnparseableResponse per review: add a UTF-16-JSON regression param, assert details.operation on the envelope, and cover the get/delete catch sites (were untested). Addresses CodeRabbit review on PR #549. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/command/workflow.py | 8 +++-- .../comfy_cli/command/test_workflow_saved.py | 34 ++++++++++++++++--- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index 5b37ca20..cf1c0692 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -624,13 +624,15 @@ def _http_request( if not raw: return status, None try: - return status, json.loads(raw) + return status, json.loads(raw.decode("utf-8")) except (json.JSONDecodeError, UnicodeDecodeError) 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. (``json.loads`` decodes bytes itself and raises ``UnicodeDecodeError`` - # — not a ``JSONDecodeError`` — on non-UTF-8 input, so catch both.) + # 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``; catch both. raise _ResponseUnparseable() from e diff --git a/tests/comfy_cli/command/test_workflow_saved.py b/tests/comfy_cli/command/test_workflow_saved.py index 5ccbcdf0..fcd8309d 100644 --- a/tests/comfy_cli/command/test_workflow_saved.py +++ b/tests/comfy_cli/command/test_workflow_saved.py @@ -537,13 +537,19 @@ def test_404_surfaces_workflow_not_found(self, cloud_target, monkeypatch, capsys # 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: a valid-UTF-8 non-JSON page -# (e.g. an HTML proxy/error page returned 200) and a non-UTF-8 byte string. The -# latter makes ``json.loads`` raise ``UnicodeDecodeError`` (not ``JSONDecodeError``), -# so both must be caught. Neither may be reported as "no data" (empty list / null id). +# A non-empty 200 body the client can't decode as JSON. Three shapes, all malformed: +# - a valid-UTF-8 non-JSON page (e.g. an HTML proxy/error page returned 200); +# - a non-UTF-8 byte string that isn't valid UTF-8 at all; +# - 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 is what surfaces it. This param locks that in. +# Decoding UTF-8 first raises ``UnicodeDecodeError`` on the two non-UTF-8 cases and +# ``JSONDecodeError`` on the HTML one, so both must be caught. Neither may be reported +# as "no data" (empty list / null id). _UNPARSEABLE_BODIES = [ pytest.param(b"502 Bad Gateway", 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"), ] @@ -555,6 +561,7 @@ def test_list_surfaces_error_not_empty(self, cloud_target, monkeypatch, 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): @@ -565,3 +572,22 @@ def test_save_surfaces_error_not_null_id(self, cloud_target, tmp_path, monkeypat # 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" From 83af3913281e0f8423ffd3fd870314f8a4e237f8 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 10:45:23 -0700 Subject: [PATCH 3/3] fix(workflow): harden cloud-response parsing per Cursor review (BE-3334) Address adversarial review-panel findings on the unparseable-200 handling: - _http_request: catch the ValueError base (+ RecursionError) instead of only (JSONDecodeError, UnicodeDecodeError). Both were already ValueError subclasses; the base also maps json.loads' bare ValueError from an integer past CPython's 4300-digit int/str limit, which previously escaped as a raw traceback. - list_cmd: guard the response shape like get/save do. A valid-JSON non-dict 200 (an array or scalar) hit (body).get("data") -> raw AttributeError; coercing it to [] would masquerade a malformed shape as a genuinely-empty listing. Now mapped to cloud_http_error. - _handle_cloud_http_error: bound the HTTP error-body read with e.read(1000) rather than read-all-then-slice, so a large/malicious error page can't force an unbounded allocation on the error path. - tests: add a bigint bare-ValueError body param and non-dict-shape list cases. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/command/workflow.py | 26 ++++++++++++++-- .../comfy_cli/command/test_workflow_saved.py | 30 ++++++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index cf1c0692..1cfed208 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -625,14 +625,19 @@ def _http_request( return status, None try: return status, json.loads(raw.decode("utf-8")) - except (json.JSONDecodeError, UnicodeDecodeError) as e: + 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``; catch both. + # 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 @@ -648,7 +653,9 @@ def _handle_cloud_http_error(renderer, e, *, operation: str, workflow_id: str | details={"operation": operation, "workflow_id": workflow_id}, ) elif isinstance(e, urllib.error.HTTPError): - body = (e.read() or b"")[:1000].decode("utf-8", "replace") + # 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( code="workflow_not_found", @@ -958,6 +965,19 @@ def list_cmd( except (urllib.error.HTTPError, urllib.error.URLError, OSError, _ResponseUnparseable) as e: raise _handle_cloud_http_error(renderer, e, operation="list") from e + # 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) + rows = (body or {}).get("data") or [] payload = { "count": len(rows), diff --git a/tests/comfy_cli/command/test_workflow_saved.py b/tests/comfy_cli/command/test_workflow_saved.py index fcd8309d..7a299152 100644 --- a/tests/comfy_cli/command/test_workflow_saved.py +++ b/tests/comfy_cli/command/test_workflow_saved.py @@ -537,19 +537,22 @@ def test_404_surfaces_workflow_not_found(self, cloud_target, monkeypatch, capsys # 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. Three shapes, all malformed: -# - a valid-UTF-8 non-JSON page (e.g. an HTML proxy/error page returned 200); -# - a non-UTF-8 byte string that isn't valid UTF-8 at all; +# 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 is what surfaces it. This param locks that in. -# Decoding UTF-8 first raises ``UnicodeDecodeError`` on the two non-UTF-8 cases and -# ``JSONDecodeError`` on the HTML one, so both must be caught. Neither may be reported -# as "no data" (empty list / null id). +# 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"502 Bad Gateway", 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"), ] @@ -591,3 +594,16 @@ def test_delete_surfaces_error_not_success(self, cloud_target, monkeypatch, caps 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"