From c14ff329be91090b20914ca99000695084693fea Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 16 Jul 2026 22:30:20 -0700 Subject: [PATCH 1/2] refactor: share cloud HTTPError/URLError envelope handler across workflow + jobs (BE-3266) Promote workflow.py's _handle_cloud_http_error to a shared comfy_cli/command/_cloud_errors.helper, parameterized on the 404 error code/message/hint, the id label, and the operation string. workflow.py's public wrapper and jobs.py _cloud_cancel both route through it. This closes a user-visible drift: jobs cloud-cancel previously mapped 401/403 to a generic cloud_http_error, so an expired session yielded no actionable code. It now surfaces cloud_unauthorized with a `comfy cloud login` hint, matching workflow. Adds a regression test. models/search.py's three branches differ deliberately (per BE-2143) and are left untouched. --- comfy_cli/command/_cloud_errors.py | 82 ++++++++++++++++++++++++++++++ comfy_cli/command/jobs.py | 37 +++++--------- comfy_cli/command/workflow.py | 53 +++++++------------ tests/comfy_cli/jobs/test_jobs.py | 34 +++++++++++++ 4 files changed, 148 insertions(+), 58 deletions(-) create mode 100644 comfy_cli/command/_cloud_errors.py diff --git a/comfy_cli/command/_cloud_errors.py b/comfy_cli/command/_cloud_errors.py new file mode 100644 index 00000000..5d6d17f6 --- /dev/null +++ b/comfy_cli/command/_cloud_errors.py @@ -0,0 +1,82 @@ +"""Shared cloud HTTP/URL error → structured-envelope mapping (BE-3266). + +``comfy workflow`` and ``comfy jobs`` both talk to Comfy Cloud over ``urllib`` +and must turn transport failures into the same structured error envelopes. This +helper is the single source of truth for that mapping so the two call sites +can't drift — historically only ``workflow`` mapped 401/403 to +``cloud_unauthorized`` (with an actionable ``comfy cloud login`` hint) while +``jobs`` emitted a generic ``cloud_http_error``; routing both through here +closes that gap. + +The 404 branch is deliberately caller-parameterized: ``workflow`` surfaces +``workflow_not_found`` and ``jobs`` surfaces ``prompt_not_found``, each with its +own message/hint. Everything else — the identical body truncation, the +401/403 → ``cloud_unauthorized`` branch, the generic ``cloud_http_error``, and +the ``URLError``/``OSError`` network hint — is shared. +""" + +from __future__ import annotations + +import urllib.error + +import typer + + +def handle_cloud_http_error( + renderer, + e: Exception, + *, + operation: str, + not_found_code: str, + not_found_message: str, + not_found_hint: str, + id_label: str, + resource_id: str | None = None, +) -> typer.Exit: + """Map a cloud HTTP/URL failure to a structured error envelope. + + Emits the error via ``renderer.error`` and returns a ``typer.Exit`` for the + caller to ``raise ... from e`` so the original traceback is preserved. + + Args: + operation: short verb naming what failed (``"get"``, ``"cancel"``, …); + used in messages and detail payloads. + not_found_code / not_found_message / not_found_hint: the 404 envelope, + which differs per caller (``workflow_not_found`` vs + ``prompt_not_found``). + id_label: detail key for ``resource_id`` (``"workflow_id"`` / + ``"prompt_id"``). + resource_id: the id being operated on, or ``None`` for id-less + operations (e.g. ``list``). + """ + id_detail = {id_label: resource_id} + if isinstance(e, urllib.error.HTTPError): + body = (e.read() or b"")[:1000].decode("utf-8", "replace") + if e.code == 404: + renderer.error( + code=not_found_code, + message=not_found_message, + hint=not_found_hint, + details={**id_detail, "operation": operation}, + ) + elif e.code in (401, 403): + renderer.error( + code="cloud_unauthorized", + message=f"HTTP {e.code} during {operation}", + hint="re-run `comfy cloud login`", + details={"status": e.code}, + ) + else: + renderer.error( + code="cloud_http_error", + message=f"HTTP {e.code} during {operation}", + hint="check `details.body` for the server's message", + details={"status": e.code, "body": body, "operation": operation, **id_detail}, + ) + else: + renderer.error( + code="cloud_http_error", + message=f"{operation} failed: {e}", + hint="check network / `comfy auth whoami`", + ) + return typer.Exit(code=1) diff --git a/comfy_cli/command/jobs.py b/comfy_cli/command/jobs.py index 9d6db398..695435d8 100644 --- a/comfy_cli/command/jobs.py +++ b/comfy_cli/command/jobs.py @@ -1073,30 +1073,19 @@ def _cloud_cancel(prompt_id: str) -> None: try: with urllib.request.urlopen(req, timeout=15) as resp: body = resp.read() - except urllib.error.HTTPError as e: - body_text = (e.read() or b"")[:1000].decode("utf-8", "replace") - if e.code == 404: - renderer.error( - code="prompt_not_found", - message=f"no cloud job with id {prompt_id!r}", - hint="check `comfy jobs ls --where cloud`", - details={"prompt_id": prompt_id}, - ) - else: - renderer.error( - code="cloud_http_error", - message=f"HTTP {e.code} cancelling {prompt_id}", - hint="check auth and that the job exists", - details={"status": e.code, "body": body_text, "prompt_id": prompt_id}, - ) - raise typer.Exit(code=1) from e - except (urllib.error.URLError, OSError) as e: - renderer.error( - code="cloud_http_error", - message=f"cancel failed: {e}", - hint="check network / `comfy auth whoami`", - ) - raise typer.Exit(code=1) from e + except (urllib.error.HTTPError, urllib.error.URLError, OSError) as e: + from comfy_cli.command._cloud_errors import handle_cloud_http_error + + raise handle_cloud_http_error( + renderer, + e, + operation="cancel", + not_found_code="prompt_not_found", + not_found_message=f"no cloud job with id {prompt_id!r}", + not_found_hint="check `comfy jobs ls --where cloud`", + id_label="prompt_id", + resource_id=prompt_id, + ) from e parsed: dict | None try: diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index d953c10e..b70f8246 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -624,41 +624,26 @@ def _http_request( 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 + """Map HTTP failures to envelope codes. Returns an Exit to ``raise from``. - if isinstance(e, urllib.error.HTTPError): - body = (e.read() or b"")[:1000].decode("utf-8", "replace") - if e.code == 404: - renderer.error( - code="workflow_not_found", - message=f"no saved workflow with id {workflow_id!r}" - if workflow_id - else f"workflow not found ({operation})", - hint="list available workflows via `comfy --json workflow list`", - details={"workflow_id": workflow_id, "operation": operation}, - ) - elif e.code in (401, 403): - renderer.error( - code="cloud_unauthorized", - message=f"HTTP {e.code} during {operation}", - hint="re-run `comfy cloud login`", - details={"status": e.code}, - ) - else: - renderer.error( - code="cloud_http_error", - message=f"HTTP {e.code} during {operation}", - hint="check `details.body` for the server's message", - details={"status": e.code, "body": body, "operation": operation}, - ) - else: - renderer.error( - code="cloud_http_error", - message=f"{operation} failed: {e}", - hint="check network / `comfy auth whoami`", - ) - return typer.Exit(code=1) + Thin wrapper over the shared cloud-error mapper (BE-3266) that supplies the + ``workflow``-specific 404 envelope; everything else is shared with ``jobs``. + """ + from comfy_cli.command._cloud_errors import handle_cloud_http_error + + not_found_message = ( + f"no saved workflow with id {workflow_id!r}" if workflow_id else f"workflow not found ({operation})" + ) + return handle_cloud_http_error( + renderer, + e, + operation=operation, + not_found_code="workflow_not_found", + not_found_message=not_found_message, + not_found_hint="list available workflows via `comfy --json workflow list`", + id_label="workflow_id", + resource_id=workflow_id, + ) # --------------------------------------------------------------------------- diff --git a/tests/comfy_cli/jobs/test_jobs.py b/tests/comfy_cli/jobs/test_jobs.py index 5ed15898..42b66489 100644 --- a/tests/comfy_cli/jobs/test_jobs.py +++ b/tests/comfy_cli/jobs/test_jobs.py @@ -532,6 +532,40 @@ def test_jobs_cancel_cloud_404_surfaces_prompt_not_found(monkeypatch: pytest.Mon assert "prompt_not_found" in result.output +@pytest.mark.parametrize("code", [401, 403]) +def test_jobs_cancel_cloud_auth_failure_surfaces_cloud_unauthorized(monkeypatch: pytest.MonkeyPatch, code: int): + """An expired/insufficient session cancelling a cloud job surfaces the + actionable ``cloud_unauthorized`` code (shared envelope handler, BE-3266) — + not the generic ``cloud_http_error`` it produced before the two call sites + were unified.""" + import io + import urllib.error + + from typer.testing import CliRunner + + from comfy_cli.target import Target + + fake_target = Target( + kind="cloud", + base_url="https://cloud.example.com", + path_prefix="/api", + history_path="history_v2", + jobs_path="jobs", + api_key="test-key", + ) + monkeypatch.setattr("comfy_cli.target.resolve_target", lambda **kw: fake_target) + monkeypatch.setattr(jobs_mod, "_is_cloud", lambda w: True) + monkeypatch.setattr(jobs_mod, "cloud_preflight_or_exit", lambda: None) + + err = urllib.error.HTTPError("https://x/cancel", code, "Unauthorized", {}, io.BytesIO(b'{"error":"expired"}')) + _capture_urlopen(monkeypatch, {"/api/jobs/prompt-abc/cancel": err}) + + runner = CliRunner() + result = runner.invoke(jobs_mod.app, ["cancel", "prompt-abc", "--where", "cloud"]) + assert result.exit_code == 1 + assert "cloud_unauthorized" in result.output + + def test_is_cloud_honors_env_var(monkeypatch: pytest.MonkeyPatch): """``comfy --where cloud jobs status X`` sets COMFY_WHERE in the env. ``_is_cloud(None)`` must return True so the cloud path is taken. From 105a002c15c171d9ac517bb4f2b92e45ed3cdcd7 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 16 Jul 2026 22:56:20 -0700 Subject: [PATCH 2/2] fix(cloud): bound the error-body read and enrich 401/403 details (BE-3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cursor panel findings on the shared envelope handler: - `e.read()` was uncapped and eager. `base_url` is env-configurable, so a hostile/misbehaving endpoint could OOM the CLI, and a reset stream raised *before* any `renderer.error` fired — turning the structured envelope the helper exists to emit into an unhandled traceback. The read is now capped at 1000 bytes (matching the prior `[:1000]` truncation exactly), best-effort (a failed read degrades to an empty body), and skipped entirely on the 404 branch that discards it. - 401/403 hardcoded the `comfy cloud login` hint and dropped the server body and resource id from `details` — a regression for jobs, which surfaced both via the generic branch before unification. A 403 is not unambiguously an auth failure (forbidden resource, quota, already-finished job), so it now keeps the server's explanation and gets a hint that doesn't assert re-login as the fix. 401 is unchanged. `details` is free-form in error.json, so the added keys are additive. --- comfy_cli/command/_cloud_errors.py | 36 +++++- tests/comfy_cli/command/test_cloud_errors.py | 122 +++++++++++++++++++ 2 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 tests/comfy_cli/command/test_cloud_errors.py diff --git a/comfy_cli/command/_cloud_errors.py b/comfy_cli/command/_cloud_errors.py index 5d6d17f6..55d6d62c 100644 --- a/comfy_cli/command/_cloud_errors.py +++ b/comfy_cli/command/_cloud_errors.py @@ -10,7 +10,7 @@ The 404 branch is deliberately caller-parameterized: ``workflow`` surfaces ``workflow_not_found`` and ``jobs`` surfaces ``prompt_not_found``, each with its -own message/hint. Everything else — the identical body truncation, the +own message/hint. Everything else — the bounded body read, the 401/403 → ``cloud_unauthorized`` branch, the generic ``cloud_http_error``, and the ``URLError``/``OSError`` network hint — is shared. """ @@ -21,6 +21,33 @@ import typer +# Cap on the server error body we surface in ``details.body``. ``base_url`` is +# env-configurable, so a hostile or misbehaving endpoint must not be able to +# OOM the CLI with an unbounded error response. +_MAX_ERROR_BODY_BYTES = 1000 + +# A 401 is unambiguously an authentication failure. A 403 is not — it is also +# how the server denies a forbidden resource, a quota, or an already-finished +# job, so pointing the user straight at re-login would be a misleading +# remediation. Send them to the server's own explanation instead. +_UNAUTHORIZED_HINTS = { + 401: "re-run `comfy cloud login`", + 403: "re-run `comfy cloud login` if your session expired; otherwise check `details.body` — the server may be denying access to this resource", +} + + +def _read_error_body(e: urllib.error.HTTPError) -> str: + """Best-effort read of a bounded slice of the server's error body. + + A read that raises (reset or truncated stream) must not pre-empt the + structured envelope this module exists to emit, so failures degrade to an + empty body rather than escaping as an unhandled traceback. + """ + try: + return (e.read(_MAX_ERROR_BODY_BYTES) or b"").decode("utf-8", "replace") + except Exception: + return "" + def handle_cloud_http_error( renderer, @@ -51,7 +78,6 @@ def handle_cloud_http_error( """ id_detail = {id_label: resource_id} if isinstance(e, urllib.error.HTTPError): - body = (e.read() or b"")[:1000].decode("utf-8", "replace") if e.code == 404: renderer.error( code=not_found_code, @@ -63,15 +89,15 @@ def handle_cloud_http_error( renderer.error( code="cloud_unauthorized", message=f"HTTP {e.code} during {operation}", - hint="re-run `comfy cloud login`", - details={"status": e.code}, + hint=_UNAUTHORIZED_HINTS[e.code], + details={"status": e.code, "body": _read_error_body(e), "operation": operation, **id_detail}, ) else: renderer.error( code="cloud_http_error", message=f"HTTP {e.code} during {operation}", hint="check `details.body` for the server's message", - details={"status": e.code, "body": body, "operation": operation, **id_detail}, + details={"status": e.code, "body": _read_error_body(e), "operation": operation, **id_detail}, ) else: renderer.error( diff --git a/tests/comfy_cli/command/test_cloud_errors.py b/tests/comfy_cli/command/test_cloud_errors.py new file mode 100644 index 00000000..53315c28 --- /dev/null +++ b/tests/comfy_cli/command/test_cloud_errors.py @@ -0,0 +1,122 @@ +"""Unit tests for the shared cloud error → envelope mapper (BE-3266).""" + +from __future__ import annotations + +import io +import urllib.error + +import pytest +import typer + +from comfy_cli.command._cloud_errors import _MAX_ERROR_BODY_BYTES, handle_cloud_http_error + + +class _FakeRenderer: + """Captures the single ``renderer.error`` call the mapper is expected to make.""" + + def __init__(self): + self.calls: list[dict] = [] + + def error(self, **kwargs): + self.calls.append(kwargs) + + +def _http_error(code: int, body: bytes = b'{"error":"boom"}') -> urllib.error.HTTPError: + return urllib.error.HTTPError("https://cloud.example.com/x", code, "err", {}, io.BytesIO(body)) + + +def _handle(renderer, e: Exception) -> typer.Exit: + return handle_cloud_http_error( + renderer, + e, + operation="cancel", + not_found_code="prompt_not_found", + not_found_message="no cloud job with id 'p1'", + not_found_hint="check `comfy jobs ls --where cloud`", + id_label="prompt_id", + resource_id="p1", + ) + + +@pytest.mark.parametrize("code", [401, 403, 500]) +def test_error_body_read_is_capped(code: int): + """``base_url`` is env-configurable, so a hostile endpoint returning a huge + error body must not be buffered unbounded into memory.""" + renderer = _FakeRenderer() + _handle(renderer, _http_error(code, b"A" * (5 * 1024 * 1024))) + + body = renderer.calls[0]["details"]["body"] + assert len(body) <= _MAX_ERROR_BODY_BYTES + assert body == "A" * _MAX_ERROR_BODY_BYTES + + +class _ExplodingBody(io.BytesIO): + def read(self, *args): + raise ConnectionResetError("stream reset mid-read") + + +@pytest.mark.parametrize("code", [401, 403, 500]) +def test_body_read_failure_still_emits_envelope(code: int): + """A reset/truncated body stream must degrade to an empty body, not escape + as an unhandled traceback in place of the structured envelope.""" + renderer = _FakeRenderer() + e = urllib.error.HTTPError("https://x/y", code, "err", {}, _ExplodingBody(b"ignored")) + + exit_exc = _handle(renderer, e) + + assert isinstance(exit_exc, typer.Exit) + assert len(renderer.calls) == 1 + assert renderer.calls[0]["details"]["body"] == "" + + +def test_404_does_not_consume_body(): + """The 404 envelope discards the body, so it should not read the stream.""" + renderer = _FakeRenderer() + e = urllib.error.HTTPError("https://x/y", 404, "err", {}, _ExplodingBody(b"ignored")) + + _handle(renderer, e) + + call = renderer.calls[0] + assert call["code"] == "prompt_not_found" + assert "body" not in call["details"] + + +@pytest.mark.parametrize("code", [401, 403]) +def test_unauthorized_details_carry_body_and_resource_id(code: int): + """A 403 for a non-auth reason (forbidden resource, quota, finished job) + must not lose the server's explanation or the id being operated on.""" + renderer = _FakeRenderer() + _handle(renderer, _http_error(code, b'{"error":"quota exceeded"}')) + + call = renderer.calls[0] + assert call["code"] == "cloud_unauthorized" + assert call["details"]["status"] == code + assert "quota exceeded" in call["details"]["body"] + assert call["details"]["prompt_id"] == "p1" + assert call["details"]["operation"] == "cancel" + + +def test_401_hint_points_at_relogin(): + renderer = _FakeRenderer() + _handle(renderer, _http_error(401)) + assert renderer.calls[0]["hint"] == "re-run `comfy cloud login`" + + +def test_403_hint_is_softened(): + """403 is not unambiguously an auth failure, so the hint must not assert + re-login as the remediation the way 401's does.""" + renderer = _FakeRenderer() + _handle(renderer, _http_error(403)) + + hint = renderer.calls[0]["hint"] + assert "details.body" in hint + assert hint != "re-run `comfy cloud login`" + + +def test_url_error_surfaces_network_hint(): + renderer = _FakeRenderer() + _handle(renderer, urllib.error.URLError("connection refused")) + + call = renderer.calls[0] + assert call["code"] == "cloud_http_error" + assert "comfy auth whoami" in call["hint"]