diff --git a/README.md b/README.md index f238b13e..074e1408 100644 --- a/README.md +++ b/README.md @@ -152,9 +152,25 @@ Comfy provides commands that allow you to easily run the installed ComfyUI. `comfy --workspace=~/comfy launch --background -- --listen 10.0.0.10 --port 8000` - Instances launched with `--background` are displayed in the "Background ComfyUI" section of `comfy env`, providing management functionalities for a single background instance only. - - Since "Comfy Server Running" in `comfy env` only shows the default port 8188, it doesn't display ComfyUI running on a different port. - Background-running ComfyUI can be stopped with `comfy stop`. +- To point **every** command (not just one) at a ComfyUI on a non-default + address — e.g. a server you started _outside_ comfy-cli on `:8189` — export + the `COMFY_LOCAL_URL` environment variable: + + `export COMFY_LOCAL_URL=http://127.0.0.1:8189` + + - Accepts `http://host:port`, `host:port`, or `http://host` (the port + defaults to `8188`; the scheme is optional and, if present, must be + `http`). IPv6 literals are bracketed: `COMFY_LOCAL_URL=http://[::1]:8189`. + - Honored by `comfy env`, `comfy run`, `comfy jobs`, `comfy upload`/`download`, + `comfy nodes`, and every other local-targeting command, so `comfy env`'s + "Comfy Server Running" line now probes and reports the resolved address. + - Precedence (per command): a per-command `--host`/`--port` flag wins, then + `COMFY_LOCAL_URL`, then a comfy-cli-launched background server, then the + `127.0.0.1:8188` default. A malformed value is ignored with a one-line + stderr warning rather than breaking the command. + - to run ComfyUI with a specific pull request: `comfy install --pr "#1234"` diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index 4b7b55b6..71f2023b 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -962,7 +962,7 @@ def validate( pass try: - graph = Graph.load(mode=mode, input_path=input_path, host=host or "127.0.0.1", port=port or 8188) + graph = Graph.load(mode=mode, input_path=input_path, host=host, port=port) except LoadError as e: renderer.error( code="cql_no_graph", @@ -1327,13 +1327,11 @@ def which(): if renderer.is_pretty(): import sys as _sys + from comfy_cli.env_checker import _display_url + from comfy_cli.local_address import resolve_local_host_port from comfy_cli.output.panels import which_panel - cfg_bg = ConfigManager().background - if cfg_bg is not None: - host, port = cfg_bg[0], cfg_bg[1] - else: - host, port = "127.0.0.1", 8188 + host, port = resolve_local_host_port(None, None, background=ConfigManager().background) try: server_running = env_checker.check_comfy_server_running(host=host, port=port, timeout=0.5) except Exception: # noqa: BLE001 @@ -1345,7 +1343,7 @@ def which(): python_executable=_sys.executable, python_version=f"{_sys.version_info.major}.{_sys.version_info.minor}.{_sys.version_info.micro}", server_running=server_running, - server_url=f"http://{host}:{port}", + server_url=_display_url(host, port), version=ConfigManager().get_cli_version(), ) ) diff --git a/comfy_cli/command/job_watcher.py b/comfy_cli/command/job_watcher.py index 90e11fed..10bdf9f9 100644 --- a/comfy_cli/command/job_watcher.py +++ b/comfy_cli/command/job_watcher.py @@ -140,9 +140,16 @@ def watch_job( def _poll_local_once(state: jobs_state.JobState, *, host: str | None, port: int | None) -> bool: """Update ``state`` in-place from a local ComfyUI server. Return True if terminal.""" from comfy_cli.command import jobs as jobs_module - - h = host or state.host or "127.0.0.1" - p = port or state.port or 8188 + from comfy_cli.local_address import resolve_local_host_port + + # Per-job recorded state (state.host/port, captured when the job was + # submitted) still wins over the env var, so a watcher keeps polling the + # server it was launched against: flag > state > COMFY_LOCAL_URL > default. + h, p = resolve_local_host_port(host or state.host, port or state.port) + # Bracket IPv6 literals so ``_snapshot`` builds a well-formed URL (it takes + # an already-bracketed host, like the `jobs` resolver produces). + if ":" in h and not h.startswith("["): + h = f"[{h}]" try: snap = jobs_module._snapshot(h, p, state.prompt_id) except Exception as e: # noqa: BLE001 — never crash the watcher on transient errors diff --git a/comfy_cli/command/nodes.py b/comfy_cli/command/nodes.py index 27e48989..4afd91ad 100644 --- a/comfy_cli/command/nodes.py +++ b/comfy_cli/command/nodes.py @@ -74,8 +74,8 @@ def _get_graph( return Graph.load( mode=mode, input_path=input_path, - host=host or "127.0.0.1", - port=port or 8188, + host=host, + port=port, ) # Live fetch goes through the resilient loader: auto-cache on success, # refresh-and-retry once, then fall back to the last cached dump (with @@ -84,8 +84,8 @@ def _get_graph( raw = resilient_load_object_info( mode=mode, - host=host or "127.0.0.1", - port=port or 8188, + host=host, + port=port, on_stale=on_stale, ) graph = Graph.from_object_info(raw) diff --git a/comfy_cli/command/setup.py b/comfy_cli/command/setup.py index b1cedb02..262e6803 100644 --- a/comfy_cli/command/setup.py +++ b/comfy_cli/command/setup.py @@ -600,10 +600,13 @@ def _verify_cloud(console) -> None: def _verify_local(console) -> None: """Check if local ComfyUI server is running.""" - from comfy_cli.env_checker import check_comfy_server_running + from comfy_cli.config_manager import ConfigManager + from comfy_cli.env_checker import _bracket_host, check_comfy_server_running + from comfy_cli.local_address import resolve_local_host_port - if check_comfy_server_running(8188, "127.0.0.1"): - pprint(" [bold green]✓[/bold green] Local server running on [cyan]127.0.0.1:8188[/cyan]") + host, port = resolve_local_host_port(None, None, background=ConfigManager().background) + if check_comfy_server_running(port, host): + pprint(f" [bold green]✓[/bold green] Local server running on [cyan]{_bracket_host(host)}:{port}[/cyan]") else: pprint(" [dim yellow]⚠[/dim yellow] Local server not running") pprint(" [dim] Start it with: [cyan]comfy launch[/cyan][/dim]") diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index d953c10e..573e6c15 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -89,7 +89,7 @@ def _get_graph(input_path: str | None, host: str | None, port: int | None, on_st try: if input_path is not None: # Explicit offline dump — Graph.load reads + annotates it. - return Graph.load(input_path=input_path, host=host or "127.0.0.1", port=port or 8188) + return Graph.load(input_path=input_path, host=host, port=port) # Live fetch: resolve mode from global routing chain, then use resilient loader. from comfy_cli import where as where_module @@ -99,8 +99,8 @@ def _get_graph(input_path: str | None, host: str | None, port: int | None, on_st raw = resilient_load_object_info( mode=mode, - host=host or "127.0.0.1", - port=port or 8188, + host=host, + port=port, on_stale=on_stale, ) graph = Graph.from_object_info(raw) diff --git a/comfy_cli/command/workflow_fragments.py b/comfy_cli/command/workflow_fragments.py index 988be5f0..e7a986fb 100644 --- a/comfy_cli/command/workflow_fragments.py +++ b/comfy_cli/command/workflow_fragments.py @@ -333,7 +333,7 @@ def _load_object_info(renderer, *, input_path: str | None, host: str | None, por decision = where_module.resolve_default() mode = "cloud" if decision.target is where_module.WhereTarget.CLOUD else "local" - return resilient_load_object_info(mode=mode, host=host or "127.0.0.1", port=port or 8188) + return resilient_load_object_info(mode=mode, host=host, port=port) except (OSError, json.JSONDecodeError) as e: renderer.error( code="object_info_unavailable", diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index a8068aaa..ccb2e7f9 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -908,8 +908,8 @@ def load( *, mode: str = "local", input_path: str | None = None, - host: str = "127.0.0.1", - port: int = 8188, + host: str | None = None, + port: int | None = None, supported_nodes_yaml: bytes | None = None, cloud_disable_yaml: bytes | None = None, no_gpu_json: bytes | None = None, @@ -1113,7 +1113,7 @@ def _load_from_file(path: str) -> dict[str, Any]: raise LoadError(f"invalid JSON in {p}: {e}", details={"path": str(p)}) from e -def _load_from_target(*, mode: str = "local", host: str = "127.0.0.1", port: int = 8188) -> dict[str, Any]: +def _load_from_target(*, mode: str = "local", host: str | None = None, port: int | None = None) -> dict[str, Any]: """Fetch /object_info from the resolved target — local or cloud. Both paths are the same HTTP fetch; only the base URL, path prefix, diff --git a/comfy_cli/cql/loader.py b/comfy_cli/cql/loader.py index c3d0e4b1..f432882e 100644 --- a/comfy_cli/cql/loader.py +++ b/comfy_cli/cql/loader.py @@ -340,7 +340,7 @@ def read_object_info_cache(host_key: str) -> dict[str, Any] | None: return data if isinstance(data, dict) else None -def _resolve_host_key(mode: str, host: str, port: int) -> str: +def _resolve_host_key(mode: str, host: str | None, port: int | None) -> str: """Resolve the cache key (the target base URL) without doing any I/O. Mirrors how the engine resolves its fetch target so the cache key matches @@ -359,8 +359,8 @@ def _resolve_host_key(mode: str, host: str, port: int) -> str: def resilient_load_object_info( *, mode: str = "local", - host: str = "127.0.0.1", - port: int = 8188, + host: str | None = None, + port: int | None = None, input_path: str | None = None, _warn=None, on_stale=None, diff --git a/comfy_cli/env_checker.py b/comfy_cli/env_checker.py index 2aea4cbd..d45ac252 100644 --- a/comfy_cli/env_checker.py +++ b/comfy_cli/env_checker.py @@ -32,6 +32,19 @@ def format_python_version(version_info): return f"[bold red]{version_info.major}.{version_info.minor}.{version_info.micro}[/bold red]" +def _bracket_host(host: str) -> str: + """Bracket a bare IPv6 literal (``::1`` -> ``[::1]``) for use in a URL. + + Idempotent: an already-bracketed host (as returned by + ``host_port.resolve_host_port``) and hostnames / IPv4 (no ``:``) pass + through unchanged, so it's safe to apply at a shared choke point regardless + of whether the caller pre-bracketed. + """ + if ":" in host and not host.startswith("["): + return f"[{host}]" + return host + + def check_comfy_server_running(port=8188, host="localhost", timeout: float = 5.0): """ Checks if the Comfy server is running by making a GET request to the /history endpoint. @@ -43,12 +56,29 @@ def check_comfy_server_running(port=8188, host="localhost", timeout: float = 5.0 bool: True if the Comfy server is running, False otherwise. """ try: - response = requests.get(f"http://{host}:{port}/history", timeout=timeout) + response = requests.get(f"http://{_bracket_host(host)}:{port}/history", timeout=timeout) return response.status_code == 200 except requests.exceptions.RequestException: return False +def _resolved_local_address() -> tuple[str, int]: + """The local ComfyUI ``(host, port)`` ``comfy env`` should probe + report. + + Honors the same precedence as every other local command minus the + per-command flag (``comfy env`` takes none): ``COMFY_LOCAL_URL`` env > + ``config.background`` > ``127.0.0.1:8188``. + """ + from comfy_cli.local_address import resolve_local_host_port + + return resolve_local_host_port(None, None, background=ConfigManager().background) + + +def _display_url(host: str, port: int) -> str: + """``http://host:port`` with IPv6 literals bracketed for a valid URL.""" + return f"http://{_bracket_host(host)}:{port}" + + @singleton class EnvChecker: """ @@ -106,11 +136,12 @@ def fill_print_table(self): config_data = ConfigManager().get_env_data() data.extend(config_data) - if check_comfy_server_running(): + host, port = _resolved_local_address() + if check_comfy_server_running(port=port, host=host): data.append( ( "Comfy Server Running", - "[bold green]Yes[/bold green]\nhttp://localhost:8188", + f"[bold green]Yes[/bold green]\n{_display_url(host, port)}", ) ) else: @@ -126,7 +157,8 @@ def fill_data(self) -> dict: against ``schemas/env.json``. """ cm = ConfigManager() - server_running = check_comfy_server_running() + host, port = _resolved_local_address() + server_running = check_comfy_server_running(port=port, host=host) return { "python": { "version": f"{sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro}", @@ -137,6 +169,6 @@ def fill_data(self) -> dict: "config": cm.get_data(), "server": { "running": server_running, - "url": "http://localhost:8188" if server_running else None, + "url": _display_url(host, port) if server_running else None, }, } diff --git a/comfy_cli/host_port.py b/comfy_cli/host_port.py index bab4f6b4..107a561a 100644 --- a/comfy_cli/host_port.py +++ b/comfy_cli/host_port.py @@ -81,16 +81,15 @@ def _to_port(s: str, original: str) -> int: def resolve_host_port(host: str | None, port: int | None) -> tuple[str, int]: - """Fill host/port from ``config.background`` then defaults; validate and - bracket IPv6 literals so callers building ``'http://{host}:{port}'`` get a - well-formed URL (e.g. ``'::1'`` -> ``'[::1]'``).""" + """Resolve host/port by precedence — explicit flag > ``COMFY_LOCAL_URL`` + env > ``config.background`` > defaults — then validate and bracket IPv6 + literals so callers building ``'http://{host}:{port}'`` get a well-formed + URL (e.g. ``'::1'`` -> ``'[::1]'``).""" + from comfy_cli.local_address import resolve_local_host_port + cfg = ConfigManager() - bg = cfg.background - if not host and bg is not None: - host = bg[0] - if not port and bg is not None: - port = bg[1] - h = validate_host(host or DEFAULT_HOST) + host, port = resolve_local_host_port(host, port, background=cfg.background) + h = validate_host(host) if ":" in h and not h.startswith("["): h = f"[{h}]" - return (h, int(port or DEFAULT_PORT)) + return (h, int(port)) diff --git a/comfy_cli/local_address.py b/comfy_cli/local_address.py new file mode 100644 index 00000000..0f92b4e8 --- /dev/null +++ b/comfy_cli/local_address.py @@ -0,0 +1,195 @@ +"""Resolve the local ComfyUI address, honoring the ``COMFY_LOCAL_URL`` env var. + +comfy-cli's local target defaults to ``127.0.0.1:8188`` unless a per-command +``--host`` / ``--port`` is passed or a comfy-cli-launched background server was +recorded in config. ``COMFY_LOCAL_URL`` is the process-wide override that +points EVERY local command at a different address — e.g. a ComfyUI started +*outside* comfy-cli on ``:8189`` — without threading a flag through each +invocation. It is the local-address analogue of :data:`comfy_cli.where.ENV_DEFAULT` +(``COMFY_WHERE``), which picks the backend but not its address. + +Two entry points: + +- :func:`parse_local_url` — parse the env var's value (``http://host:port``, + ``host:port``, or ``http://host``; scheme optional and only ``http``; IPv6 + literals bracketed) into ``(host, port)`` where ``port`` is ``None`` when the + value omits one. Raises ``ValueError`` on garbage. +- :func:`resolve_local_host_port` — the precedence resolver: explicit flag > + ``COMFY_LOCAL_URL`` env > ``config.background`` > ``127.0.0.1:8188``, with + host and port resolved independently. + +A malformed ``COMFY_LOCAL_URL`` is *ignored with a one-line stderr warning* +rather than raised, so a typo in the env var can't hard-break every command. +""" + +from __future__ import annotations + +import os +import re +import sys +from collections.abc import Mapping + +ENV_LOCAL_URL = "COMFY_LOCAL_URL" +DEFAULT_HOST = "127.0.0.1" +DEFAULT_PORT = 8188 + +# URL-injection-unsafe host characters (mirrors ``comfy_cli.host_port``). +# Brackets are included so a stray ``[``/``]`` (e.g. ``a[xyz]`` from a +# non-IPv6 authority) is rejected rather than flowing into a malformed URL or +# being parsed as a Rich markup tag at a display site. A *well-formed* IPv6 +# literal never reaches here with brackets: ``_split_host_port`` strips them +# before validating. +_UNSAFE_HOST_CHARS = frozenset("/@?#[]") + +# Bad COMFY_LOCAL_URL values already warned about this process, so a resolver +# invoked at several sites per command doesn't spam identical warnings. +_warned: set[str] = set() + + +def parse_local_url(value: str) -> tuple[str, int | None]: + """Parse a local ComfyUI address into ``(host, port)``. + + Accepts ``http://host:port``, ``host:port``, or ``http://host``. When the + value omits a port the returned port is ``None`` (not a defaulted 8188) so + the resolver can fall a host-only override through to a recorded background + port before the default — see :func:`resolve_local_host_port`. The scheme + is optional and, when present, must be ``http``. Bracketed IPv6 literals + (``[::1]:8189``) are supported and the brackets are stripped from the + returned host. Raises ``ValueError`` on any input that isn't a well-formed + local address. + """ + v = value.strip() + if not v: + raise ValueError("empty value") + + if "://" in v: + scheme, _, rest = v.partition("://") + if scheme.lower() != "http": + raise ValueError(f"unsupported scheme {scheme!r}: only 'http' is supported") + v = rest + + # Drop any path / query / fragment — only the authority carries host:port. + for sep in ("/", "?", "#"): + i = v.find(sep) + if i != -1: + v = v[:i] + if not v: + raise ValueError("missing host") + + return _split_host_port(v) + + +def _split_host_port(authority: str) -> tuple[str, int | None]: + """Split ``host[:port]`` (IPv6-aware) into a validated ``(host, port)``. + + ``port`` is ``None`` when the authority carries no port, so the resolver + can distinguish "no port given" from an explicit value. + """ + if authority.startswith("["): + end = authority.find("]") + if end == -1: + raise ValueError("unterminated '[' in IPv6 literal") + host = authority[1:end] + rest = authority[end + 1 :] + if rest: + # Only a ``:port`` suffix may follow the bracket; anything else + # (e.g. ``[::1]8188``) is a typo we must not silently drop. + if not rest.startswith(":"): + raise ValueError("unexpected text after ']'") + port = _to_port(rest[1:]) if rest[1:] else None + else: + port = None + return _validate_host(host), port + if authority.count(":") == 1: # host:port + h, p = authority.split(":") + return _validate_host(h), (_to_port(p) if p else None) + # zero colons -> hostname only; 2+ colons -> bare IPv6 literal (no port) + return _validate_host(authority), None + + +def _to_port(s: str) -> int: + try: + port = int(s) + except ValueError: + raise ValueError(f"invalid port {s!r}: not a number") from None + if not (1 <= port <= 65535): + raise ValueError(f"invalid port {port}: out of range (1-65535)") + return port + + +def _validate_host(host: str) -> str: + """Reject hosts that could corrupt a URL built as ``http://{host}:{port}``.""" + if not host: + raise ValueError("empty host") + if any(c in host for c in _UNSAFE_HOST_CHARS): + raise ValueError(f"invalid host {host!r}: contains URL-special characters") + if any(c.isspace() or ord(c) < 0x20 or ord(c) == 0x7F for c in host): + raise ValueError(f"invalid host {host!r}: contains whitespace or control characters") + return host + + +def _env_local_host_port(env: Mapping[str, str] | None = None) -> tuple[str | None, int | None]: + """Parse ``COMFY_LOCAL_URL`` from the environment. + + Returns ``(host, port)`` from a valid value, or ``(None, None)`` when the + var is unset/empty or malformed. A malformed value is ignored with a + one-line stderr warning (deduplicated per process) so a typo can't + hard-break every command. + """ + e = env if env is not None else os.environ + raw = e.get(ENV_LOCAL_URL) + if not raw or not raw.strip(): + return None, None + try: + return parse_local_url(raw) + except ValueError as exc: + if raw not in _warned: + _warned.add(raw) + # Redact any ``user:pass@`` userinfo so credentials in a mistyped + # value aren't echoed to stderr / captured in CI logs. The value + # can also surface inside ``exc`` (e.g. an invalid-port token + # ``s3cret@host``), so redact the whole composed line, not just raw. + msg = f"warning: ignoring invalid {ENV_LOCAL_URL}={raw!r}: {exc}" + print(_redact_userinfo(msg), file=sys.stderr) + return None, None + + +# ``[scheme://]userinfo@`` — the userinfo (with an optional ``:password``) is +# everything up to an ``@`` that isn't itself a delimiter/space. +_USERINFO_RE = re.compile(r"([A-Za-z][A-Za-z0-9+.\-]*://)?[^\s/@'\"]+(?::[^\s/@'\"]*)?@") + + +def _redact_userinfo(text: str) -> str: + """Mask any ``user:pass@`` userinfo in ``text`` so secrets aren't logged. + + Deduplication still keys on the original raw value; only the *printed* + form is redacted. Applied to the whole warning line because the credential + can appear both in the echoed value and inside the parse error's message. + """ + return _USERINFO_RE.sub(lambda m: f"{m.group(1) or ''}***@", text) + + +def resolve_local_host_port( + host: str | None, + port: int | None, + background: tuple | None = None, + *, + env: Mapping[str, str] | None = None, +) -> tuple[str, int]: + """Resolve the local ComfyUI ``(host, port)`` by precedence. + + Precedence, resolved *independently* for host and port: + explicit flag > ``COMFY_LOCAL_URL`` env > ``config.background`` > + :data:`DEFAULT_HOST` / :data:`DEFAULT_PORT`. So an explicit ``--port`` with + no ``--host`` still picks up the env var's host, and vice-versa. + + ``background`` is the persisted ``ConfigManager().background`` tuple + (``(host, port)``) or ``None``. The returned host is *raw* (unbracketed); + callers building a URL bracket IPv6 literals themselves. + """ + env_host, env_port = _env_local_host_port(env) + bg_host = background[0] if background else None + bg_port = background[1] if background else None + resolved_host = host or env_host or bg_host or DEFAULT_HOST + resolved_port = port or env_port or bg_port or DEFAULT_PORT + return resolved_host, int(resolved_port) diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index 6f9a3649..b39cb11d 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -59,6 +59,14 @@ comfy --json cloud whoami # signed_in, auth_method, base_url If the user is signed in, commands auto-route to cloud — just run them without `--where`. Mention routing only when the user asks to switch. +`COMFY_WHERE` picks the backend; `COMFY_LOCAL_URL` sets the **local +address** for every command when it isn't `127.0.0.1:8188` (e.g. a ComfyUI +started outside comfy-cli): `export COMFY_LOCAL_URL=http://127.0.0.1:8189` +(accepts `http://host:port`, `host:port`, or `http://host`; port defaults to +`8188`; IPv6 as `http://[::1]:8189`). Per-command precedence: `--host`/`--port` +flag → `COMFY_LOCAL_URL` → a comfy-cli-launched background server → +`127.0.0.1:8188`. + ## Error codes — react, don't guess The most common error codes and what to do: diff --git a/comfy_cli/target.py b/comfy_cli/target.py index c71f6dfa..2aaf0da2 100644 --- a/comfy_cli/target.py +++ b/comfy_cli/target.py @@ -106,10 +106,13 @@ def resolve_target( api_key=api_key, ) - # Local — keep the existing host/port resolution semantics. host/port may - # be None here; callers fill those in from their config_manager. - resolved_host = host or "127.0.0.1" - resolved_port = int(port or 8188) + # Local — resolve host/port by precedence: explicit flag > COMFY_LOCAL_URL + # env > 127.0.0.1:8188. host/port may be None here; callers that also honor + # a persisted background server (run/jobs) resolve that upstream via + # host_port.resolve_host_port before reaching us. + from comfy_cli.local_address import resolve_local_host_port + + resolved_host, resolved_port = resolve_local_host_port(host, port) # Bracket IPv6 literals so the URL is well-formed (RFC 3986 §3.2.2). url_host = f"[{resolved_host}]" if ":" in resolved_host and not resolved_host.startswith("[") else resolved_host return Target( diff --git a/tests/comfy_cli/test_env_checker.py b/tests/comfy_cli/test_env_checker.py index bed31756..b7681d2f 100644 --- a/tests/comfy_cli/test_env_checker.py +++ b/tests/comfy_cli/test_env_checker.py @@ -64,6 +64,22 @@ def test_caller_can_override_timeout(self, mock_get): check_comfy_server_running(port=8188, host="127.0.0.1", timeout=42) assert mock_get.call_args.kwargs["timeout"] == 42 + @patch("comfy_cli.env_checker.requests.get") + def test_bare_ipv6_host_is_bracketed_in_probe_url(self, mock_get): + # A bare IPv6 literal must be bracketed or the probe URL is malformed + # (``http://::1:8189/history``) and the server always reads as "down". + mock_get.return_value.status_code = 200 + check_comfy_server_running(port=8189, host="::1") + assert mock_get.call_args.args == ("http://[::1]:8189/history",) + + @patch("comfy_cli.env_checker.requests.get") + def test_already_bracketed_ipv6_host_is_not_double_bracketed(self, mock_get): + # Callers that pre-bracket (e.g. host_port.resolve_host_port) must not + # yield ``http://[[::1]]:8189``. + mock_get.return_value.status_code = 200 + check_comfy_server_running(port=8189, host="[::1]") + assert mock_get.call_args.args == ("http://[::1]:8189/history",) + class TestEnvChecker: @pytest.fixture diff --git a/tests/comfy_cli/test_local_address.py b/tests/comfy_cli/test_local_address.py new file mode 100644 index 00000000..d4aec448 --- /dev/null +++ b/tests/comfy_cli/test_local_address.py @@ -0,0 +1,274 @@ +"""Tests for ``comfy_cli.local_address`` — COMFY_LOCAL_URL parsing + the +local host/port precedence resolver, plus its use at the real resolution sites +(``resolve_target``, the ``jobs`` resolver, and ``comfy env``).""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest + +from comfy_cli.local_address import ( + DEFAULT_HOST, + DEFAULT_PORT, + ENV_LOCAL_URL, + parse_local_url, + resolve_local_host_port, +) + +# --------------------------------------------------------------------------- +# parse_local_url +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "value,expected", + [ + ("http://127.0.0.1:8189", ("127.0.0.1", 8189)), + ("127.0.0.1:8189", ("127.0.0.1", 8189)), + ("http://localhost", ("localhost", None)), # scheme, no port -> None (falls through) + ("localhost", ("localhost", None)), # bare host -> no port + ("http://example.test:9000/", ("example.test", 9000)), # trailing path dropped + ("[::1]:8189", ("::1", 8189)), # bracketed IPv6 + port + ("http://[::1]:8189", ("::1", 8189)), # scheme + bracketed IPv6 + ("[::1]", ("::1", None)), # bracketed IPv6, no port + ("::1", ("::1", None)), # bare IPv6 literal (2+ colons) -> no port + (" http://127.0.0.1:8189 ", ("127.0.0.1", 8189)), # whitespace stripped + ("HTTP://127.0.0.1:8189", ("127.0.0.1", 8189)), # scheme case-insensitive + ], +) +def test_parse_local_url_valid(value, expected): + assert parse_local_url(value) == expected + + +@pytest.mark.parametrize( + "value", + [ + "", # empty + " ", # whitespace only + "https://127.0.0.1:8189", # non-http scheme + "ftp://host:21", # non-http scheme + "http://", # missing host + ":8189", # empty host + "host:notaport", # non-numeric port + "host:0", # port out of range (low) + "host:70000", # port out of range (high) + "[::1:8189", # unterminated bracket + "[::1]x8189", # junk after bracket + "user@evil.com:8189", # URL-special char (userinfo '@') in the authority + "a[xyz]", # stray brackets in a non-IPv6 authority (markup-injection vector) + "a[xyz]:8189", # same, with a port + ], +) +def test_parse_local_url_invalid_raises(value): + with pytest.raises(ValueError): + parse_local_url(value) + + +# --------------------------------------------------------------------------- +# resolve_local_host_port — precedence +# --------------------------------------------------------------------------- + + +def test_resolve_defaults_when_nothing_set(): + assert resolve_local_host_port(None, None, env={}) == (DEFAULT_HOST, DEFAULT_PORT) + + +def test_resolve_flag_wins_over_everything(): + env = {ENV_LOCAL_URL: "http://envhost:9000"} + bg = ("bghost", 9001, 4242) + assert resolve_local_host_port("flaghost", 7000, background=bg, env=env) == ("flaghost", 7000) + + +def test_resolve_env_wins_over_background_and_default(): + env = {ENV_LOCAL_URL: "http://envhost:9000"} + bg = ("bghost", 9001, 4242) + assert resolve_local_host_port(None, None, background=bg, env=env) == ("envhost", 9000) + + +def test_resolve_background_wins_over_default(): + assert resolve_local_host_port(None, None, background=("bghost", 9001, 4242), env={}) == ("bghost", 9001) + + +def test_resolve_host_and_port_are_independent(): + # Explicit --port with no --host still takes the env var's host. + env = {ENV_LOCAL_URL: "http://envhost:9000"} + assert resolve_local_host_port(None, 7000, env=env) == ("envhost", 7000) + # Explicit --host with no --port still takes the env var's port. + assert resolve_local_host_port("flaghost", None, env=env) == ("flaghost", 9000) + + +def test_resolve_env_port_only_keeps_default_host(): + # COMFY_LOCAL_URL carrying only a host still contributes just its host; + # the port falls through to the default (no port in the URL -> 8188). + env = {ENV_LOCAL_URL: "http://envhost"} + assert resolve_local_host_port(None, None, env=env) == ("envhost", DEFAULT_PORT) + + +def test_resolve_host_only_env_falls_through_to_background_port(): + # A host-only COMFY_LOCAL_URL must NOT shadow a recorded background port + # with a defaulted 8188: host comes from the env, port from background. + env = {ENV_LOCAL_URL: "http://envhost"} + assert resolve_local_host_port(None, None, background=("bghost", 9001, 4242), env=env) == ("envhost", 9001) + + +def test_resolve_env_brackets_in_host_are_ignored_with_warning(capsys): + # A stray-bracket authority is malformed: ignored (not a live target), and + # never flows into a URL / Rich markup where it would corrupt output. + import comfy_cli.local_address as la + + la._warned.clear() + env = {ENV_LOCAL_URL: "http://a[xyz]:8189"} + assert resolve_local_host_port(None, None, background=("bghost", 9001), env=env) == ("bghost", 9001) + assert ENV_LOCAL_URL in capsys.readouterr().err + + +def test_resolve_invalid_env_warning_redacts_userinfo(capsys): + # Credentials in a mistyped value must not be echoed to stderr / CI logs. + import comfy_cli.local_address as la + + la._warned.clear() + env = {ENV_LOCAL_URL: "http://user:s3cret@host/path"} # '@' fails validation + resolve_local_host_port(None, None, env=env) + err = capsys.readouterr().err + assert "s3cret" not in err and "user" not in err + assert "***@host" in err + + +def test_resolve_invalid_env_is_ignored_with_warning(capsys): + import comfy_cli.local_address as la + + la._warned.clear() + env = {ENV_LOCAL_URL: "https://not-http:9000"} + # Falls through to background/default; the bad value is ignored, not raised. + assert resolve_local_host_port(None, None, background=("bghost", 9001), env=env) == ("bghost", 9001) + err = capsys.readouterr().err + assert ENV_LOCAL_URL in err and "https://not-http:9000" in err + + +def test_resolve_invalid_env_warning_is_deduplicated(capsys): + import comfy_cli.local_address as la + + la._warned.clear() + env = {ENV_LOCAL_URL: "garbage://x"} + resolve_local_host_port(None, None, env=env) + resolve_local_host_port(None, None, env=env) + # Only one warning line for the same bad value across the process. + assert capsys.readouterr().err.count(ENV_LOCAL_URL) == 1 + + +# --------------------------------------------------------------------------- +# Integration — the env var reaches the real resolution sites +# --------------------------------------------------------------------------- + + +def test_resolve_target_local_honors_env(monkeypatch): + from comfy_cli.target import resolve_target + + monkeypatch.setenv(ENV_LOCAL_URL, "http://127.0.0.1:8189") + target = resolve_target(where="local") + assert target.kind == "local" + assert target.base_url == "http://127.0.0.1:8189" + assert target.host == "127.0.0.1" + assert target.port == 8189 + + +def test_resolve_target_local_flag_beats_env(monkeypatch): + from comfy_cli.target import resolve_target + + monkeypatch.setenv(ENV_LOCAL_URL, "http://127.0.0.1:8189") + target = resolve_target(where="local", port=8188) + # Explicit --port wins; host still comes from the env var. + assert target.base_url == "http://127.0.0.1:8188" + assert target.port == 8188 + + +def test_jobs_resolver_honors_env(monkeypatch): + from comfy_cli.command.jobs import _resolve_host_port + + monkeypatch.setenv(ENV_LOCAL_URL, "http://127.0.0.1:8189") + with patch("comfy_cli.host_port.ConfigManager") as cm: + cm.return_value.background = None + assert _resolve_host_port(None, None) == ("127.0.0.1", 8189) + # Explicit flag still wins over the env var. + assert _resolve_host_port(None, 8188) == ("127.0.0.1", 8188) + + +def test_run_resolver_precedence(monkeypatch): + from comfy_cli.host_port import resolve_host_port + + monkeypatch.setenv(ENV_LOCAL_URL, "http://envhost:8189") + with patch("comfy_cli.host_port.ConfigManager") as cm: + # env beats background beats default. + cm.return_value.background = ("bghost", 9001, 4242) + assert resolve_host_port(None, None) == ("envhost", 8189) + # explicit flag beats env. + assert resolve_host_port("flaghost", 7000) == ("flaghost", 7000) + + +def test_env_json_reports_url_from_env_var(monkeypatch): + """`comfy env --json` server.url honors COMFY_LOCAL_URL (mocked probe).""" + from comfy_cli.env_checker import EnvChecker + + monkeypatch.setenv(ENV_LOCAL_URL, "http://127.0.0.1:8189") + checker = EnvChecker() + with ( + patch("comfy_cli.env_checker.check_comfy_server_running", return_value=True), + patch("comfy_cli.env_checker.ConfigManager") as cm, + ): + cm.return_value.background = None + cm.return_value.get_data.return_value = {} + data = checker.fill_data() + assert data["server"]["running"] is True + assert data["server"]["url"] == "http://127.0.0.1:8189" + + +def test_env_probe_uses_resolved_address(monkeypatch): + """The probe must hit the env-resolved address, not hardcoded :8188.""" + from comfy_cli import env_checker + + monkeypatch.setenv(ENV_LOCAL_URL, "http://127.0.0.1:8189") + checker = env_checker.EnvChecker() + with ( + patch("comfy_cli.env_checker.check_comfy_server_running", return_value=False) as probe, + patch("comfy_cli.env_checker.ConfigManager") as cm, + ): + cm.return_value.background = None + cm.return_value.get_data.return_value = {} + checker.fill_data() + assert probe.call_args.kwargs == {"port": 8189, "host": "127.0.0.1"} + + +def test_env_json_reports_bracketed_ipv6(monkeypatch): + from comfy_cli.env_checker import EnvChecker + + monkeypatch.setenv(ENV_LOCAL_URL, "http://[::1]:8189") + checker = EnvChecker() + with ( + patch("comfy_cli.env_checker.check_comfy_server_running", return_value=True), + patch("comfy_cli.env_checker.ConfigManager") as cm, + ): + cm.return_value.background = None + cm.return_value.get_data.return_value = {} + data = checker.fill_data() + assert data["server"]["url"] == "http://[::1]:8189" + + +def test_job_watcher_state_beats_env(monkeypatch): + """Per-job recorded state wins over the env var: flag > state > env > default.""" + from comfy_cli.command import job_watcher + + monkeypatch.setenv(ENV_LOCAL_URL, "http://envhost:8189") + state = MagicMock() + state.host = "recorded-host" + state.port = 9999 + state.prompt_id = "pid" + captured = {} + + def fake_snapshot(h, p, pid): + captured["h"], captured["p"] = h, p + return None # no record yet -> watcher keeps polling, returns False + + with patch("comfy_cli.command.jobs._snapshot", side_effect=fake_snapshot): + job_watcher._poll_local_once(state, host=None, port=None) + assert (captured["h"], captured["p"]) == ("recorded-host", 9999)