diff --git a/comfy_cli/command/generate/app.py b/comfy_cli/command/generate/app.py index 5657ccf8..7d26db85 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 @@ -45,6 +51,7 @@ 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, …)." @@ -54,6 +61,108 @@ "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: + 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 + 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 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 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, + 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]{sanitize_markup(message)}[/bold red]") + if hint: + rprint(f"[dim]{sanitize_markup(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. + + `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 @@ -75,6 +184,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 +295,25 @@ 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: + # `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: saved = output.save_urls(result.image_urls, download, request_id) @@ -221,19 +359,34 @@ 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`." ) - 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.") + # 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( + 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 +397,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 +455,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 +465,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 +483,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 +540,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 +548,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 +563,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{sanitize_markup(e.body)}", + ) _track_error("api", e) raise typer.Exit(code=1) from e @@ -435,8 +593,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{sanitize_markup(preview)}", + ) _track_error("non_json_response", e) raise typer.Exit(code=1) @@ -464,6 +627,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) @@ -479,8 +643,19 @@ def _on_progress(p: float) -> None: create_path=ep.path, ) except (client.ApiError, httpx.HTTPError) as 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) @@ -592,7 +767,13 @@ def _param_record(f: schema.FlagDef) -> dict[str, object]: 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) renderer = _renderer_for(meta) partner = _arg_value(clean, "--partner", "-p") category = _arg_value(clean, "--category", "--style", "-c") @@ -625,14 +806,18 @@ 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) renderer = _renderer_for(meta) if not clean or clean[0].startswith("-"): if renderer.is_pretty(): rprint("[bold red]Usage: comfy generate schema [/bold red]") else: renderer.error( - code="missing_argument", + code="generate_bad_args", message="Usage: comfy generate schema ", hint="pass a model alias, e.g. `comfy generate schema flux-pro` " "(run `comfy generate list` to see them)", @@ -643,10 +828,14 @@ 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_model_unknown", + code="generate_unknown_model", message=str(e), hint="run `comfy generate list` to see the available model aliases", details={"requested": clean[0]}, @@ -698,7 +887,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 @@ -708,7 +897,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) @@ -720,24 +909,27 @@ 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]", + ) 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( @@ -795,52 +987,76 @@ 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]", + ) 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_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) - 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 as_json = bool(meta.get("json", False)) 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) + poll_error: client.ApiError | httpx.HTTPError | None = None with _spinner() as prog: task = prog.add_task(f"Resuming job {job_id}", total=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: + 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/error_codes.py b/comfy_cli/error_codes.py index fd2cc0e9..b2e1dc80 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -630,10 +630,55 @@ class ErrorCode: ), # --- generate / emit ----------------------------------------------------- ErrorCode( - "generate_model_unknown", - "`comfy generate schema ` got a name that is neither a known alias nor a curated " - "endpoint id. `details.requested` carries the name as typed; the message lists close matches.", - "run `comfy generate list` to see the available model aliases", + "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", diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index ab7b0f15..a65b7ee8 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -669,8 +669,8 @@ Mechanical contracts that bite agents — encode them, don't rediscover: `generate_schema` in `comfy discover`). `data.models[]` carries `{alias, id, partner, category, mode, summary}` with the **untruncated** summary; `data.params[]` carries `{name, type, required, default, enum, - description}`. Unknown model → `generate_model_unknown`; missing model arg → - `missing_argument`. This catalog is **not** in `comfy discover` — `generate + description}`. Unknown model → `generate_unknown_model`; missing model arg → + `generate_bad_args`. This catalog is **not** in `comfy discover` — `generate list` is the only source of the alias list. - **Machine-readable output:** `generate --json` prints the raw API response as JSON; `generate upload --json` emits structured @@ -679,6 +679,18 @@ 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_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 + 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_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 new file mode 100644 index 00000000..863e8f5f --- /dev/null +++ b/tests/comfy_cli/command/generate/test_json_errors.py @@ -0,0 +1,457 @@ +"""`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 + 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" + + +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 + + +# ─── 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 + + +# ─── #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 diff --git a/tests/comfy_cli/command/generate/test_list_schema_envelope.py b/tests/comfy_cli/command/generate/test_list_schema_envelope.py index f8fa26b4..970b2bea 100644 --- a/tests/comfy_cli/command/generate/test_list_schema_envelope.py +++ b/tests/comfy_cli/command/generate/test_list_schema_envelope.py @@ -207,7 +207,7 @@ def test_schema_unknown_model_emits_registered_error_code(): assert result.returncode == 1 assert envelope["ok"] is False assert envelope["data"] is None - assert envelope["error"]["code"] == "generate_model_unknown" + assert envelope["error"]["code"] == "generate_unknown_model" assert error_codes.is_registered(envelope["error"]["code"]) assert envelope["error"]["hint"] assert envelope["error"]["details"]["requested"] == "definitely-not-a-model" @@ -220,7 +220,7 @@ def test_schema_without_a_model_emits_registered_error_code(): envelope = json.loads(result.stdout.splitlines()[-1]) assert result.returncode == 1 assert envelope["ok"] is False - assert envelope["error"]["code"] == "missing_argument" + assert envelope["error"]["code"] == "generate_bad_args" assert error_codes.is_registered(envelope["error"]["code"]) diff --git a/tests/comfy_cli/command/generate/test_spend_gate.py b/tests/comfy_cli/command/generate/test_spend_gate.py index 509d0bbc..593d22ae 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 ─────────────────── @@ -250,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