refactor: share cloud HTTPError/URLError envelope handler across workflow + jobs (BE-3266)

comfy_cli/command/_cloud_errors.py

@@ -0,0 +1,108 @@
+"""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 bounded body read, 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
+
+# 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,
+    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):
+        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=_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": _read_error_body(e), "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)

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:

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,
+    )
 
 
 # ---------------------------------------------------------------------------

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"]

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.