From 9d2f158128dbe8e54e3c91b5158fcb9d8fec5ca4 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 25 Jul 2026 17:45:30 -0700 Subject: [PATCH 1/4] fix(generate): emit envelope/1 errors under global --json and reject a flag-shaped model target `comfy --json generate --prompt=x` exited 1 with no JSON on stdout: the flag token was swallowed by the model-alias positional, `spec.get_endpoint` raised, and the handler printed via raw `rich.print`. Any envelope-consuming caller got a blank failure. Every error path in the `generate` module now routes through a `_fail` helper: when the global renderer is in JSON / JSON-stream mode it emits exactly one `envelope/1` error with a stable, registered `code`; otherwise it keeps the historical rich-red output byte-for-byte. Success/result output is unchanged -- the module's non-migration rationale still stands for results. A flag-shaped first positional now fails immediately with `generate_target_required` and points at both `comfy generate list` and `comfy run-template` (the local text-to-image alternative), instead of dying deep inside the spec lookup. Also closes three adjacent silent failures: unhandled SchemaError in `generate list` / `generate schema`, and unreported poll failures behind the transient spinner in the submit and resume paths. --- comfy_cli/command/generate/app.py | 229 +++++++++++++---- comfy_cli/error_codes.py | 51 ++++ comfy_cli/skills/comfy/SKILL.md | 11 + .../command/generate/test_json_errors.py | 242 ++++++++++++++++++ .../command/generate/test_spend_gate.py | 19 +- 5 files changed, 498 insertions(+), 54 deletions(-) create mode 100644 tests/comfy_cli/command/generate/test_json_errors.py diff --git a/comfy_cli/command/generate/app.py b/comfy_cli/command/generate/app.py index 221ae4c4..e688e37b 100644 --- a/comfy_cli/command/generate/app.py +++ b/comfy_cli/command/generate/app.py @@ -25,8 +25,9 @@ import sys import uuid +from collections.abc import Mapping from pathlib import Path -from typing import Annotated +from typing import Annotated, Any import httpx import typer @@ -38,6 +39,11 @@ # stderr whenever stdout isn't a TTY, leaving stdout empty -- so `comfy generate # ... > out.txt` would write an empty file. Migrate only once `generate` emits # envelopes via the renderer. +# +# That rationale covers *results* only. FAILURES go through `_fail` below, which +# emits an `envelope/1` error whenever the global renderer is in JSON / +# JSON-stream mode -- an envelope-consuming caller must never get exit 1 with a +# blank stdout. from rich import print as rprint from rich.progress import Progress, SpinnerColumn, TextColumn, TimeElapsedColumn @@ -54,6 +60,62 @@ "help_option_names": [], } +_TARGET_REQUIRED_MSG = ( + "`comfy generate` requires a partner model alias as its first argument " + '(e.g. `comfy generate flux-pro --prompt "a cat on the moon"`); it is a cloud/partner ' + "verb that spends credits." +) +_TARGET_REQUIRED_HINT = ( + "Run `comfy generate list` to see model aliases. For local text-to-image, use `comfy run-template` instead." +) + + +def _fail( + *, + code: str, + message: str, + hint: str | None = None, + details: Mapping[str, Any] | None = None, + legacy_json: bool = False, + pretty: str | None = None, +) -> None: + """Report a `generate` failure through whichever channel the caller asked for. + + - Global `--json` / `--json-stream` (and any non-TTY stdout, which the + renderer already resolves to JSON) → exactly one `envelope/1` error on + stdout with a stable `code`. Without this an envelope-consuming caller + (comfy-local-mcp, scripts, agents) sees exit 1 with nothing to read. + - `legacy_json` → the pre-existing command-local `--json` error object, kept + byte-compatible for the paths that already emitted one. + - Otherwise → the historical rich-red line (plus a dim hint line where the + path already printed one), so pretty/TTY output is unchanged. + + ``hint`` is only passed where pretty mode already printed that second line; + everywhere else `renderer.error` falls back to the code's registered hint, + which keeps pretty output identical while JSON callers still get navigation. + + Keyword-only on purpose: ``tests/comfy_cli/output/test_error_code_registry.py`` + scans for literal ``code="…"`` kwargs to pin every raised code against + :mod:`comfy_cli.error_codes`, and a positional first argument would be + invisible to it. + """ + renderer = get_renderer() + if renderer.is_json(): + renderer.error(code=code, message=message, hint=hint, details=details) + return + if legacy_json: + output.print_json({"error": message, "code": code}) + return + rprint(pretty if pretty is not None else f"[bold red]{message}[/bold red]") + if hint: + rprint(f"[dim]{hint}[/dim]") + + +def _transport_code(exc: BaseException) -> str: + """`generate_network_error` for a transport failure, `generate_api_error` for + an HTTP/API-level one — the two arrive together on most call sites.""" + return "generate_network_error" if isinstance(exc, httpx.HTTPError) else "generate_api_error" + def register_with(parent: typer.Typer) -> None: """Wire the ``generate`` command into a Typer app. We register directly @@ -75,6 +137,18 @@ def _generate_entry( if target is None or target in {"-h", "--help"}: _print_top_help() raise typer.Exit(code=0) + if target.startswith("-"): + # `ignore_unknown_options` lets a flag token slide into the model-alias + # positional, so `comfy generate --prompt=x` used to die deep inside + # `spec.get_endpoint` as an "unknown model". Fail here instead, at the + # earliest point, with an actionable message. + _fail( + code="generate_target_required", + message=_TARGET_REQUIRED_MSG, + hint=_TARGET_REQUIRED_HINT, + details={"received": target}, + ) + raise typer.Exit(code=1) extra = list(ctx.args) if target == "list": tracking.track_event("generate:list") @@ -174,8 +248,21 @@ def _emit_result(result: poll.PollResult, *, request_id: str, download: str | No output.print_json(result.raw) return if result.status != "succeeded": - rprint(f"[bold red]Job {result.status}: {result.error or 'unknown error'}[/bold red]") - output.print_json(result.raw) + # A terminal non-succeeded job is a FAILURE, not a result, so it owes the + # caller an envelope even though the success paths above deliberately + # bypass the renderer. (The `as_json` branch returned already: that is + # the command-local `--json` raw-response contract, left untouched.) + renderer = get_renderer() + message = f"Job {result.status}: {result.error or 'unknown error'}" + if renderer.is_json(): + renderer.error( + code="generate_job_failed", + message=message, + details={"status": result.status, "response": result.raw}, + ) + else: + rprint(f"[bold red]{message}[/bold red]") + output.print_json(result.raw) raise typer.Exit(code=1) if download and result.image_urls: saved = output.save_urls(result.image_urls, download, request_id) @@ -225,15 +312,18 @@ def _confirm_spend(*, model_name: str, assume_yes: bool, as_json: bool) -> None: f"`comfy generate {model_name}` spends Comfy credits and no consent was given. " "Re-run with --yes, or persist consent with `comfy generate consent always`." ) - if as_json: - output.print_json({"error": msg, "code": "spend_consent_required"}) - else: - rprint(f"[bold red]{msg}[/bold red]") + _fail(code="spend_consent_required", message=msg, legacy_json=as_json) raise SpendNotConfirmed(msg) rprint(f"[bold]{model_name}[/bold] runs via the partner API and [bold]spends Comfy credits[/bold].") rprint("[dim]Skip this prompt with --yes; persist always-proceed with `comfy generate consent always`.[/dim]") if not typer.confirm("Proceed?", default=False): - rprint("Canceled — no credits were spent.") + # Reachable with a TTY stdin but a redirected (JSON-mode) stdout, so the + # decline still owes the caller an envelope rather than a bare line. + _fail( + code="spend_consent_required", + message="Canceled — no credits were spent.", + pretty="Canceled — no credits were spent.", + ) raise SpendNotConfirmed("user declined the spend confirmation prompt") @@ -244,16 +334,13 @@ def _consent(extra_args: list[str]) -> None: try: clean, meta = _separate_meta_flags(extra_args) except schema.SchemaError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_bad_args", message=str(e)) raise typer.Exit(code=1) as_json = bool(meta.get("json", False)) action = clean[0] if clean and not clean[0].startswith("-") else "show" if action not in {"show", "always", "ask"}: msg = f"Unknown consent action {action!r}. Usage: comfy generate consent [show|always|ask]" - if as_json: - output.print_json({"error": msg}) - else: - rprint(f"[bold red]{msg}[/bold red]") + _fail(code="generate_bad_args", message=msg, legacy_json=as_json) raise typer.Exit(code=1) if action == "always": ConfigManager().set(constants.CONFIG_KEY_SPEND_AUTO_CONFIRM, "true") @@ -305,7 +392,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: ep = spec.get_endpoint(model) except spec.SpecError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_unknown_model", message=str(e), details={"model": model}) _track_error("schema", e) raise typer.Exit(code=1) @@ -315,7 +402,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: remaining, meta = _separate_meta_flags(extra_args) except schema.SchemaError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_bad_args", message=str(e)) _track_error("schema", e) raise typer.Exit(code=1) @@ -333,9 +420,12 @@ def _track_error(error_kind: str, exc: BaseException) -> None: # they want. values = schema.parse_args(flags, remaining, require_all=not emit_path) except schema.SchemaError as e: - rprint(f"[bold red]{e}[/bold red]") name = gen_props["model_alias"] or ep.id - rprint(f"[dim]Run `comfy generate schema {name}` for the full parameter list.[/dim]") + _fail( + code="generate_bad_args", + message=str(e), + hint=f"Run `comfy generate schema {name}` for the full parameter list.", + ) _track_error("schema", e) raise typer.Exit(code=1) @@ -387,7 +477,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: api_key = client.resolve_api_key(meta.get("api-key") if isinstance(meta.get("api-key"), str) else None) except client.ApiError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_api_error", message=str(e)) _track_error("api", e) raise typer.Exit(code=1) @@ -395,14 +485,14 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: timeout = float(timeout_raw) if isinstance(timeout_raw, str) else 300.0 except ValueError as e: - rprint(f"[bold red]--timeout: expected number, got {timeout_raw!r}[/bold red]") + _fail(code="generate_timeout_invalid", message=f"--timeout: expected number, got {timeout_raw!r}") _track_error("schema", e) raise typer.Exit(code=1) try: _apply_upload_transforms(values, flags, ep, api_key) except (client.ApiError, httpx.HTTPError) as e: - rprint(f"[bold red]Upload failed: {e}[/bold red]") + _fail(code=_transport_code(e), message=f"Upload failed: {e}") _track_error("upload", e) raise typer.Exit(code=1) @@ -410,14 +500,19 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: resp = client.send_request(ep, values, flags, api_key, timeout=timeout) except httpx.HTTPError as e: - rprint(f"[bold red]Network error contacting {spec.base_url()}: {e}[/bold red]") + _fail(code="generate_network_error", message=f"Network error contacting {spec.base_url()}: {e}") _track_error("network", e) raise typer.Exit(code=1) from e try: client.raise_for_status(resp) except client.ApiError as e: - rprint(f"[bold red]API error {e.status}[/bold red]\n{e.body}") + _fail( + code="generate_api_error", + message=f"API error {e.status}", + details={"status": e.status, "body": e.body}, + pretty=f"[bold red]API error {e.status}[/bold red]\n{e.body}", + ) _track_error("api", e) raise typer.Exit(code=1) from e @@ -435,8 +530,13 @@ def _track_error(error_kind: str, exc: BaseException) -> None: try: body = resp.json() except ValueError as e: - rprint("[bold red]Unexpected non-JSON response.[/bold red]") - rprint(resp.text[:500]) + preview = resp.text[:500] + _fail( + code="generate_api_error", + message="Unexpected non-JSON response.", + details={"body_preview": preview}, + pretty=f"[bold red]Unexpected non-JSON response.[/bold red]\n{preview}", + ) _track_error("non_json_response", e) raise typer.Exit(code=1) @@ -479,6 +579,9 @@ def _on_progress(p: float) -> None: create_path=ep.path, ) except (client.ApiError, httpx.HTTPError) as e: + # The spinner is transient, so without this the run ended with + # a bare exit 1 and an empty screen in EVERY mode. + _fail(code=_transport_code(e), message=f"Job {job_id} failed while polling: {e}") _track_error("network" if isinstance(e, httpx.HTTPError) else "api", e) raise typer.Exit(code=1) from e try: @@ -543,7 +646,13 @@ def _arg_value(args: list[str], *names: str) -> str | None: def _list_models(extra_args: list[str]) -> None: """`comfy generate list` — show available models with their short aliases.""" - clean, meta = _separate_meta_flags(extra_args) + try: + clean, meta = _separate_meta_flags(extra_args) + except schema.SchemaError as e: + # e.g. `comfy generate list --download` (meta flag with no value) — this + # used to escape as an unhandled SchemaError traceback. + _fail(code="generate_bad_args", message=str(e)) + raise typer.Exit(code=1) as_json = bool(meta.get("json", False)) partner = _arg_value(clean, "--partner", "-p") category = _arg_value(clean, "--category", "--style", "-c") @@ -582,21 +691,19 @@ def _list_models(extra_args: list[str]) -> None: def _schema(extra_args: list[str]) -> None: """`comfy generate schema ` — show params for a model (fal-style).""" - clean, meta = _separate_meta_flags(extra_args) + try: + clean, meta = _separate_meta_flags(extra_args) + except schema.SchemaError as e: + _fail(code="generate_bad_args", message=str(e)) + raise typer.Exit(code=1) as_json = bool(meta.get("json", False)) if not clean or clean[0].startswith("-"): - if as_json: - output.print_json({"error": "Usage: comfy generate schema "}) - raise typer.Exit(code=1) - rprint("[bold red]Usage: comfy generate schema [/bold red]") + _fail(code="generate_bad_args", message="Usage: comfy generate schema ", legacy_json=as_json) raise typer.Exit(code=1) try: ep = spec.get_endpoint(clean[0]) except spec.SpecError as e: - if as_json: - output.print_json({"error": str(e)}) - raise typer.Exit(code=1) - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_unknown_model", message=str(e), details={"model": clean[0]}, legacy_json=as_json) raise typer.Exit(code=1) if as_json: flags = schema.flags_for(ep) @@ -644,7 +751,7 @@ def _refresh() -> None: fetched_from = fallback r = _fetch_spec(fallback) except httpx.HTTPError as e: - rprint(f"[bold red]Failed to fetch {fetched_from}: {e}[/bold red]") + _fail(code="generate_network_error", message=f"Failed to fetch {fetched_from}: {e}") raise typer.Exit(code=1) # Validate before caching so a 200-with-garbage response never poisons the @@ -654,7 +761,7 @@ def _refresh() -> None: try: spec.validate_spec_text(body) except spec.SpecError as e: - rprint(f"[bold red]Refusing to cache spec from {fetched_from}: {e}[/bold red]") + _fail(code="generate_spec_invalid", message=f"Refusing to cache spec from {fetched_from}: {e}") raise typer.Exit(code=1) path = spec.write_cache(body) @@ -666,24 +773,28 @@ def _upload(extra_args: list[str]) -> None: try: remaining, meta = _separate_meta_flags(extra_args) except schema.SchemaError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_bad_args", message=str(e)) raise typer.Exit(code=1) # `remaining` already excludes recognized --meta flags AND their values, so # `comfy generate upload --api-key KEY ./img.png` correctly resolves to "./img.png". if not remaining: - rprint("[bold red]Usage: comfy generate upload [--json][/bold red]") + _fail( + code="generate_bad_args", + message="Usage: comfy generate upload [--json]", + pretty="[bold red]Usage: comfy generate upload [--json][/bold red]", + ) raise typer.Exit(code=1) target = remaining[0] try: api_key = client.resolve_api_key(meta.get("api-key") if isinstance(meta.get("api-key"), str) else None) except client.ApiError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_api_error", message=str(e)) raise typer.Exit(code=1) as_json = bool(meta.get("json", False)) try: result = upload.upload_target(target, api_key) except (client.ApiError, httpx.HTTPError) as e: - rprint(f"[bold red]Upload failed: {e}[/bold red]") + _fail(code=_transport_code(e), message=f"Upload failed: {e}") raise typer.Exit(code=1) if as_json: output.print_json( @@ -741,27 +852,33 @@ def _apply_upload_transforms(values: dict, flags: list[schema.FlagDef], endpoint def _resume(extra_args: list[str]) -> None: if len(extra_args) < 2 or extra_args[0].startswith("-") or extra_args[1].startswith("-"): - rprint("[bold red]Usage: comfy generate resume [--download PATH] [--json][/bold red]") + _fail( + code="generate_bad_args", + message="Usage: comfy generate resume [--download PATH] [--json]", + pretty="[bold red]Usage: comfy generate resume [--download PATH] [--json][/bold red]", + ) raise typer.Exit(code=1) model, job_id = extra_args[0], extra_args[1] tail = extra_args[2:] try: ep = spec.get_endpoint(model) except spec.SpecError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_unknown_model", message=str(e), details={"model": model}) raise typer.Exit(code=1) if not ep.polling: - rprint(f"[bold red]{model} is a sync model; nothing to resume.[/bold red]") + _fail( + code="generate_bad_args", message=f"{model} is a sync model; nothing to resume.", details={"model": model} + ) raise typer.Exit(code=1) try: _, meta = _separate_meta_flags(tail) except schema.SchemaError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_bad_args", message=str(e)) raise typer.Exit(code=1) try: api_key = client.resolve_api_key(meta.get("api-key") if isinstance(meta.get("api-key"), str) else None) except client.ApiError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_api_error", message=str(e)) raise typer.Exit(code=1) timeout = float(meta.get("timeout") or 300.0) if isinstance(meta.get("timeout"), str) else 300.0 download = meta.get("download") if isinstance(meta.get("download"), str) else None @@ -770,7 +887,7 @@ def _resume(extra_args: list[str]) -> None: try: initial = poll.build_synthetic_initial(ep.polling, job_id, base_url=spec.base_url()) except client.ApiError as e: - rprint(f"[bold red]{e}[/bold red]") + _fail(code="generate_api_error", message=str(e)) raise typer.Exit(code=1) poller = poll.get_poller(ep.polling) @@ -780,13 +897,19 @@ def _resume(extra_args: list[str]) -> None: def _on_progress(p: float) -> None: prog.update(task, description=f"Job {job_id} ({p * 100:.0f}%)") - result = poller( - initial, - api_key=api_key, - timeout=timeout, - on_progress=_on_progress, - create_path=ep.path, - ) + try: + result = poller( + initial, + api_key=api_key, + timeout=timeout, + on_progress=_on_progress, + create_path=ep.path, + ) + except (client.ApiError, httpx.HTTPError) as e: + # Same transient-spinner trap as the submit path: an unhandled poll + # failure here used to surface as a raw traceback. + _fail(code=_transport_code(e), message=f"Job {job_id} failed while polling: {e}") + raise typer.Exit(code=1) from e _emit_result(result, request_id=job_id, download=download, as_json=as_json) diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index f0729744..5eb7dfb3 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -572,6 +572,57 @@ class ErrorCode: "add the name under `vars:` in /comfy.yaml, then re-compose", ), # --- generate / emit ----------------------------------------------------- + ErrorCode( + "generate_target_required", + "`comfy generate` was invoked with a flag token where its first positional argument (the " + "partner model alias) belongs — e.g. `comfy generate --prompt=x`. `generate` is a " + "cloud/partner verb that spends credits; it always needs a model alias first.", + 'name a model alias first (`comfy generate flux-pro --prompt "…"`, `comfy generate list` to ' + "browse them), or use `comfy run-template` for local text-to-image", + ), + ErrorCode( + "generate_unknown_model", + "The model alias/id passed to `comfy generate` (or `generate schema` / `generate resume`) is " + "not in the partner-endpoint catalog.", + "run `comfy generate list` to see available models; `comfy generate refresh` re-fetches the catalog", + ), + ErrorCode( + "generate_bad_args", + "`comfy generate` could not parse its arguments: a missing/malformed flag value, a missing " + "required model parameter, a bad subcommand usage, or a resume of a non-polling model.", + "run `comfy generate schema ` for the parameter list, or `comfy generate --help` for usage", + ), + ErrorCode( + "generate_timeout_invalid", + "`comfy generate --timeout` was given a value that isn't a number.", + "pass seconds as a number, e.g. `--timeout 300`", + ), + ErrorCode( + "generate_api_error", + "The partner-proxy API rejected the call or returned an unusable response (auth failure, " + "non-2xx status, non-JSON body). `details.status` / `details.body` carry the response when " + "the failure was an HTTP status.", + "check `comfy cloud login` / COMFY_API_KEY and the reported status; retry if it was a 5xx", + ), + ErrorCode( + "generate_network_error", + "A transport-level failure (DNS, TLS, connect, read timeout) while talking to the partner " + "proxy — the request may never have reached it.", + "check network connectivity and retry; raise `--timeout` if the model is slow", + ), + ErrorCode( + "generate_job_failed", + "The partner job reached a terminal non-succeeded state (failed/cancelled). " + "`details.response` carries the raw partner response.", + "check `details.response` for the partner's reason; fix the inputs and re-run, or " + "`comfy generate resume ` if the job may still settle", + ), + ErrorCode( + "generate_spec_invalid", + "`comfy generate refresh` fetched an OpenAPI document that failed validation, so it was " + "refused rather than cached over the working catalog.", + "check COMFY_API_BASE_URL points at the Comfy API; the existing cached catalog is still usable", + ), ErrorCode( "emit_workflow_failed", "`generate --emit-workflow` could not build the partner-node workflow.", diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index 51d2303e..6cc0b23c 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -670,6 +670,17 @@ Mechanical contracts that bite agents — encode them, don't rediscover: error code `emit_workflow_failed`) — the output is a runnable partner-node workflow you can compose with (fragments+`run` route, no extra API key). The default pretty path (no flags) is still human-only — do not parse it. +- **Failures always come back as an envelope under global `--json`.** Any + failed/malformed `comfy --json generate …` emits exactly one `envelope/1` + error on stdout with a stable code — `generate_target_required`, + `generate_unknown_model`, `generate_bad_args`, `generate_timeout_invalid`, + `generate_api_error`, `generate_network_error`, `generate_spec_invalid`, + `spend_consent_required` — so branch on `error.code`, never on the text. + (Success payloads are unchanged: still the raw API response, not an + envelope.) In particular `comfy generate --prompt "…"` with no model alias + is `generate_target_required`: `generate` is a paid cloud/partner verb and + always needs an alias first. For **local** text-to-image, use + `comfy run-template` instead. - **`--emit-workflow` resolves the escape-hatch vs. quality tradeoff:** fragments+`run` is the default for graph work; `generate` is the highest-quality single-shot for partner models. With diff --git a/tests/comfy_cli/command/generate/test_json_errors.py b/tests/comfy_cli/command/generate/test_json_errors.py new file mode 100644 index 00000000..13d54551 --- /dev/null +++ b/tests/comfy_cli/command/generate/test_json_errors.py @@ -0,0 +1,242 @@ +"""`comfy generate` failure paths under the GLOBAL machine-output modes. + +Every `comfy generate` error used to reach an envelope-consuming caller as exit +1 with a bare `rich.print` line on stdout and nothing parseable — the failure +mode that made a malformed `comfy --json generate --prompt=x` undiagnosable from +the caller side (the flag token was swallowed by the model-alias positional and +`spec.get_endpoint` raised deep inside). + +These tests pin the contract from the caller's side: under global `--json` / +`--json-stream`, a failed `comfy generate` always emits exactly ONE `envelope/1` +error object on stdout with a stable `code`, and exits non-zero. Pretty mode +(forced here with the global `--no-json`, since CliRunner's piped stdout would +otherwise auto-resolve to JSON) keeps its historical rich output. +""" + +from __future__ import annotations + +import json + +import httpx +import pytest +from typer.testing import CliRunner + +from comfy_cli import error_codes +from comfy_cli.cmdline import app as cli_app +from comfy_cli.command.generate import app as gen_app + + +@pytest.fixture(autouse=True) +def disable_tracking_prompt(monkeypatch): + monkeypatch.setattr("comfy_cli.tracking.prompt_tracking_consent", lambda *a, **kw: None) + monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **kw: None) + + +@pytest.fixture +def runner(): + return CliRunner() + + +@pytest.fixture +def api_key(monkeypatch): + monkeypatch.setenv("COMFY_API_KEY", "comfyui-test") + return "comfyui-test" + + +def _sole_envelope(result) -> dict: + """Parse stdout as exactly one `envelope/1` error line, asserting the shape + every consumer relies on. The single-line assertion is the point: a second + JSON object (the legacy flat error) would break naive callers.""" + lines = [ln for ln in result.stdout.strip().splitlines() if ln.strip()] + assert len(lines) == 1, f"expected a single envelope line, got {lines!r}" + env = json.loads(lines[0]) + assert env["schema"] == "envelope/1" + assert env["type"] == "envelope" + assert env["ok"] is False + assert env["error"]["message"] + assert error_codes.is_registered(env["error"]["code"]) + return env + + +# ─── the BE-4571 case: a flag token where the model alias belongs ──────── + + +def test_json_flag_shaped_target_emits_target_required_envelope(runner): + """`comfy --json generate --prompt=x` used to exit 1 with NO json on stdout + and empty stderr. It now names the real problem.""" + r = runner.invoke(cli_app, ["--json", "generate", "--prompt=x"]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_target_required" + assert "model alias" in env["error"]["message"] + # Actionable: it points at both the right generate invocation and the local + # alternative, so the caller isn't left at a dead end. + assert "run-template" in env["error"]["hint"] + + +@pytest.mark.parametrize("token", ["--prompt=x", "-p", "--width"]) +def test_flag_shaped_targets_all_rejected(runner, token): + r = runner.invoke(cli_app, ["--json", "generate", token]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_target_required" + + +@pytest.mark.parametrize("flag", ["-h", "--help"]) +def test_help_flags_are_not_treated_as_a_flag_shaped_target(runner, flag): + """The guard must not swallow the help short-circuit.""" + r = runner.invoke(cli_app, ["generate", flag]) + assert r.exit_code == 0 + assert "comfy generate" in r.stdout + + +def test_json_stream_target_required_envelope_is_the_terminal_line(runner): + r = runner.invoke(cli_app, ["--json-stream", "generate", "--prompt=x"]) + assert r.exit_code == 1 + last = json.loads(r.stdout.strip().splitlines()[-1]) + assert last["schema"] == "envelope/1" + assert last["ok"] is False + assert last["error"]["code"] == "generate_target_required" + + +def test_pretty_flag_shaped_target_stays_rich(runner): + r = runner.invoke(cli_app, ["--no-json", "generate", "--prompt=x"]) + assert r.exit_code == 1 + assert "model alias" in r.stdout + assert "run-template" in r.stdout + assert "envelope/1" not in r.stdout + + +# ─── the other JSON-mode error paths ───────────────────────────────────── + + +def test_json_unknown_model_envelope(runner, api_key): + r = runner.invoke(cli_app, ["--json", "generate", "not-a-real-model", "--prompt=x"]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_unknown_model" + assert env["error"]["details"]["model"] == "not-a-real-model" + assert env["error"]["hint"] # falls back to the registered hint + + +def test_json_missing_required_param_envelope(runner, api_key): + r = runner.invoke(cli_app, ["--json", "generate", "flux-pro", "--prompt", "x"]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_bad_args" + assert "comfy generate schema" in env["error"]["hint"] + + +def test_json_bad_timeout_envelope(runner, api_key): + r = runner.invoke( + cli_app, + ["--json", "generate", "flux-pro", "--prompt", "x", "--width", "1", "--height", "1", "--timeout", "nope"], + ) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_timeout_invalid" + + +def test_json_api_error_envelope_carries_status_and_body(runner, api_key, monkeypatch): + monkeypatch.setattr( + gen_app.client.httpx, + "post", + lambda *a, **kw: httpx.Response(401, json={"message": "Invalid token"}), + ) + r = runner.invoke(cli_app, ["--json", "generate", "flux-pro", "--prompt", "x", "--width", "1", "--height", "1"]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_api_error" + assert env["error"]["details"]["status"] == 401 + assert "Invalid token" in env["error"]["details"]["body"] + + +def test_json_network_error_envelope(runner, api_key, monkeypatch): + def boom(*a, **kw): + raise httpx.ConnectError("connection refused") + + monkeypatch.setattr(gen_app.client.httpx, "post", boom) + r = runner.invoke(cli_app, ["--json", "generate", "flux-pro", "--prompt", "x", "--width", "1", "--height", "1"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_network_error" + + +def test_json_schema_subcommand_unknown_model_envelope(runner): + r = runner.invoke(cli_app, ["--json", "generate", "schema", "not-a-real-model"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_unknown_model" + + +def test_json_schema_subcommand_usage_envelope(runner): + r = runner.invoke(cli_app, ["--json", "generate", "schema"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_bad_args" + + +def test_json_resume_usage_envelope(runner, api_key): + r = runner.invoke(cli_app, ["--json", "generate", "resume"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_bad_args" + + +def test_json_upload_usage_envelope(runner, api_key): + r = runner.invoke(cli_app, ["--json", "generate", "upload"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_bad_args" + + +def test_json_list_bad_meta_flag_envelope(runner): + """`--download` with no value used to escape `_list_models` as an unhandled + SchemaError traceback.""" + r = runner.invoke(cli_app, ["--json", "generate", "list", "--download"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_bad_args" + + +# ─── pretty-mode regression guards ─────────────────────────────────────── + + +def test_pretty_unknown_model_output_unchanged(runner, api_key): + r = runner.invoke(cli_app, ["--no-json", "generate", "not-a-real-model", "--prompt=x"]) + assert r.exit_code == 1 + assert "Unknown model" in r.stdout + assert "comfy generate list" in r.stdout + assert "envelope/1" not in r.stdout + + +def test_pretty_missing_required_still_suggests_schema(runner, api_key): + r = runner.invoke(cli_app, ["--no-json", "generate", "flux-pro", "--prompt", "x"]) + assert r.exit_code == 1 + assert "Missing required" in r.stdout + assert "comfy generate schema flux-pro" in r.stdout + assert "envelope/1" not in r.stdout + + +def _mock_failed_job(monkeypatch): + """Submit succeeds, the async poll settles on a non-succeeded terminal state.""" + submit = httpx.Response(200, json={"id": "job-xyz", "polling_url": "https://x/poll"}) + poll_fail = httpx.Response(200, json={"status": "Content Moderated", "progress": 0.0}) + monkeypatch.setattr(gen_app.client.httpx, "post", lambda *a, **kw: submit) + monkeypatch.setattr("comfy_cli.command.generate.client.get", lambda *a, **kw: poll_fail) + monkeypatch.setattr("comfy_cli.command.generate.poll._sleep", lambda *_: None) + + +_GEN_ARGS = ["generate", "flux-pro", "--prompt", "x", "--width", "1", "--height", "1"] + + +def test_json_failed_job_envelope(runner, api_key, monkeypatch): + """A terminal non-succeeded job is a failure, not a result: under global + `--json` (and no command-local `--json`) it gets an envelope, not the red + line + raw payload.""" + _mock_failed_job(monkeypatch) + r = runner.invoke(cli_app, ["--json", *_GEN_ARGS]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_job_failed" + assert env["error"]["details"]["response"]["status"] == "Content Moderated" + + +def test_pretty_failed_job_output_unchanged(runner, api_key, monkeypatch): + _mock_failed_job(monkeypatch) + r = runner.invoke(cli_app, ["--no-json", *_GEN_ARGS]) + assert r.exit_code == 1 + assert "failed" in r.stdout.lower() + assert "envelope/1" not in r.stdout diff --git a/tests/comfy_cli/command/generate/test_spend_gate.py b/tests/comfy_cli/command/generate/test_spend_gate.py index 7dcb952e..685a3066 100644 --- a/tests/comfy_cli/command/generate/test_spend_gate.py +++ b/tests/comfy_cli/command/generate/test_spend_gate.py @@ -69,10 +69,27 @@ def interactive_tty(monkeypatch): def test_json_without_consent_fails_closed_and_spends_nothing(runner, api_key, post_spy): + # CliRunner's stdout is a pipe, so the global renderer resolves to JSON and + # the gate's refusal arrives as an `envelope/1` error rather than the + # command-local `{"error": …, "code": …}` object. r = runner.invoke(cli_app, ["generate", "dalle", "--prompt", "x", "--json"]) assert r.exit_code == 1 assert post_spy == [] payload = json.loads(r.stdout) + assert payload["schema"] == "envelope/1" + assert payload["ok"] is False + assert payload["error"]["code"] == "spend_consent_required" + assert "--yes" in payload["error"]["message"] + + +def test_pretty_renderer_keeps_the_local_json_error_object(runner, api_key, post_spy): + """Global `--no-json` (a TTY-shaped run) + the command-local `--json`: the + pre-existing flat error object is unchanged — only the envelope-consuming + path was migrated.""" + r = runner.invoke(cli_app, ["--no-json", "generate", "dalle", "--prompt", "x", "--json"]) + assert r.exit_code == 1 + assert post_spy == [] + payload = json.loads(r.stdout) assert payload["code"] == "spend_consent_required" assert "--yes" in payload["error"] @@ -103,7 +120,7 @@ def test_gate_runs_before_auth_resolution(runner, post_spy): r = runner.invoke(cli_app, ["generate", "dalle", "--prompt", "x", "--json"]) assert r.exit_code == 1 assert post_spy == [] - assert json.loads(r.stdout)["code"] == "spend_consent_required" + assert json.loads(r.stdout)["error"]["code"] == "spend_consent_required" # ─── Bypasses: --yes flag and spend.auto_confirm config ─────────────────── From 9074a170d0382c70dcb7daf935af9ec0d51c332b Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 25 Jul 2026 18:36:10 -0700 Subject: [PATCH 2/4] fix(generate): resolve Cursor panel findings on the JSON-error migration (BE-4577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven consolidated-panel findings, each with a regression test that fails without its fix: - HIGH: `generate resume … --timeout nope` parsed with a bare `float()`, so the ValueError escaped `main()` (which traps only KeyboardInterrupt / typer.Exit / SystemExit) as a traceback — exit 1 with a blank stdout, the exact failure this PR exists to remove. Guarded like the submit path. - MED: the pollers called `resp.json()` unguarded on a 200, so a proxy/CDN HTML error page raised a bare ValueError that neither caller's `(ApiError, HTTPError)` tuple caught. `_decode_poll_body` now raises ApiError (and rejects a non-object body, which would have blown up on `.get()` a line later). - MED: the spend prompt was gated on the command-local `--json` only. With a TTY stdin and a machine stdout (`comfy generate … | jq`) the notice and the prompt went to the piped stdout — invisible to the human being asked, and spliced ahead of the caller's JSON. They go to stderr when the renderer is in JSON mode; the prompt itself is kept (a TTY user can still answer it, and reads it on the same terminal). Pretty mode is untouched. - LOW: `httpx.HTTPStatusError` subclasses HTTPError, so `upload_remote_url`'s `raise_for_status` on a 404 source URL was reported as `generate_network_error` — "check connectivity and retry", the wrong advice for a 4xx that reached the server. Now `generate_api_error`, with the tracking bucket following suit. - LOW: server-controlled text (an ApiError body, a response preview, a partner's failure reason) was interpolated into Rich markup, so a bracketed token either got swallowed (`[link]`) or raised MarkupError (`[/path]`), turning a clean error into a traceback. Escaped at every such site. - LOW: the poll failure was reported inside `with _spinner()`, where a transient Progress is still auto-refreshing stdout — on a TTY the envelope would interleave with spinner control codes. Reported after the block exits. - NIT: `_fail`'s docstring claimed byte-compatibility for legacy_json where `code` is in fact newly added; reworded. Also lists `generate_job_failed` in the SKILL.md error-code set, which the PR had omitted. Co-Authored-By: Claude Opus 5 --- comfy_cli/command/generate/app.py | 112 ++++++++++++++---- comfy_cli/command/generate/poll.py | 22 +++- comfy_cli/skills/comfy/SKILL.md | 5 +- .../command/generate/test_json_errors.py | 104 ++++++++++++++++ .../command/generate/test_spend_gate.py | 38 ++++++ 5 files changed, 257 insertions(+), 24 deletions(-) diff --git a/comfy_cli/command/generate/app.py b/comfy_cli/command/generate/app.py index e688e37b..cf5889ea 100644 --- a/comfy_cli/command/generate/app.py +++ b/comfy_cli/command/generate/app.py @@ -45,6 +45,7 @@ # JSON-stream mode -- an envelope-consuming caller must never get exit 1 with a # blank stdout. from rich import print as rprint +from rich.markup import escape from rich.progress import Progress, SpinnerColumn, TextColumn, TimeElapsedColumn from comfy_cli import constants, tracking, ui @@ -85,11 +86,22 @@ def _fail( renderer already resolves to JSON) → exactly one `envelope/1` error on stdout with a stable `code`. Without this an envelope-consuming caller (comfy-local-mcp, scripts, agents) sees exit 1 with nothing to read. - - `legacy_json` → the pre-existing command-local `--json` error object, kept - byte-compatible for the paths that already emitted one. + - `legacy_json` → the command-local `--json` error object. The `error` key + is byte-compatible with what those paths already emitted; `code` is added + (additively — the `_consent` unknown-action and `_schema` usage/unknown-model + paths previously emitted `error` alone) so a machine caller on the local + flag gets the same stable identifier the envelope carries. - Otherwise → the historical rich-red line (plus a dim hint line where the path already printed one), so pretty/TTY output is unchanged. + The default pretty line escapes `message` as Rich markup: several call sites + interpolate server-controlled text (an `ApiError`'s body, a response + preview, a partner's failure reason), and a bracketed token Rich reads as a + tag would either swallow the text or raise `MarkupError` — turning a clean + error into a traceback. A caller-supplied `pretty` is passed through + verbatim: it is explicitly pre-formatted markup and escapes its own + interpolations. + ``hint`` is only passed where pretty mode already printed that second line; everywhere else `renderer.error` falls back to the code's registered hint, which keeps pretty output identical while JSON callers still get navigation. @@ -106,17 +118,41 @@ def _fail( if legacy_json: output.print_json({"error": message, "code": code}) return - rprint(pretty if pretty is not None else f"[bold red]{message}[/bold red]") + rprint(pretty if pretty is not None else f"[bold red]{escape(message)}[/bold red]") if hint: rprint(f"[dim]{hint}[/dim]") def _transport_code(exc: BaseException) -> str: """`generate_network_error` for a transport failure, `generate_api_error` for - an HTTP/API-level one — the two arrive together on most call sites.""" + an HTTP/API-level one — the two arrive together on most call sites. + + `httpx.HTTPStatusError` subclasses `HTTPError` but is *not* a transport + failure: the request reached the server and came back non-2xx (e.g. + `upload_remote_url`'s `raise_for_status` on a 404 source URL). Calling that + a network error would tell automation to check connectivity and retry, which + is exactly the wrong move for a 4xx. + """ + if isinstance(exc, httpx.HTTPStatusError): + return "generate_api_error" return "generate_network_error" if isinstance(exc, httpx.HTTPError) else "generate_api_error" +def _notice(markup: str, *, err: bool) -> None: + """Print a human-facing Rich line to stdout, or to stderr when stdout is the + machine channel.""" + if err: + get_renderer().stderr_console().print(markup) + else: + rprint(markup) + + +def _track_kind(exc: BaseException) -> str: + """Tracking bucket matching `_transport_code` — an HTTP-status failure is an + API error, not a network one.""" + return "network" if _transport_code(exc) == "generate_network_error" else "api" + + def register_with(parent: typer.Typer) -> None: """Wire the ``generate`` command into a Typer app. We register directly (rather than as a sub-app via ``add_typer``) so the first positional after @@ -261,7 +297,9 @@ def _emit_result(result: poll.PollResult, *, request_id: str, download: str | No details={"status": result.status, "response": result.raw}, ) else: - rprint(f"[bold red]{message}[/bold red]") + # `result.error` is the partner's own text — escape it so a bracketed + # token isn't parsed as Rich markup. + rprint(f"[bold red]{escape(message)}[/bold red]") output.print_json(result.raw) raise typer.Exit(code=1) if download and result.image_urls: @@ -308,15 +346,27 @@ def _confirm_spend(*, model_name: str, assume_yes: bool, as_json: bool) -> None: if assume_yes or _spend_auto_confirmed(): return if as_json or not _stdin_is_tty(): + # `--json`/no-TTY: no prompt is answerable, so fail closed. msg = ( f"`comfy generate {model_name}` spends Comfy credits and no consent was given. " "Re-run with --yes, or persist consent with `comfy generate consent always`." ) _fail(code="spend_consent_required", message=msg, legacy_json=as_json) raise SpendNotConfirmed(msg) - rprint(f"[bold]{model_name}[/bold] runs via the partner API and [bold]spends Comfy credits[/bold].") - rprint("[dim]Skip this prompt with --yes; persist always-proceed with `comfy generate consent always`.[/dim]") - if not typer.confirm("Proceed?", default=False): + # A TTY stdin but a machine stdout (`comfy generate … | jq`) is a real + # combination, and the prompt is human I/O: written to a piped stdout it is + # invisible to the person being asked — they see a silent hang — and it + # splices human text ahead of whatever JSON the caller is parsing. Route the + # notice and the prompt to stderr in that case; a TTY user reads them + # exactly as before (same terminal) and stdout stays parseable. Pretty mode + # is untouched. + to_err = get_renderer().is_json() + _notice(f"[bold]{model_name}[/bold] runs via the partner API and [bold]spends Comfy credits[/bold].", err=to_err) + _notice( + "[dim]Skip this prompt with --yes; persist always-proceed with `comfy generate consent always`.[/dim]", + err=to_err, + ) + if not typer.confirm("Proceed?", default=False, err=to_err): # Reachable with a TTY stdin but a redirected (JSON-mode) stdout, so the # decline still owes the caller an envelope rather than a bare line. _fail( @@ -511,7 +561,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: code="generate_api_error", message=f"API error {e.status}", details={"status": e.status, "body": e.body}, - pretty=f"[bold red]API error {e.status}[/bold red]\n{e.body}", + pretty=f"[bold red]API error {e.status}[/bold red]\n{escape(str(e.body))}", ) _track_error("api", e) raise typer.Exit(code=1) from e @@ -535,7 +585,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: code="generate_api_error", message="Unexpected non-JSON response.", details={"body_preview": preview}, - pretty=f"[bold red]Unexpected non-JSON response.[/bold red]\n{preview}", + pretty=f"[bold red]Unexpected non-JSON response.[/bold red]\n{escape(preview)}", ) _track_error("non_json_response", e) raise typer.Exit(code=1) @@ -564,6 +614,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: return poller = poll.get_poller(ep.polling) + poll_error: client.ApiError | httpx.HTTPError | None = None with _spinner() as prog: task = prog.add_task(f"Generating with {name} (job {job_id})", total=None) @@ -579,11 +630,19 @@ def _on_progress(p: float) -> None: create_path=ep.path, ) except (client.ApiError, httpx.HTTPError) as e: - # The spinner is transient, so without this the run ended with - # a bare exit 1 and an empty screen in EVERY mode. - _fail(code=_transport_code(e), message=f"Job {job_id} failed while polling: {e}") - _track_error("network" if isinstance(e, httpx.HTTPError) else "api", e) - raise typer.Exit(code=1) from e + poll_error = e + if poll_error is not None: + # The spinner is transient, so without this the run ended with a + # bare exit 1 and an empty screen in EVERY mode. Reported AFTER + # the `with` exits: inside it a transient Progress is still + # auto-refreshing on stdout, so on a TTY the envelope would come + # out interleaved with spinner control codes. + _fail( + code=_transport_code(poll_error), + message=f"Job {job_id} failed while polling: {poll_error}", + ) + _track_error(_track_kind(poll_error), poll_error) + raise typer.Exit(code=1) from poll_error try: _emit_result(result, request_id=job_id, download=download, as_json=as_json) tracking.track_event("generate:success", gen_props) @@ -880,7 +939,16 @@ def _resume(extra_args: list[str]) -> None: except client.ApiError as e: _fail(code="generate_api_error", message=str(e)) raise typer.Exit(code=1) - timeout = float(meta.get("timeout") or 300.0) if isinstance(meta.get("timeout"), str) else 300.0 + timeout_raw = meta.get("timeout") + try: + # Same guard as the submit path: an unguarded `float()` here let + # `generate resume --timeout nope` escape `main()` (which + # only traps KeyboardInterrupt/typer.Exit/SystemExit) as a traceback — + # exit 1 with a blank stdout, the exact failure this change removes. + timeout = float(timeout_raw or 300.0) if isinstance(timeout_raw, str) else 300.0 + except ValueError: + _fail(code="generate_timeout_invalid", message=f"--timeout: expected number, got {timeout_raw!r}") + raise typer.Exit(code=1) download = meta.get("download") if isinstance(meta.get("download"), str) else None as_json = bool(meta.get("json", False)) @@ -891,6 +959,7 @@ def _resume(extra_args: list[str]) -> None: raise typer.Exit(code=1) poller = poll.get_poller(ep.polling) + poll_error: client.ApiError | httpx.HTTPError | None = None with _spinner() as prog: task = prog.add_task(f"Resuming job {job_id}", total=None) @@ -906,10 +975,13 @@ def _on_progress(p: float) -> None: create_path=ep.path, ) except (client.ApiError, httpx.HTTPError) as e: - # Same transient-spinner trap as the submit path: an unhandled poll - # failure here used to surface as a raw traceback. - _fail(code=_transport_code(e), message=f"Job {job_id} failed while polling: {e}") - raise typer.Exit(code=1) from e + poll_error = e + if poll_error is not None: + # Same transient-spinner trap as the submit path: an unhandled poll + # failure here used to surface as a raw traceback, and reporting it + # inside the `with` would interleave the envelope with the live spinner. + _fail(code=_transport_code(poll_error), message=f"Job {job_id} failed while polling: {poll_error}") + raise typer.Exit(code=1) from poll_error _emit_result(result, request_id=job_id, download=download, as_json=as_json) diff --git a/comfy_cli/command/generate/poll.py b/comfy_cli/command/generate/poll.py index 2e576edb..11629a90 100644 --- a/comfy_cli/command/generate/poll.py +++ b/comfy_cli/command/generate/poll.py @@ -110,6 +110,24 @@ def _sleep(seconds: float) -> None: time.sleep(seconds) +def _decode_poll_body(resp: httpx.Response) -> dict[str, Any]: + """Parse a 200 poll response, turning an unparseable body into an ``ApiError``. + + A proxy/CDN that answers 200 with an HTML error page (or an empty body) + used to raise a bare ``ValueError`` out of ``resp.json()``, which is neither + ``ApiError`` nor ``httpx.HTTPError`` — so every caller's ``except`` tuple + missed it and the run died as an unhandled traceback. Raising ``ApiError`` + puts it back on the path that already reports poll failures. + """ + try: + body = resp.json() + except ValueError as e: + raise client.ApiError(resp.status_code, resp.text, f"Poll response was not JSON: {resp.text[:200]!r}") from e + if not isinstance(body, dict): + raise client.ApiError(resp.status_code, resp.text, f"Poll response was not a JSON object: {body!r}") + return body + + def poll_bfl( initial: dict[str, Any], api_key: str, @@ -129,7 +147,7 @@ def poll_bfl( resp = client.get(url, api_key=api_key) if resp.status_code >= 400: client.raise_for_status(resp) - last_body = resp.json() + last_body = _decode_poll_body(resp) status = str(last_body.get("status", "")).strip() if on_progress is not None: progress = last_body.get("progress") @@ -282,7 +300,7 @@ def poll_generic( resp = client.get(url, api_key=api_key) if resp.status_code >= 400: client.raise_for_status(resp) - last_body = resp.json() + last_body = _decode_poll_body(resp) if on_progress is not None and spec.progress_path: p = _dotget(last_body, spec.progress_path) if isinstance(p, int | float): diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index 6cc0b23c..ec328523 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -674,8 +674,9 @@ Mechanical contracts that bite agents — encode them, don't rediscover: failed/malformed `comfy --json generate …` emits exactly one `envelope/1` error on stdout with a stable code — `generate_target_required`, `generate_unknown_model`, `generate_bad_args`, `generate_timeout_invalid`, - `generate_api_error`, `generate_network_error`, `generate_spec_invalid`, - `spend_consent_required` — so branch on `error.code`, never on the text. + `generate_api_error`, `generate_network_error`, `generate_job_failed`, + `generate_spec_invalid`, `spend_consent_required` — so branch on + `error.code`, never on the text. (Success payloads are unchanged: still the raw API response, not an envelope.) In particular `comfy generate --prompt "…"` with no model alias is `generate_target_required`: `generate` is a paid cloud/partner verb and diff --git a/tests/comfy_cli/command/generate/test_json_errors.py b/tests/comfy_cli/command/generate/test_json_errors.py index 13d54551..54dca41c 100644 --- a/tests/comfy_cli/command/generate/test_json_errors.py +++ b/tests/comfy_cli/command/generate/test_json_errors.py @@ -240,3 +240,107 @@ def test_pretty_failed_job_output_unchanged(runner, api_key, monkeypatch): assert r.exit_code == 1 assert "failed" in r.stdout.lower() assert "envelope/1" not in r.stdout + + +# ─── review follow-ups: the remaining traceback / mis-code paths ───────── + + +def test_json_resume_bad_timeout_envelope(runner, api_key): + """`generate resume … --timeout nope` parsed the value with a bare `float()`, + so the `ValueError` escaped `main()` (which traps only KeyboardInterrupt / + typer.Exit / SystemExit) as a traceback — exit 1, blank stdout, the exact + shape this change exists to remove. The submit path already guarded it.""" + r = runner.invoke(cli_app, ["--json", "generate", "resume", "flux-pro", "job-1", "--timeout", "nope"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_timeout_invalid" + + +def test_json_non_json_poll_body_envelope(runner, api_key, monkeypatch): + """A 200 poll response with an unparseable body (a proxy/CDN HTML error + page) raised a bare `ValueError` out of `resp.json()` — neither `ApiError` + nor `httpx.HTTPError`, so every caller's `except` tuple missed it.""" + submit = httpx.Response(200, json={"id": "job-xyz", "polling_url": "https://x/poll"}) + monkeypatch.setattr(gen_app.client.httpx, "post", lambda *a, **kw: submit) + monkeypatch.setattr( + "comfy_cli.command.generate.client.get", + lambda *a, **kw: httpx.Response(200, text="502 Bad Gateway"), + ) + monkeypatch.setattr("comfy_cli.command.generate.poll._sleep", lambda *_: None) + r = runner.invoke(cli_app, ["--json", *_GEN_ARGS]) + assert r.exit_code == 1 + env = _sole_envelope(r) + assert env["error"]["code"] == "generate_api_error" + assert "not JSON" in env["error"]["message"] + + +def test_json_non_dict_poll_body_envelope(runner, api_key, monkeypatch): + """Valid JSON that isn't an object would blow up on `.get()` a line later.""" + submit = httpx.Response(200, json={"id": "job-xyz", "polling_url": "https://x/poll"}) + monkeypatch.setattr(gen_app.client.httpx, "post", lambda *a, **kw: submit) + monkeypatch.setattr( + "comfy_cli.command.generate.client.get", + lambda *a, **kw: httpx.Response(200, json=["not", "an", "object"]), + ) + monkeypatch.setattr("comfy_cli.command.generate.poll._sleep", lambda *_: None) + r = runner.invoke(cli_app, ["--json", *_GEN_ARGS]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_api_error" + + +def test_http_status_error_is_an_api_error_not_a_network_one(runner, api_key, monkeypatch): + """`upload_remote_url` calls `raise_for_status`, so a 404 source URL arrives + as `httpx.HTTPStatusError` — an `httpx.HTTPError` subclass. Reporting that + as `generate_network_error` tells automation to check connectivity and + retry, which is the wrong move for a 4xx that reached the server fine.""" + req = httpx.Request("GET", "https://host/missing.png") + err = httpx.HTTPStatusError("404", request=req, response=httpx.Response(404, request=req)) + monkeypatch.setattr(gen_app.upload, "upload_target", lambda *a, **kw: (_ for _ in ()).throw(err)) + r = runner.invoke(cli_app, ["--json", "generate", "upload", "https://host/missing.png"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_api_error" + + +def test_transport_error_stays_a_network_error(runner, api_key, monkeypatch): + """The sibling of the above: a genuine transport failure keeps its code.""" + err = httpx.ConnectError("connection refused") + monkeypatch.setattr(gen_app.upload, "upload_target", lambda *a, **kw: (_ for _ in ()).throw(err)) + r = runner.invoke(cli_app, ["--json", "generate", "upload", "https://host/x.png"]) + assert r.exit_code == 1 + assert _sole_envelope(r)["error"]["code"] == "generate_network_error" + + +def test_pretty_server_text_with_rich_tags_does_not_raise_markup_error(runner, api_key, monkeypatch): + """Server-controlled text is interpolated into the pretty error line, and a + bracketed token Rich reads as a closing tag (`[/path]`) raised MarkupError — + turning a clean error into a traceback.""" + monkeypatch.setattr( + gen_app.client.httpx, + "post", + lambda *a, **kw: httpx.Response(400, json={"detail": "bad input at [/path] and [link]"}), + ) + r = runner.invoke(cli_app, ["--no-json", *_GEN_ARGS]) + assert r.exit_code == 1 + assert r.exception is None or isinstance(r.exception, SystemExit) + # The literal text survives instead of being eaten as markup. + assert "[/path]" in r.stdout + assert "[link]" in r.stdout + + +def test_pretty_failed_job_reason_with_rich_tags_survives(capsys): + """Same hazard on the partner's terminal-failure reason, which `_emit_result` + interpolates into its own red line rather than going through `_fail`.""" + import typer + + from comfy_cli.output.renderer import Renderer, get_renderer, reset_renderer_for_testing, set_renderer + + set_renderer(Renderer.resolve(no_json_flag=True, env={}, is_stdout_tty=True)) + try: + assert not get_renderer().is_json() + result = gen_app.poll.PollResult( + status="failed", raw={"status": "x"}, image_urls=[], error="moderated at [/path]" + ) + with pytest.raises(typer.Exit): + gen_app._emit_result(result, request_id="job-1", download=None, as_json=False) + finally: + reset_renderer_for_testing() + assert "[/path]" in capsys.readouterr().out diff --git a/tests/comfy_cli/command/generate/test_spend_gate.py b/tests/comfy_cli/command/generate/test_spend_gate.py index 685a3066..7e18e836 100644 --- a/tests/comfy_cli/command/generate/test_spend_gate.py +++ b/tests/comfy_cli/command/generate/test_spend_gate.py @@ -267,3 +267,41 @@ def test_consent_unknown_action_errors(runner): r = runner.invoke(cli_app, ["generate", "consent", "sometimes"]) assert r.exit_code == 1 assert "Unknown consent action" in r.stdout + + +# ─── TTY stdin + machine stdout: prompt on stderr, stdout stays parseable ── + + +def test_interactive_prompt_goes_to_stderr_when_stdout_is_machine(api_key, post_spy, interactive_tty): + """`comfy generate … | jq` from a terminal: stdin is a TTY (so the human can + answer) but stdout is the machine channel. Written to stdout the prompt is + invisible — the person sees a silent hang — and it splices human text ahead + of the JSON the caller parses. It belongs on stderr; a TTY user reads it on + the same terminal either way.""" + split = CliRunner(mix_stderr=False) + r = split.invoke(cli_app, ["generate", "dalle", "--prompt", "x"], input="n\n") + assert r.exit_code == 1 + assert post_spy == [] + # stdout carries the decline envelope and no human text. (CliRunner echoes + # the piped keystroke back onto stdout; a real terminal echoes to the tty, + # so drop that artifact rather than pin it.) + lines = [ln for ln in r.stdout.strip().splitlines() if ln.strip() and ln.strip() != "n"] + assert "spends Comfy credits" not in r.stdout + assert "Proceed?" not in r.stdout + assert len(lines) == 1, f"stdout was polluted with human text: {lines!r}" + payload = json.loads(lines[0]) + assert payload["schema"] == "envelope/1" + assert payload["error"]["code"] == "spend_consent_required" + # stderr: the notice and the prompt the human actually needs to see. + assert "spends Comfy credits" in r.stderr + assert "Proceed?" in r.stderr + + +def test_pretty_interactive_prompt_stays_on_stdout(api_key, post_spy, interactive_tty): + """Pretty mode (a plain TTY run) is untouched: notice and prompt on stdout.""" + split = CliRunner(mix_stderr=False) + r = split.invoke(cli_app, ["--no-json", "generate", "dalle", "--prompt", "x"], input="n\n") + assert r.exit_code == 1 + assert post_spy == [] + assert "spends Comfy credits" in r.stdout + assert "no credits were spent" in r.stdout From 522380b1cba1581d2d638ff88ac966f818011a27 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 29 Jul 2026 13:02:59 -0700 Subject: [PATCH 3/4] test(generate): pin one-error-object contract on the --json-stream envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged that the json-stream test only checked the LAST stdout line, so a duplicate or legacy flat `{"error": ...}` object earlier in the stream would slip through the suite's own stated contract. Assert on every parsed line instead: exactly one object carries an `error` key, and it is the terminal line. Kept the terminal-line semantic rather than switching to the suite's `_sole_envelope` helper — stream mode may legitimately precede the envelope with progress events, so a sole-line assertion would over-constrain the format; the defect CodeRabbit named is a SECOND error object, and that is what this now catches. Co-Authored-By: Claude Opus 5 --- tests/comfy_cli/command/generate/test_json_errors.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tests/comfy_cli/command/generate/test_json_errors.py b/tests/comfy_cli/command/generate/test_json_errors.py index 54dca41c..cd1f5e77 100644 --- a/tests/comfy_cli/command/generate/test_json_errors.py +++ b/tests/comfy_cli/command/generate/test_json_errors.py @@ -92,7 +92,15 @@ def test_help_flags_are_not_treated_as_a_flag_shaped_target(runner, flag): def test_json_stream_target_required_envelope_is_the_terminal_line(runner): r = runner.invoke(cli_app, ["--json-stream", "generate", "--prompt=x"]) assert r.exit_code == 1 - last = json.loads(r.stdout.strip().splitlines()[-1]) + objs = [json.loads(ln) for ln in r.stdout.strip().splitlines() if ln.strip()] + # Stream mode may legitimately precede the envelope with progress events, so + # the contract is terminal-line, not sole-line. What must NOT happen is a + # SECOND error object — the legacy flat `{"error": ...}` alongside the + # envelope would leave a caller reading whichever it hit first. + errors = [o for o in objs if "error" in o] + assert len(errors) == 1, f"expected exactly one error object, got {errors!r}" + last = objs[-1] + assert errors[0] is last assert last["schema"] == "envelope/1" assert last["ok"] is False assert last["error"]["code"] == "generate_target_required" From 97284b64602134d1440773ca2d2ef65c4415d5bb Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 29 Jul 2026 14:01:15 -0700 Subject: [PATCH 4/4] fix(generate): sanitize remote text in pretty error paths (BE-4577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on the `envelope/1` migration. CodeRabbit flagged the two new usage lines as passing `[--json]` / `[--download PATH]` to Rich as unparseable style tags. That specific claim does not reproduce: Rich's tag regex requires `[a-z#/@]` after the bracket, so a `-`-led token is literal text. But it pointed at real duplication — both `pretty=` overrides were byte-for-byte what `_fail`'s default branch already renders, so they are dropped rather than escaped. Output is unchanged (verified against rendered ANSI) and the text now routes through the escaping path, making the hazard structurally impossible. Auditing the rest of those pretty paths against #614 turned up the real bug. `rich.markup.escape` neutralizes `[tag]` but passes `\x1b` straight through, and `generate` prints via a bare `rich.print`, so it gets none of `Renderer`'s automatic sanitization. Three sites interpolated remote text with escaping only — an `ApiError` body, the non-JSON response preview, and the partner's job-failure reason — letting a partner response clear the user's screen, rewrite the window title, or repaint earlier lines to spoof CLI output. All now use `sanitize_markup`; the JSON paths deliberately keep raw bytes, since `json.dumps` already encodes them inert and stripping there would mutate what agents parse. Self-review found one more, reachable today: `generate schema '[/bold]'` died with a MarkupError traceback and empty stdout, because the pretty branch interpolated a `SpecError` quoting the alias verbatim. That line came in with #621 rather than this PR, but it is the same defect class in the same module and exactly the exit-1-with-nothing-to-read failure this migration exists to remove, so it is fixed here too. Tests: every new assertion was mutation-checked to fail without its fix. Also pins that the JSON envelope does NOT sanitize, so a future "sanitize everywhere" change cannot silently corrupt envelope data. Co-Authored-By: Claude Opus 5 --- comfy_cli/command/generate/app.py | 53 +++++---- .../command/generate/test_app_lifecycle.py | 24 ++++ .../command/generate/test_json_errors.py | 103 ++++++++++++++++++ 3 files changed, 161 insertions(+), 19 deletions(-) diff --git a/comfy_cli/command/generate/app.py b/comfy_cli/command/generate/app.py index b86f0a97..7d26db85 100644 --- a/comfy_cli/command/generate/app.py +++ b/comfy_cli/command/generate/app.py @@ -45,13 +45,13 @@ # JSON-stream mode -- an envelope-consuming caller must never get exit 1 with a # blank stdout. from rich import print as rprint -from rich.markup import escape from rich.progress import Progress, SpinnerColumn, TextColumn, TimeElapsedColumn from comfy_cli import constants, tracking, ui from comfy_cli.command.generate import adapters, client, emit, output, poll, schema, spec, upload from comfy_cli.config_manager import ConfigManager from comfy_cli.output.renderer import Renderer, get_renderer +from comfy_cli.output.sanitize import sanitize_markup _HELP = "Generate images via ComfyUI partner nodes (Flux, Ideogram, DALL·E, Recraft, Stability, …)." @@ -80,7 +80,7 @@ def _fail( legacy_json: bool = False, pretty: str | None = None, ) -> None: - """Report a `generate` failure through whichever channel the caller asked for. + r"""Report a `generate` failure through whichever channel the caller asked for. - Global `--json` / `--json-stream` (and any non-TTY stdout, which the renderer already resolves to JSON) → exactly one `envelope/1` error on @@ -94,13 +94,24 @@ def _fail( - Otherwise → the historical rich-red line (plus a dim hint line where the path already printed one), so pretty/TTY output is unchanged. - The default pretty line escapes `message` as Rich markup: several call sites - interpolate server-controlled text (an `ApiError`'s body, a response - preview, a partner's failure reason), and a bracketed token Rich reads as a - tag would either swallow the text or raise `MarkupError` — turning a clean - error into a traceback. A caller-supplied `pretty` is passed through - verbatim: it is explicitly pre-formatted markup and escapes its own - interpolations. + The default pretty line runs `message` (and `hint`) through `sanitize_markup`: + several call sites interpolate server-controlled text (an `ApiError`'s body, a + response preview, a partner's failure reason). Markup escaping alone is not + enough for remote text — it stops a bracketed token from being read as a tag + (which would swallow the text or raise `MarkupError`), but `\x1b` survives it, + so a CSI/OSC sequence would reach the terminal and could clear the screen or + repaint earlier lines to spoof CLI output. `sanitize_markup` strips the escape + bytes *and* escapes markup; see `comfy_cli.output.sanitize` (#614), which this + module must call explicitly because `generate` prints via a bare `rich.print` + rather than through `Renderer`. + + A caller-supplied `pretty` is passed through verbatim: it is explicitly + pre-formatted markup and is responsible for sanitizing its own interpolations + (the two that carry remote text do). + + The JSON/NDJSON paths above deliberately do NOT sanitize: `json.dumps` encodes + `\x1b` as a `\u` escape already, and stripping there would mutate the data + agents parse. ``hint`` is only passed where pretty mode already printed that second line; everywhere else `renderer.error` falls back to the code's registered hint, @@ -118,9 +129,9 @@ def _fail( if legacy_json: output.print_json({"error": message, "code": code}) return - rprint(pretty if pretty is not None else f"[bold red]{escape(message)}[/bold red]") + rprint(pretty if pretty is not None else f"[bold red]{sanitize_markup(message)}[/bold red]") if hint: - rprint(f"[dim]{hint}[/dim]") + rprint(f"[dim]{sanitize_markup(hint)}[/dim]") def _transport_code(exc: BaseException) -> str: @@ -297,9 +308,11 @@ def _emit_result(result: poll.PollResult, *, request_id: str, download: str | No details={"status": result.status, "response": result.raw}, ) else: - # `result.error` is the partner's own text — escape it so a bracketed - # token isn't parsed as Rich markup. - rprint(f"[bold red]{escape(message)}[/bold red]") + # `result.error` is the partner's own text. `sanitize_markup` (not a bare + # `escape`) because it is remote: escaping alone stops a bracketed token + # from being parsed as Rich markup but passes `\x1b` straight through, + # letting the partner clear the screen or repaint earlier lines (#614). + rprint(f"[bold red]{sanitize_markup(message)}[/bold red]") output.print_json(result.raw) raise typer.Exit(code=1) if download and result.image_urls: @@ -561,7 +574,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: code="generate_api_error", message=f"API error {e.status}", details={"status": e.status, "body": e.body}, - pretty=f"[bold red]API error {e.status}[/bold red]\n{escape(str(e.body))}", + pretty=f"[bold red]API error {e.status}[/bold red]\n{sanitize_markup(e.body)}", ) _track_error("api", e) raise typer.Exit(code=1) from e @@ -585,7 +598,7 @@ def _track_error(error_kind: str, exc: BaseException) -> None: code="generate_api_error", message="Unexpected non-JSON response.", details={"body_preview": preview}, - pretty=f"[bold red]Unexpected non-JSON response.[/bold red]\n{escape(preview)}", + pretty=f"[bold red]Unexpected non-JSON response.[/bold red]\n{sanitize_markup(preview)}", ) _track_error("non_json_response", e) raise typer.Exit(code=1) @@ -815,7 +828,11 @@ def _schema(extra_args: list[str]) -> None: ep = spec.get_endpoint(clean[0]) except spec.SpecError as e: if renderer.is_pretty(): - rprint(f"[bold red]{e}[/bold red]") + # The message embeds `clean[0]` verbatim, so `comfy generate schema + # '[/bold]'` reached Rich as an unbalanced closing tag and died with a + # MarkupError traceback and empty stdout — the exact failure this + # module's `_fail` path exists to prevent. + rprint(f"[bold red]{sanitize_markup(e)}[/bold red]") else: renderer.error( code="generate_unknown_model", @@ -900,7 +917,6 @@ def _upload(extra_args: list[str]) -> None: _fail( code="generate_bad_args", message="Usage: comfy generate upload [--json]", - pretty="[bold red]Usage: comfy generate upload [--json][/bold red]", ) raise typer.Exit(code=1) target = remaining[0] @@ -974,7 +990,6 @@ def _resume(extra_args: list[str]) -> None: _fail( code="generate_bad_args", message="Usage: comfy generate resume [--download PATH] [--json]", - pretty="[bold red]Usage: comfy generate resume [--download PATH] [--json][/bold red]", ) raise typer.Exit(code=1) model, job_id = extra_args[0], extra_args[1] diff --git a/tests/comfy_cli/command/generate/test_app_lifecycle.py b/tests/comfy_cli/command/generate/test_app_lifecycle.py index a8cbb0a3..3e171395 100644 --- a/tests/comfy_cli/command/generate/test_app_lifecycle.py +++ b/tests/comfy_cli/command/generate/test_app_lifecycle.py @@ -92,6 +92,30 @@ def test_resume_flag_like_job_id_surfaces_usage_error(self, runner, captured_eve assert r.exit_code == 1 assert "Usage: comfy generate resume" in r.output + @pytest.mark.parametrize( + ("argv", "usage"), + [ + (["--no-json", "generate", "upload"], "Usage: comfy generate upload [--json]"), + ( + ["--no-json", "generate", "resume"], + "Usage: comfy generate resume [--download PATH] [--json]", + ), + ], + ) + def test_usage_errors_render_bracketed_flags_literally(self, runner, captured_events, argv, usage): + # `--no-json` is load-bearing: under CliRunner stdout is not a TTY, so the + # renderer defaults to JSON (renderer.py precedence rule 6) and the assertion + # would pass on the envelope's `message` field without ever rendering markup. + # The pretty branch is what an interactive user actually sees, and it feeds + # the text through Rich — `[--json]` / `[--download PATH]` survive only + # because `_fail` runs `message` through `sanitize_markup`. A hand-rolled + # unescaped `pretty=` override would swallow those brackets (or raise + # MarkupError on a tag-shaped token); this pins the rendered line so that + # regresses loudly. + r = runner.invoke(cli_app, argv) + assert r.exit_code == 1 + assert usage in r.output + def test_refresh_fires_generate_refresh(self, runner, captured_events, monkeypatch): # Mock the httpx call so we don't actually hit the network. monkeypatch.setattr( diff --git a/tests/comfy_cli/command/generate/test_json_errors.py b/tests/comfy_cli/command/generate/test_json_errors.py index cd1f5e77..863e8f5f 100644 --- a/tests/comfy_cli/command/generate/test_json_errors.py +++ b/tests/comfy_cli/command/generate/test_json_errors.py @@ -352,3 +352,106 @@ def test_pretty_failed_job_reason_with_rich_tags_survives(capsys): finally: reset_renderer_for_testing() assert "[/path]" in capsys.readouterr().out + + +# ─── #614: markup escaping alone is not enough for remote text ─────────── +# +# `rich.markup.escape` neutralizes `[tag]` but passes `\x1b` through untouched, so +# these paths need `sanitize_markup`. Without it a partner API can emit CSI/OSC and +# clear the user's screen, rewrite the window title, or repaint earlier lines to +# spoof CLI output. `generate` prints via a bare `rich.print`, so it gets none of +# `Renderer`'s automatic coverage and must sanitize at each site itself. + +# ESC[2J clears the screen; ESC]0;…BEL rewrites the window title; ESC[31m restyles. +_ANSI_ATTACK = "denied \x1b[2J\x1b]0;pwned\x07 really \x1b[31mred" + + +def _assert_ansi_neutralized(text: str) -> None: + assert "\x1b[2J" not in text, "screen-clear sequence reached the terminal" + assert "\x1b]0;" not in text, "window-title sequence reached the terminal" + assert "\x1b" not in text + # The human-readable words survive — only the control bytes are dropped. + assert "denied" in text and "really" in text + + +def test_pretty_api_error_body_strips_ansi(runner, api_key, monkeypatch): + """`content=` (not `json=`) on purpose: `e.body` is the response's *raw text*, so + a compliant server that encodes ESC as `\\u001b` is already inert and would make + this test vacuous. The hazard is a body carrying live escape bytes, which is + exactly what a non-JSON error page from a proxy or gateway delivers.""" + monkeypatch.setattr( + gen_app.client.httpx, + "post", + lambda *a, **kw: httpx.Response(400, content=_ANSI_ATTACK.encode()), + ) + r = runner.invoke(cli_app, ["--no-json", *_GEN_ARGS]) + assert r.exit_code == 1 + _assert_ansi_neutralized(r.stdout) + + +def test_pretty_non_json_response_preview_strips_ansi(runner, api_key, monkeypatch): + """The `resp.text[:500]` preview path — a 200 whose body isn't JSON. Same raw-byte + exposure as above, reached through a different `_fail` call site.""" + monkeypatch.setattr( + gen_app.client.httpx, + "post", + lambda *a, **kw: httpx.Response(200, content=_ANSI_ATTACK.encode()), + ) + r = runner.invoke(cli_app, ["--no-json", *_GEN_ARGS]) + assert r.exit_code == 1 + _assert_ansi_neutralized(r.stdout) + + +def test_pretty_failed_job_reason_strips_ansi(capsys): + """The `_emit_result` red line, which builds its own markup instead of `_fail`'s.""" + import typer + + from comfy_cli.output.renderer import Renderer, get_renderer, reset_renderer_for_testing, set_renderer + + set_renderer(Renderer.resolve(no_json_flag=True, env={}, is_stdout_tty=True)) + try: + assert not get_renderer().is_json() + result = gen_app.poll.PollResult(status="failed", raw={"status": "x"}, image_urls=[], error=_ANSI_ATTACK) + with pytest.raises(typer.Exit): + gen_app._emit_result(result, request_id="job-1", download=None, as_json=False) + finally: + reset_renderer_for_testing() + _assert_ansi_neutralized(capsys.readouterr().out) + + +def test_pretty_schema_unknown_model_with_markup_does_not_traceback(runner): + """`generate schema`'s pretty branch interpolated the `SpecError` — which quotes + the requested alias verbatim — straight into markup, so a name Rich reads as an + unbalanced closing tag crashed with `MarkupError` and printed NOTHING. Exit 1 + with empty stdout is the undiagnosable failure this module is meant to prevent, + so it is pinned here even though the line arrived via #621 rather than this PR. + """ + r = runner.invoke(cli_app, ["--no-json", "generate", "schema", "[/bold]"]) + assert r.exit_code == 1 + assert r.exception is None or isinstance(r.exception, SystemExit), r.exception + # Something actionable actually reached the user. + assert r.stdout.strip() + assert "[/bold]" in r.stdout + + +def test_json_envelope_preserves_ansi_bytes(runner, api_key, monkeypatch): + """The machine path must NOT sanitize: `json.dumps` already encodes `\\x1b` as a + `\\u001b` escape, which is inert on the wire, and stripping it here would + silently mutate the bytes an agent parses (and diverge from the raw response + it may be diffing against). Pins the pretty/machine split so a future + 'sanitize everywhere' change can't quietly corrupt envelope data.""" + monkeypatch.setattr( + gen_app.client.httpx, + "post", + lambda *a, **kw: httpx.Response(400, content=_ANSI_ATTACK.encode()), + ) + r = runner.invoke(cli_app, ["--json", *_GEN_ARGS]) + assert r.exit_code == 1 + env = _sole_envelope(r) + # Encoded, not emitted: the envelope line carries no live escape byte, so nothing + # acts on the terminal even though the data is fully intact. + assert "\x1b" not in r.stdout + assert "\\u001b" in r.stdout + # And the decoded value is the partner's string byte-for-byte — sanitizing here + # would have silently dropped the ESCs from what the agent parses. + assert env["error"]["details"]["body"] == _ANSI_ATTACK