diff --git a/comfy_cli/command/models/search.py b/comfy_cli/command/models/search.py index fcab06c2..251a1fe3 100644 --- a/comfy_cli/command/models/search.py +++ b/comfy_cli/command/models/search.py @@ -1,6 +1,8 @@ """``comfy models`` — live model discovery against local or cloud. -Four subcommands, all routed by ``--where`` (cloud auto-detect by default): +Four subcommands, all routed by ``--where`` (cloud auto-detect by default); +each also accepts ``--host``/``--port`` to aim a *local* query at a specific +ComfyUI (rejected against a cloud target), mirroring ``comfy upload``: comfy models list-folders # GET /api/experiment/models | /models comfy models list-folder # GET /api/experiment/models/ | /models/ @@ -178,7 +180,20 @@ def _emit_http_error(e: urllib.error.HTTPError, *, renderer, target, message: st raise typer.Exit(code=1) from e -def _resolve_and_stamp(renderer, where: str | None): +# How a `cloud` routing decision was reached, in words, for the --host/--port +# rejection message. Keys are `where.WhereResolution.source` values. Mirrors the +# copy `cmdline.upload` uses (BE-5662) — kept local rather than imported so this +# module stays free of a `cmdline` import cycle. +_WHERE_SOURCE_PHRASES = { + "flag": "targeting cloud via --where cloud", + "env": "targeting cloud via the COMFY_WHERE environment variable", + "project": "targeting cloud via this project's configured default", + "config": "targeting cloud via your saved `where_default` setting", + "auto": "targeting cloud because you're signed in (no explicit --where)", +} + + +def _resolve_and_stamp(renderer, where: str | None, *, host: str | None = None, port: int | None = None): """Resolve the routing Target for a ``models`` verb and stamp it on the renderer. Every verb here calls this at the point it decides local-vs-cloud, so the @@ -186,10 +201,70 @@ def _resolve_and_stamp(renderer, where: str | None): Errors raised *before* this (an unsafe path segment) keep ``where: null``, which is correct — nothing had routed yet. Explicit ``emit(..., where=...)`` arguments still take precedence over the stamp. + + ``host``/``port`` route a **local** ``models`` query at a specific ComfyUI + (the ``--host``/``--port`` flags), mirroring ``comfy upload`` (BE-5662): + they are validated the same way, rejected against an effective ``cloud`` + target, and otherwise handed to ``resolve_target``, which applies the local + precedence explicit flag > ``COMFY_LOCAL_URL`` > ``127.0.0.1:8188``. With + both ``None`` (no flags), resolution is exactly what it was before the flags + existed. The cloud target's address comes from the signed-in account and + ignores host/port entirely, so pairing them with a cloud target is a usage + error rather than a silently-ignored flag — the root cause of the comfy-mcp + bug where ``search_models`` answered from the local machine regardless of + the configured remote target. """ + from comfy_cli import where as where_module + from comfy_cli.config_manager import ConfigManager + from comfy_cli.host_port import report_usage_error, validate_host from comfy_cli.target import resolve_target - target = resolve_target(where=where) + # Validate the flags before resolving anything: the host lands verbatim in + # ``http://{host}:{port}/...``, so a URL-special or control character is a + # usage error (BadParameter, exit 2) regardless of target. Port range is + # checked the same way ``comfy upload`` does. ``report_usage_error`` emits + # the terminating envelope for that rejection in JSON/NDJSON mode; the + # exception still escapes, so click's exit-2 usage contract is unchanged. + with report_usage_error(renderer): + if host is not None: + host = validate_host(host) + if port is not None and not (1 <= port <= 65535): + raise typer.BadParameter(f"invalid port: {port} is out of range (1-65535)") + + try: + decision = where_module.resolve( + flag=where, config_value=ConfigManager().get(where_module.CONFIG_KEY_WHERE_DEFAULT) + ) + except ValueError as e: + renderer.error(code="where_invalid", message=str(e), hint="use --where local or --where cloud") + raise typer.Exit(code=1) from e + + effective_where = "cloud" if decision.target is where_module.WhereTarget.CLOUD else "local" + # Routing is decided, so every error envelope from here down can name the + # target — including the `host_flag_cloud` rejection immediately below. + renderer.where = effective_where + # --host/--port address a local ComfyUI; the cloud target's address comes + # from the signed-in account and ignores them (``Target.host``/``port`` are + # documented local-only). Reject the combination rather than silently + # answering from a machine the caller didn't name. + if effective_where == "cloud" and (host is not None or port is not None): + # The cloud target can come from an explicit --where, but equally from + # COMFY_WHERE, a project/config default, or credential auto-detection — + # so name the source rather than accusing the user of a flag they may + # never have typed. + source = _WHERE_SOURCE_PHRASES.get(decision.source, f"resolved to cloud by {decision.source}") + renderer.error( + code="host_flag_cloud", + message=f"--host/--port target a local ComfyUI server, but this run is {source}", + hint=( + "pass --where local to aim at a local server; to reach a different cloud address " + "set COMFY_CLOUD_BASE_URL or run `comfy cloud set-base-url`" + ), + details={"host": host, "port": port, "where": effective_where, "where_source": decision.source}, + ) + raise typer.Exit(code=1) + + target = resolve_target(where=effective_where, host=host, port=port) renderer.where = target.kind return target @@ -211,9 +286,17 @@ def list_folders_cmd( str | None, typer.Option("--where", show_default=False, help="Override the resolved routing mode."), ] = None, + host: Annotated[ + str | None, + typer.Option(help="Server host (defaults to COMFY_LOCAL_URL or 127.0.0.1). Local targets only."), + ] = None, + port: Annotated[ + int | None, + typer.Option(help="Server port (defaults to COMFY_LOCAL_URL or 8188). Local targets only."), + ] = None, ): renderer = get_renderer() - target = _resolve_and_stamp(renderer, where) + target = _resolve_and_stamp(renderer, where, host=host, port=port) url = target.url(*_models_path_parts(target)) try: @@ -286,10 +369,18 @@ def list_folder_cmd( int | None, typer.Option("--limit", show_default=False, help="Cap output to N rows."), ] = None, + host: Annotated[ + str | None, + typer.Option(help="Server host (defaults to COMFY_LOCAL_URL or 127.0.0.1). Local targets only."), + ] = None, + port: Annotated[ + int | None, + typer.Option(help="Server port (defaults to COMFY_LOCAL_URL or 8188). Local targets only."), + ] = None, ): renderer = get_renderer() _reject_unsafe_path_segment(folder, kind="folder", renderer=renderer) - target = _resolve_and_stamp(renderer, where) + target = _resolve_and_stamp(renderer, where, host=host, port=port) # Percent-encoded for the same reason `_local_folder_matches` does it: the # relaxed validation above admits spaces, `?`/`#`, and non-ASCII, none of # which may be allowed to alter the request. Error payloads below carry the @@ -662,11 +753,19 @@ def search_cmd( str | None, typer.Option("--where", show_default=False, help="Override the resolved routing mode."), ] = None, + host: Annotated[ + str | None, + typer.Option(help="Server host (defaults to COMFY_LOCAL_URL or 127.0.0.1). Local targets only."), + ] = None, + port: Annotated[ + int | None, + typer.Option(help="Server port (defaults to COMFY_LOCAL_URL or 8188). Local targets only."), + ] = None, ): renderer = get_renderer() if type_ is not None: _reject_unsafe_path_segment(type_, kind="type", renderer=renderer) - target = _resolve_and_stamp(renderer, where) + target = _resolve_and_stamp(renderer, where, host=host, port=port) try: if target.is_cloud: @@ -738,9 +837,17 @@ def show_cmd( str | None, typer.Option("--where", show_default=False, help="Override the resolved routing mode."), ] = None, + host: Annotated[ + str | None, + typer.Option(help="Server host (defaults to COMFY_LOCAL_URL or 127.0.0.1). Local targets only."), + ] = None, + port: Annotated[ + int | None, + typer.Option(help="Server port (defaults to COMFY_LOCAL_URL or 8188). Local targets only."), + ] = None, ): renderer = get_renderer() - target = _resolve_and_stamp(renderer, where) + target = _resolve_and_stamp(renderer, where, host=host, port=port) if not target.is_cloud: # On local there's no asset catalog. We can confirm the file exists by diff --git a/tests/comfy_cli/command/models/test_search.py b/tests/comfy_cli/command/models/test_search.py index 5503e450..e32d1917 100644 --- a/tests/comfy_cli/command/models/test_search.py +++ b/tests/comfy_cli/command/models/test_search.py @@ -911,3 +911,172 @@ def test_local_is_explicitly_unsupported(self, local_target, monkeypatch, capsys env = _run(["show", "anything.safetensors", "--where", "local"], capsys) assert env["ok"] is False assert env["error"]["code"] == "models_show_local_unsupported" + + +# --------------------------------------------------------------------------- +# --host/--port routing (BE-5788) — mirrors `comfy upload` (BE-5662) +# --------------------------------------------------------------------------- + + +class TestHostPortRouting: + """``comfy models --host/--port`` aims a LOCAL query at a specific + ComfyUI, closing the gap where these four verbs could only reach the + process-wide ``COMFY_LOCAL_URL`` (or the 127.0.0.1:8188 default). The URL + the request actually hits is asserted end-to-end through the *real* + ``resolve_target`` — no fixture pins it — so the precedence (explicit flag > + ``COMFY_LOCAL_URL`` > default), resolved independently for host and port, is + exercised, not mocked. + """ + + def test_host_and_port_reach_the_resolved_url(self, monkeypatch, capsys): + monkeypatch.delenv("COMFY_LOCAL_URL", raising=False) + _patch_urlopen(monkeypatch, {"10.0.0.5:9999/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local", "--host", "10.0.0.5", "--port", "9999"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://10.0.0.5:9999/models" + + def test_no_flags_keeps_the_loopback_default(self, monkeypatch, capsys): + # Acceptance: no flags -> behavior identical to today. + monkeypatch.delenv("COMFY_LOCAL_URL", raising=False) + _patch_urlopen(monkeypatch, {"127.0.0.1:8188/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://127.0.0.1:8188/models" + + def test_no_flags_still_honors_comfy_local_url(self, monkeypatch, capsys): + monkeypatch.setenv("COMFY_LOCAL_URL", "http://192.168.1.50:7777") + _patch_urlopen(monkeypatch, {"192.168.1.50:7777/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://192.168.1.50:7777/models" + + def test_flags_beat_comfy_local_url(self, monkeypatch, capsys): + monkeypatch.setenv("COMFY_LOCAL_URL", "http://192.168.1.50:7777") + _patch_urlopen(monkeypatch, {"10.0.0.5:9999/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local", "--host", "10.0.0.5", "--port", "9999"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://10.0.0.5:9999/models" + + def test_host_only_flag_keeps_the_env_port(self, monkeypatch, capsys): + # host and port resolve independently, so --host alone must not drop the + # env var's port back to the 8188 default. + monkeypatch.setenv("COMFY_LOCAL_URL", "http://192.168.1.50:7777") + _patch_urlopen(monkeypatch, {"10.0.0.5:7777/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local", "--host", "10.0.0.5"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://10.0.0.5:7777/models" + + def test_ipv6_host_is_bracketed_in_the_url(self, monkeypatch, capsys): + monkeypatch.delenv("COMFY_LOCAL_URL", raising=False) + _patch_urlopen(monkeypatch, {"[::1]:8189/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local", "--host", "::1", "--port", "8189"], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == "http://[::1]:8189/models" + + def test_search_threads_host_port_to_resolve_target(self, monkeypatch, capsys): + # The other verbs route through the same helper; assert `search` in + # particular hands the pair to `resolve_target`. + from comfy_cli.target import Target + + seen: dict[str, Any] = {} + + def fake_resolve_target(**kwargs): + seen.update(kwargs) + return Target( + kind="local", + base_url="http://10.0.0.5:9999", + path_prefix="", + history_path="history", + host="10.0.0.5", + port=9999, + ) + + monkeypatch.setattr("comfy_cli.target.resolve_target", fake_resolve_target) + _patch_urlopen(monkeypatch, {"10.0.0.5:9999/models/loras": _LOCAL_FILES_BY_FOLDER["loras"]}) + env = _run( + ["search", "--text", "ltx", "--type", "lora", "--where", "local", "--host", "10.0.0.5", "--port", "9999"], + capsys, + ) + assert env["ok"] is True, env + assert seen["host"] == "10.0.0.5" + assert seen["port"] == 9999 + assert seen["where"] == "local" + + +class TestHostPortCloudRejection: + """``--host``/``--port`` name a LOCAL server, so pairing them with an + effective cloud target is a structured usage error rather than a silently + ignored flag — the comfy-mcp root cause this ticket fixes.""" + + @pytest.mark.parametrize( + "args", + [ + ["list-folders", "--where", "cloud", "--host", "10.0.0.5"], + ["list-folders", "--where", "cloud", "--port", "9999"], + ["list-folder", "loras", "--where", "cloud", "--host", "10.0.0.5"], + ["search", "--where", "cloud", "--host", "10.0.0.5", "--port", "9999"], + ["show", "flux1-dev.safetensors", "--where", "cloud", "--port", "9999"], + ], + ) + def test_host_or_port_with_cloud_where_flag_is_rejected(self, args, monkeypatch, capsys): + # urlopen must never be reached: the rejection fires before routing. + _patch_urlopen(monkeypatch, {}) + env = _run(args, capsys) + assert env["ok"] is False, env + assert env["error"]["code"] == "host_flag_cloud" + + def test_cloud_where_env_is_rejected_and_names_the_source(self, monkeypatch, capsys): + # COMFY_WHERE is how a top-level `comfy --where cloud` arrives, so the + # guard keys off the RESOLVED target, not the flag. + monkeypatch.setenv("COMFY_WHERE", "cloud") + _patch_urlopen(monkeypatch, {}) + env = _run(["search", "--host", "10.0.0.5"], capsys) + assert env["ok"] is False, env + assert env["error"]["code"] == "host_flag_cloud" + assert "COMFY_WHERE" in env["error"]["message"] + assert env["error"]["details"]["where_source"] == "env" + + +class TestHostPortUsageErrors: + """A bad ``--host``/``--port`` is a usage error (exit 2), validated the same + way ``comfy upload`` validates its flags, before any request is made.""" + + @pytest.fixture + def runner(self): + return CliRunner() + + @pytest.mark.parametrize( + "bad", + [ + "evil/host", + "user@evil", + "host?x", + "host#x", + "host\rname", + "host\nname", + "", + " ", + "a%0d%0aX-Injected:%201", + "host%2fpath", + "127.0.0.1:8188", + "localhost:8188", + ], + ) + def test_invalid_host_is_a_usage_error(self, runner, bad): + # typer.BadParameter -> click UsageError -> exit code 2. + result = runner.invoke(search_cmd.app, ["list-folders", "--where", "local", "--host", bad]) + assert result.exit_code == 2, result.output + + @pytest.mark.parametrize("bad", ["0", "65536", "-1"]) + def test_out_of_range_port_is_a_usage_error(self, runner, bad): + result = runner.invoke(search_cmd.app, ["list-folders", "--where", "local", "--port", bad]) + assert result.exit_code == 2, result.output + + @pytest.mark.parametrize("good", ["::1", "[::1]", "fe80::1"]) + def test_ipv6_literal_host_is_accepted(self, good, monkeypatch, capsys): + monkeypatch.delenv("COMFY_LOCAL_URL", raising=False) + bracketed = good if good.startswith("[") else f"[{good}]" + _patch_urlopen(monkeypatch, {f"{bracketed}:8188/models": _LOCAL_FOLDERS}) + env = _run(["list-folders", "--where", "local", "--host", good], capsys) + assert env["ok"] is True, env + assert env["data"]["url"] == f"http://{bracketed}:8188/models"