From 2cddfdf459807a7da7b967baa359f6aa216d5f5b Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 13 Jul 2026 21:04:57 -0700 Subject: [PATCH 1/3] feat: emit envelope/1 from `launch --background` and `stop` under --json `comfy --json launch --background` and `comfy --json stop` accepted the global `--json` flag but ignored it: they printed human text with exit 0 and never emitted an envelope/1 document, unlike every other wrapped command. Programmatic callers that require the envelope (the local MCP `_run_comfy`, CI scripts) then read a successful lifecycle action as a malformed/failed response and retry non-idempotent operations. Emit the standard envelope in JSON mode from both commands: - `stop` -> data {host, port, stopped} - `launch --background` -> data {host, port, pid, background} (emitted at the success marker, right before os._exit so the flushed line survives) Both success and the in-band error paths now emit a schema-valid envelope in JSON mode; pretty (non-JSON) output is unchanged. Ships stop.json / launch.json data schemas, registers both commands in COMMAND_SCHEMAS, and adds six lifecycle error codes. Tests pin both success shapes, the error shapes, schema validity, and that pretty mode emits no envelope. Foreground `comfy launch` is intentionally excluded: it blocks on the server and exits with ComfyUI's return code, so there is no success moment to emit. Closes #509 --- comfy_cli/cmdline.py | 34 ++- comfy_cli/command/launch.py | 71 ++++- comfy_cli/discovery.py | 4 +- comfy_cli/error_codes.py | 35 +++ comfy_cli/schemas/launch.json | 27 ++ comfy_cli/schemas/stop.json | 23 ++ .../command/test_launch_stop_json.py | 282 ++++++++++++++++++ 7 files changed, 458 insertions(+), 18 deletions(-) create mode 100644 comfy_cli/schemas/launch.json create mode 100644 comfy_cli/schemas/stop.json create mode 100644 tests/comfy_cli/command/test_launch_stop_json.py diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index 4662b788..4d76b0c1 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -1112,22 +1112,40 @@ def validate_comfyui(_env_checker): @app.command(help="Stop background ComfyUI") @tracking.track_command() def stop(): - if constants.CONFIG_KEY_BACKGROUND not in ConfigManager().config["DEFAULT"]: - rprint("[bold red]No ComfyUI is running in the background.[/bold red]\n") - raise typer.Exit(code=1) + renderer = get_renderer() + config = ConfigManager() - bg_info = ConfigManager().background + bg_info = config.background if constants.CONFIG_KEY_BACKGROUND in config.config["DEFAULT"] else None if not bg_info: - rprint("[bold red]No ComfyUI is running in the background.[/bold red]\n") + if renderer.is_json(): + renderer.error( + code="no_background", + message="No ComfyUI is running in the background.", + command="stop", + ) + else: + rprint("[bold red]No ComfyUI is running in the background.[/bold red]\n") raise typer.Exit(code=1) - is_killed = utils.kill_all(bg_info[2]) + + host, port, pid = bg_info + is_killed = utils.kill_all(pid) if not is_killed: rprint("[bold red]Failed to stop ComfyUI in the background.[/bold red]\n") else: - rprint(f"[bold yellow]Background ComfyUI is stopped.[/bold yellow] ({bg_info[0]}:{bg_info[1]})") + rprint(f"[bold yellow]Background ComfyUI is stopped.[/bold yellow] ({host}:{port})") + + config.remove_background() - ConfigManager().remove_background() + # In JSON mode, emit the envelope so programmatic callers can tell a + # successful stop from a malformed/absent response (pretty mode already + # printed the human line above; emit() is a no-op there). + renderer.emit( + {"host": host, "port": port, "stopped": is_killed}, + command="stop", + where="local", + changed=is_killed, + ) @app.command(help="Launch ComfyUI: ?[--background] ?[-- ]") diff --git a/comfy_cli/command/launch.py b/comfy_cli/command/launch.py index 1f015367..997c3910 100644 --- a/comfy_cli/command/launch.py +++ b/comfy_cli/command/launch.py @@ -17,6 +17,7 @@ from comfy_cli.command.custom_nodes.cm_cli_util import find_cm_cli, resolve_manager_gui_mode from comfy_cli.config_manager import ConfigManager from comfy_cli.env_checker import check_comfy_server_running +from comfy_cli.output import get_renderer from comfy_cli.output import rprint as print # context-aware print: stderr in JSON mode from comfy_cli.resolve_python import resolve_workspace_python from comfy_cli.workspace_manager import WorkspaceManager, WorkspaceType @@ -155,10 +156,18 @@ def launch( resolved_workspace = workspace_manager.workspace_path if not resolved_workspace: - print( - "\nComfyUI is not available.\nTo install ComfyUI, you can run:\n\n\tcomfy install\n\n", - file=sys.stderr, - ) + renderer = get_renderer() + if renderer.is_json(): + renderer.error( + code="workspace_not_found", + message="ComfyUI is not available.", + command="launch", + ) + else: + print( + "\nComfyUI is not available.\nTo install ComfyUI, you can run:\n\n\tcomfy install\n\n", + file=sys.stderr, + ) raise typer.Exit(code=1) if (extra is None or len(extra) == 0) and workspace_manager.workspace_type == WorkspaceType.DEFAULT: @@ -189,11 +198,19 @@ def launch( def background_launch(extra, frontend_pr=None): + renderer = get_renderer() config_background = ConfigManager().background if config_background is not None and utils.is_running(config_background[2]): - print( - "[bold red]ComfyUI is already running in background.\nYou cannot start more than one background service.[/bold red]\n" - ) + if renderer.is_json(): + renderer.error( + code="background_already_running", + message="ComfyUI is already running in background. You cannot start more than one background service.", + command="launch", + ) + else: + print( + "[bold red]ComfyUI is already running in background.\nYou cannot start more than one background service.[/bold red]\n" + ) raise typer.Exit(code=1) port = 8188 @@ -217,11 +234,29 @@ def background_launch(extra, frontend_pr=None): try: port = int(port) except (TypeError, ValueError): - print(f"[bold red]Invalid --port value {port!r}; expected an integer.[/bold red]\n") + if renderer.is_json(): + renderer.error( + code="invalid_port", + message=f"Invalid --port value {port!r}; expected an integer.", + command="launch", + details={"port": port}, + ) + else: + print(f"[bold red]Invalid --port value {port!r}; expected an integer.[/bold red]\n") raise typer.Exit(code=1) if check_comfy_server_running(port): - print(f"[bold red]The {port} port is already in use. A new ComfyUI server cannot be launched.\n[bold red]\n") + if renderer.is_json(): + renderer.error( + code="port_in_use", + message=f"The {port} port is already in use. A new ComfyUI server cannot be launched.", + command="launch", + details={"port": port}, + ) + else: + print( + f"[bold red]The {port} port is already in use. A new ComfyUI server cannot be launched.\n[bold red]\n" + ) raise typer.Exit(code=1) cmd = [ @@ -248,6 +283,12 @@ def background_launch(extra, frontend_pr=None): ) print("\n[bold red]Execution error: failed to launch ComfyUI[/bold red]\n") + if renderer.is_json(): + renderer.error( + code="launch_failed", + message="Failed to launch ComfyUI in the background.", + command="launch", + ) # NOTE: os.exit(0) doesn't work os._exit(1) @@ -365,6 +406,18 @@ def _handle(line): cfg.config["DEFAULT"][constants.CONFIG_KEY_BACKGROUND_LOG] = log_path cfg.write_config() + # In JSON mode, emit the success envelope so programmatic callers + # (the local MCP, CI) can parse a real result instead of an empty + # stdout. No-op in pretty mode — the human line above already ran. + # Emit before os._exit(0): _write_json_line flushes, and os._exit + # skips interpreter cleanup so a buffered write would be lost. + get_renderer().emit( + {"host": listen, "port": port, "pid": process.pid, "background": True}, + command="launch", + where="local", + changed=True, + ) + # NOTE: os.exit(0) doesn't work. os._exit(0) if logging_flag: diff --git a/comfy_cli/discovery.py b/comfy_cli/discovery.py index f98e9493..955e8622 100644 --- a/comfy_cli/discovery.py +++ b/comfy_cli/discovery.py @@ -97,7 +97,9 @@ # config "comfy set-default": "set_default", "comfy version": "version", - # background server logs + # background server lifecycle + "comfy launch": "launch", + "comfy stop": "stop", "comfy logs": "logs", } diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 4cba21d3..9e6aeafc 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -583,6 +583,41 @@ class ErrorCode: "`comfy feedback` was run in JSON/non-interactive mode without an inline message.", 'comfy feedback "your feedback here"', ), + # --- launch / stop lifecycle --------------------------------------------- + ErrorCode( + "no_background", + "`comfy stop` found no ComfyUI running in the background (nothing to stop).", + "start one with `comfy launch --background` first", + ), + ErrorCode( + "workspace_not_found", + "`comfy launch` could not resolve a ComfyUI workspace to launch.", + "run `comfy install`, or pass `--workspace `", + ), + ErrorCode( + "background_already_running", + "`comfy launch --background` refused because a ComfyUI is already running in the background " + "(only one background service is allowed).", + "stop it first with `comfy stop`", + ), + ErrorCode( + "invalid_port", + "`comfy launch --background` was given a `--port` that isn't an integer. `details.port` carries " + "the offending value.", + "pass an integer port, e.g. `--port 8188`", + ), + ErrorCode( + "port_in_use", + "`comfy launch --background` found the requested port already in use, so a new ComfyUI server " + "could not be started. `details.port` carries the port.", + "stop whatever holds the port, or launch on a different `--port`", + ), + ErrorCode( + "launch_failed", + "`comfy launch --background` started ComfyUI but never saw the success marker before the process " + "exited — startup failed. The captured log is available via `comfy logs`.", + "inspect `comfy logs` for the startup error, fix it, and retry", + ), ) diff --git a/comfy_cli/schemas/launch.json b/comfy_cli/schemas/launch.json new file mode 100644 index 00000000..d942b1f0 --- /dev/null +++ b/comfy_cli/schemas/launch.json @@ -0,0 +1,27 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://comfy.org/schemas/launch.json", + "title": "comfy launch --json data payload", + "description": "Emitted by `comfy launch --background` once ComfyUI has started successfully in the background. Foreground `comfy launch` blocks on the server and does not emit this envelope.", + "type": "object", + "required": ["host", "port", "pid", "background"], + "additionalProperties": true, + "properties": { + "host": { + "type": "string", + "description": "Host the background server bound to (e.g. `127.0.0.1`)." + }, + "port": { + "type": "integer", + "description": "Port the background server is listening on." + }, + "pid": { + "type": "integer", + "description": "Process id of the launched background ComfyUI. Pass to `comfy stop` semantics via the recorded background entry." + }, + "background": { + "type": "boolean", + "description": "Always true — this payload is only emitted for `--background` launches." + } + } +} diff --git a/comfy_cli/schemas/stop.json b/comfy_cli/schemas/stop.json new file mode 100644 index 00000000..440592b3 --- /dev/null +++ b/comfy_cli/schemas/stop.json @@ -0,0 +1,23 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$id": "https://comfy.org/schemas/stop.json", + "title": "comfy stop --json data payload", + "description": "Result of stopping the background ComfyUI started by `comfy launch --background`.", + "type": "object", + "required": ["host", "port", "stopped"], + "additionalProperties": true, + "properties": { + "host": { + "type": "string", + "description": "Host the background server was bound to (e.g. `127.0.0.1`)." + }, + "port": { + "type": "integer", + "description": "Port the background server was listening on." + }, + "stopped": { + "type": "boolean", + "description": "True when the background process was killed. The recorded background entry is cleared regardless." + } + } +} diff --git a/tests/comfy_cli/command/test_launch_stop_json.py b/tests/comfy_cli/command/test_launch_stop_json.py new file mode 100644 index 00000000..96727875 --- /dev/null +++ b/tests/comfy_cli/command/test_launch_stop_json.py @@ -0,0 +1,282 @@ +"""Envelope contract tests for `comfy stop` and `comfy launch --background`. + +These pin the agent-facing contract that GitHub issue #509 was about: the +global `--json` flag used to be accepted but ignored by these two lifecycle +commands, so a successful stop/launch printed human text and emitted no +envelope/1 document. Programmatic callers (the local MCP `_run_comfy`, CI) +then read a success as a malformed/failed response. + +We assert: +- JSON mode emits a single, schema-valid envelope on both success and the + in-band error paths, with the shapes the issue specifies. +- Pretty (non-JSON) mode is unchanged — no envelope on stdout. +""" + +from __future__ import annotations + +import asyncio +import json +from pathlib import Path + +import jsonschema +import pytest +import typer + +from comfy_cli import cmdline +from comfy_cli.caller import Caller +from comfy_cli.command import launch as launch_mod +from comfy_cli.output.renderer import OutputMode, Renderer, set_renderer + +SCHEMAS_DIR = Path(__file__).resolve().parents[3] / "comfy_cli" / "schemas" + + +def _force_renderer(mode: OutputMode) -> Renderer: + r = Renderer.resolve( + is_stdout_tty=False, + env={}, + caller=Caller(kind="user", agentic=False, source_env=None), + json_flag=(mode is OutputMode.JSON), + ) + r.mode = mode + set_renderer(r) + return r + + +def _last_envelope(capsys) -> dict: + out = capsys.readouterr().out.strip() + assert out, "expected an envelope on stdout" + return json.loads(out.splitlines()[-1]) + + +def _validator_for(name: str) -> jsonschema.Validator: + schema = json.loads((SCHEMAS_DIR / name).read_text()) + store: dict = {} + for path in SCHEMAS_DIR.glob("*.json"): + s = json.loads(path.read_text()) + if s.get("$id"): + store[s["$id"]] = s + store[path.name] = s + base = SCHEMAS_DIR.absolute().as_uri() + "/" + resolver = jsonschema.RefResolver(base_uri=base, referrer=schema, store=store) + return jsonschema.Draft202012Validator(schema, resolver=resolver) + + +def _validate(envelope: dict, data_schema: str) -> None: + _validator_for("envelope.json").validate(envelope) + if envelope["ok"]: + _validator_for(data_schema).validate(envelope["data"]) + + +class _FakeConfig: + """Stand-in for the ConfigManager singleton used by `stop`.""" + + def __init__(self, background): + self.background = background + self.config = {"DEFAULT": {}} + if background is not None: + # `stop` gates on the raw config key before reading `.background`. + self.config["DEFAULT"]["background"] = repr(background) + self.removed = False + + def remove_background(self): + self.removed = True + + +# -------------------------------------------------------------------------- +# stop +# -------------------------------------------------------------------------- + + +class TestStopJson: + def test_success_emits_stop_envelope(self, monkeypatch, capsys): + fake = _FakeConfig(("127.0.0.1", 8188, 4242)) + monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) + monkeypatch.setattr(cmdline.utils, "kill_all", lambda pid: True) + _force_renderer(OutputMode.JSON) + + cmdline.stop() + + env = _last_envelope(capsys) + _validate(env, "stop.json") + assert env["ok"] is True + assert env["command"] == "stop" + assert env["where"] == "local" + assert env["changed"] is True + assert env["data"] == {"host": "127.0.0.1", "port": 8188, "stopped": True} + assert fake.removed is True + + def test_kill_failure_still_emits_envelope_stopped_false(self, monkeypatch, capsys): + fake = _FakeConfig(("127.0.0.1", 8188, 4242)) + monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) + monkeypatch.setattr(cmdline.utils, "kill_all", lambda pid: False) + _force_renderer(OutputMode.JSON) + + cmdline.stop() + + env = _last_envelope(capsys) + _validate(env, "stop.json") + assert env["ok"] is True + assert env["data"]["stopped"] is False + assert env["changed"] is False + # The stale background entry is cleared regardless of the kill outcome. + assert fake.removed is True + + def test_no_background_is_structured_error(self, monkeypatch, capsys): + fake = _FakeConfig(None) + monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) + monkeypatch.setattr( + cmdline.utils, "kill_all", lambda pid: pytest.fail("kill_all must not run with nothing to stop") + ) + _force_renderer(OutputMode.JSON) + + with pytest.raises(typer.Exit) as ei: + cmdline.stop() + assert ei.value.exit_code == 1 + + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["command"] == "stop" + assert env["error"]["code"] == "no_background" + + def test_pretty_mode_unchanged_no_envelope(self, monkeypatch, capsys): + fake = _FakeConfig(("127.0.0.1", 8188, 4242)) + monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) + monkeypatch.setattr(cmdline.utils, "kill_all", lambda pid: True) + _force_renderer(OutputMode.PRETTY) + + cmdline.stop() + + out = capsys.readouterr().out + # Human line still prints to stdout; crucially, no JSON envelope does. + assert "Background ComfyUI is stopped" in out + assert "envelope/1" not in out + + +# -------------------------------------------------------------------------- +# launch --background +# -------------------------------------------------------------------------- + + +class _StopExit(Exception): + """Sentinel raised in place of os._exit so the monitor loop unwinds.""" + + def __init__(self, code): + self.code = code + + +def _drive_monitor_with_success_line(monkeypatch, tmp_path, mode: OutputMode): + """Run `launch_and_monitor` end-to-end with a fake child that writes the + success marker to the logfile, so the real success branch (config write + + envelope emit + os._exit) executes.""" + monkeypatch.chdir(tmp_path) + + class _FakePopen: + def __init__(self, cmd, stdout=None, **kwargs): + self.pid = 4242 + # The real child writes to this logfile fd; emulate the success + # banner the monitor tails for. + stdout.write("Launching ComfyUI from: /ws\nTo see the GUI go to: http://127.0.0.1:8188\n") + stdout.flush() + + def poll(self): + # None keeps the reader loop consuming lines; the success handler + # fires (and raises via the patched os._exit) before EOF matters. + return None + + monkeypatch.setattr(launch_mod.subprocess, "Popen", _FakePopen) + + def _fake_exit(code): + raise _StopExit(code) + + monkeypatch.setattr(launch_mod.os, "_exit", _fake_exit) + _force_renderer(mode) + + with pytest.raises(_StopExit) as ei: + asyncio.run(launch_mod.launch_and_monitor(["comfy", "launch"], "127.0.0.1", 8188)) + return ei.value + + +class TestLaunchBackgroundJson: + def test_success_emits_launch_envelope(self, monkeypatch, tmp_path, capsys): + exit_exc = _drive_monitor_with_success_line(monkeypatch, tmp_path, OutputMode.JSON) + assert exit_exc.code == 0 + + env = _last_envelope(capsys) + _validate(env, "launch.json") + assert env["ok"] is True + assert env["command"] == "launch" + assert env["where"] == "local" + assert env["changed"] is True + assert env["data"] == {"host": "127.0.0.1", "port": 8188, "pid": 4242, "background": True} + + def test_success_pretty_mode_writes_no_envelope(self, monkeypatch, tmp_path, capsys): + exit_exc = _drive_monitor_with_success_line(monkeypatch, tmp_path, OutputMode.PRETTY) + assert exit_exc.code == 0 + out = capsys.readouterr().out + assert "envelope/1" not in out + + def test_launch_failure_is_structured_error(self, monkeypatch, capsys): + from unittest.mock import AsyncMock + + monkeypatch.setattr(launch_mod, "check_comfy_server_running", lambda port: False) + # A fresh (unbacked) ConfigManager: no background running. + fake_cfg = type("C", (), {"background": None})() + monkeypatch.setattr(launch_mod, "ConfigManager", lambda: fake_cfg) + monkeypatch.setattr(launch_mod, "launch_and_monitor", AsyncMock(return_value=["boom\n"])) + + exits = {} + monkeypatch.setattr(launch_mod.os, "_exit", lambda code: exits.setdefault("code", code)) + _force_renderer(OutputMode.JSON) + + launch_mod.background_launch(extra=[]) + + assert exits["code"] == 1 + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["command"] == "launch" + assert env["error"]["code"] == "launch_failed" + + def test_already_running_is_structured_error(self, monkeypatch, capsys): + fake_cfg = type("C", (), {"background": ("127.0.0.1", 8188, 4242)})() + monkeypatch.setattr(launch_mod, "ConfigManager", lambda: fake_cfg) + monkeypatch.setattr(launch_mod.utils, "is_running", lambda pid: True) + _force_renderer(OutputMode.JSON) + + with pytest.raises(typer.Exit): + launch_mod.background_launch(extra=[]) + + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["error"]["code"] == "background_already_running" + + def test_invalid_port_is_structured_error(self, monkeypatch, capsys): + fake_cfg = type("C", (), {"background": None})() + monkeypatch.setattr(launch_mod, "ConfigManager", lambda: fake_cfg) + _force_renderer(OutputMode.JSON) + + with pytest.raises(typer.Exit): + launch_mod.background_launch(extra=["--port", "notaport"]) + + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["error"]["code"] == "invalid_port" + assert env["error"]["details"]["port"] == "notaport" + + +# -------------------------------------------------------------------------- +# discovery registration +# -------------------------------------------------------------------------- + + +def test_launch_and_stop_registered_in_discovery(): + from comfy_cli.discovery import COMMAND_SCHEMAS + + assert COMMAND_SCHEMAS["comfy stop"] == "stop" + assert COMMAND_SCHEMAS["comfy launch"] == "launch" + # Both schema files ship so `comfy discover` can advertise the shapes. + assert (SCHEMAS_DIR / "stop.json").is_file() + assert (SCHEMAS_DIR / "launch.json").is_file() From 3a4f92c5f4dcee4ebe7dbaf5fe41ebd8cef14a32 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 13 Jul 2026 21:17:58 -0700 Subject: [PATCH 2/3] test(launch/stop json): assert single stdout line + migrate off RefResolver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address CodeRabbit review on #510: - Harden `_last_envelope` to assert JSON mode emits exactly one stdout line, so a future pretty-print leak into the machine channel fails loudly instead of hiding behind "grab the last line". (The gate-rprint code change CodeRabbit suggested is unnecessary: rprint/print already route to stderr in JSON mode, so stdout is not polluted — this assertion pins that contract.) - Migrate the schema-validator helper from the deprecated jsonschema.RefResolver to referencing.Registry, resolving by both $id and filename. Co-Authored-By: Claude Opus 4.8 --- .../command/test_launch_stop_json.py | 33 +++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/tests/comfy_cli/command/test_launch_stop_json.py b/tests/comfy_cli/command/test_launch_stop_json.py index 96727875..b1f7e2af 100644 --- a/tests/comfy_cli/command/test_launch_stop_json.py +++ b/tests/comfy_cli/command/test_launch_stop_json.py @@ -21,6 +21,7 @@ import jsonschema import pytest import typer +from referencing import Registry, Resource from comfy_cli import cmdline from comfy_cli.caller import Caller @@ -45,20 +46,32 @@ def _force_renderer(mode: OutputMode) -> Renderer: def _last_envelope(capsys) -> dict: out = capsys.readouterr().out.strip() assert out, "expected an envelope on stdout" - return json.loads(out.splitlines()[-1]) + lines = out.splitlines() + # JSON mode reserves stdout for exactly one envelope line; all human text + # (rprint/print) must be routed to stderr. More than one stdout line means + # a pretty-mode print leaked into the machine channel — fail loudly here + # instead of silently grabbing the last line. + assert len(lines) == 1, f"JSON mode must emit exactly one stdout line, got {len(lines)}: {lines!r}" + return json.loads(lines[0]) + + +def _schema_registry() -> Registry: + """Registry of every shipped schema, resolvable by both its canonical + ``$id`` and its filename (envelope.json's ``$ref: "error.json"`` is a + filename-relative reference).""" + registry: Registry = Registry() + for path in SCHEMAS_DIR.glob("*.json"): + s = json.loads(path.read_text()) + resource = Resource.from_contents(s) + registry = registry.with_resource(uri=path.name, resource=resource) + if s.get("$id"): + registry = registry.with_resource(uri=s["$id"], resource=resource) + return registry def _validator_for(name: str) -> jsonschema.Validator: schema = json.loads((SCHEMAS_DIR / name).read_text()) - store: dict = {} - for path in SCHEMAS_DIR.glob("*.json"): - s = json.loads(path.read_text()) - if s.get("$id"): - store[s["$id"]] = s - store[path.name] = s - base = SCHEMAS_DIR.absolute().as_uri() + "/" - resolver = jsonschema.RefResolver(base_uri=base, referrer=schema, store=store) - return jsonschema.Draft202012Validator(schema, resolver=resolver) + return jsonschema.Draft202012Validator(schema, registry=_schema_registry()) def _validate(envelope: dict, data_schema: str) -> None: From 1259dbf3d0da203c10eb1cb83d80a7102f1c21c9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Mon, 13 Jul 2026 21:26:21 -0700 Subject: [PATCH 3/3] fix(launch/stop json): don't orphan on kill failure; enforce port range; guard emit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the cursor-review panel findings on #510: - stop (High/Medium): `remove_background()` no longer runs unconditionally. When `kill_all` fails AND the server is still running, keep the PID record and emit an `ok=false` `stop_failed` error with a non-zero exit, so a later stop/logs can still target it and callers see the failure. An already-gone process (kill_all False, not running) stays a success: clear the stale record, emit `stopped=false`/`changed=false`. Killed → `stopped=true`/`changed=true`. - launch (Low): reject `--port` values outside 1-65535 with the clear `invalid_port` signal instead of a generic downstream launch failure. - launch (Nit): guard the success `emit()` before `os._exit(0)` so a stdout write failure (e.g. BrokenPipeError) still reaches the clean exit. Adds a `stop_failed` error code, updates stop.json's `stopped` description, and extends the contract tests (genuine-failure vs already-gone stop, out-of-range port). Co-Authored-By: Claude Opus 4.8 --- comfy_cli/cmdline.py | 27 ++++++++--- comfy_cli/command/launch.py | 32 ++++++++++--- comfy_cli/error_codes.py | 7 +++ comfy_cli/schemas/stop.json | 2 +- .../command/test_launch_stop_json.py | 45 ++++++++++++++++++- 5 files changed, 99 insertions(+), 14 deletions(-) diff --git a/comfy_cli/cmdline.py b/comfy_cli/cmdline.py index 4d76b0c1..26e1598a 100644 --- a/comfy_cli/cmdline.py +++ b/comfy_cli/cmdline.py @@ -1130,16 +1130,33 @@ def stop(): host, port, pid = bg_info is_killed = utils.kill_all(pid) - if not is_killed: - rprint("[bold red]Failed to stop ComfyUI in the background.[/bold red]\n") - else: - rprint(f"[bold yellow]Background ComfyUI is stopped.[/bold yellow] ({host}:{port})") + # kill_all returns False both when the process is already gone and when the + # kill genuinely fails. Only a server that is *still running* afterward is a + # real failure: keep its PID record so a later `comfy stop`/`comfy logs` can + # still target it, and report non-zero. An already-dead process is a + # successful "nothing left to stop" — clear the stale record and succeed. + if not is_killed and utils.is_running(pid): + if renderer.is_json(): + renderer.error( + code="stop_failed", + message="Failed to stop ComfyUI in the background; it is still running.", + command="stop", + details={"host": host, "port": port, "pid": pid}, + ) + else: + rprint("[bold red]Failed to stop ComfyUI in the background.[/bold red]\n") + raise typer.Exit(code=1) config.remove_background() + if not renderer.is_json(): + rprint(f"[bold yellow]Background ComfyUI is stopped.[/bold yellow] ({host}:{port})") + # In JSON mode, emit the envelope so programmatic callers can tell a # successful stop from a malformed/absent response (pretty mode already - # printed the human line above; emit() is a no-op there). + # printed the human line above; emit() is a no-op there). ``stopped`` is + # True only when we actively killed the process; an already-gone server is + # a success with ``changed=False``. renderer.emit( {"host": host, "port": port, "stopped": is_killed}, command="stop", diff --git a/comfy_cli/command/launch.py b/comfy_cli/command/launch.py index 997c3910..740e1733 100644 --- a/comfy_cli/command/launch.py +++ b/comfy_cli/command/launch.py @@ -245,6 +245,20 @@ def background_launch(extra, frontend_pr=None): print(f"[bold red]Invalid --port value {port!r}; expected an integer.[/bold red]\n") raise typer.Exit(code=1) + if not (1 <= port <= 65535): + # Out-of-range ports parse fine but never bind; surface the clear + # invalid_port signal here rather than a generic launch failure later. + if renderer.is_json(): + renderer.error( + code="invalid_port", + message=f"Invalid --port value {port}; expected an integer in the range 1-65535.", + command="launch", + details={"port": port}, + ) + else: + print(f"[bold red]Invalid --port value {port}; expected an integer in the range 1-65535.[/bold red]\n") + raise typer.Exit(code=1) + if check_comfy_server_running(port): if renderer.is_json(): renderer.error( @@ -411,12 +425,18 @@ def _handle(line): # stdout. No-op in pretty mode — the human line above already ran. # Emit before os._exit(0): _write_json_line flushes, and os._exit # skips interpreter cleanup so a buffered write would be lost. - get_renderer().emit( - {"host": listen, "port": port, "pid": process.pid, "background": True}, - command="launch", - where="local", - changed=True, - ) + # Guard the write so a stdout failure (e.g. BrokenPipeError if the + # caller closed the read end) still reaches the clean os._exit(0) + # below instead of unwinding the monitor with a traceback. + try: + get_renderer().emit( + {"host": listen, "port": port, "pid": process.pid, "background": True}, + command="launch", + where="local", + changed=True, + ) + except Exception: + pass # NOTE: os.exit(0) doesn't work. os._exit(0) diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 9e6aeafc..5097b51a 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -589,6 +589,13 @@ class ErrorCode: "`comfy stop` found no ComfyUI running in the background (nothing to stop).", "start one with `comfy launch --background` first", ), + ErrorCode( + "stop_failed", + "`comfy stop` tried to kill the background ComfyUI but it is still running (e.g. a " + "permission or transient error). The PID record is kept so it can be targeted again. " + "`details` carries the host/port/pid.", + "check permissions and retry `comfy stop`, or kill the pid in `details` manually", + ), ErrorCode( "workspace_not_found", "`comfy launch` could not resolve a ComfyUI workspace to launch.", diff --git a/comfy_cli/schemas/stop.json b/comfy_cli/schemas/stop.json index 440592b3..b0e99ed3 100644 --- a/comfy_cli/schemas/stop.json +++ b/comfy_cli/schemas/stop.json @@ -17,7 +17,7 @@ }, "stopped": { "type": "boolean", - "description": "True when the background process was killed. The recorded background entry is cleared regardless." + "description": "True when this call actively killed the background process. False when the server was already gone (still a success, with `changed=false`). The recorded background entry is cleared in both cases. If the kill fails while the server is still running, `comfy stop` instead emits an `ok=false` envelope with the `stop_failed` error and keeps the record." } } } diff --git a/tests/comfy_cli/command/test_launch_stop_json.py b/tests/comfy_cli/command/test_launch_stop_json.py index b1f7e2af..5cf1c615 100644 --- a/tests/comfy_cli/command/test_launch_stop_json.py +++ b/tests/comfy_cli/command/test_launch_stop_json.py @@ -118,10 +118,37 @@ def test_success_emits_stop_envelope(self, monkeypatch, capsys): assert env["data"] == {"host": "127.0.0.1", "port": 8188, "stopped": True} assert fake.removed is True - def test_kill_failure_still_emits_envelope_stopped_false(self, monkeypatch, capsys): + def test_kill_failure_still_running_is_structured_error(self, monkeypatch, capsys): + # kill_all failed AND the server is still running: a genuine failure. + # Must report ok=false / exit 1 and KEEP the record so it can be + # targeted again (no orphaning). fake = _FakeConfig(("127.0.0.1", 8188, 4242)) monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) monkeypatch.setattr(cmdline.utils, "kill_all", lambda pid: False) + monkeypatch.setattr(cmdline.utils, "is_running", lambda pid: True) + _force_renderer(OutputMode.JSON) + + with pytest.raises(typer.Exit) as ei: + cmdline.stop() + assert ei.value.exit_code == 1 + + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["command"] == "stop" + assert env["error"]["code"] == "stop_failed" + assert env["error"]["details"]["pid"] == 4242 + # The record is preserved so a later stop/logs can still target it. + assert fake.removed is False + + def test_already_gone_is_success_changed_false(self, monkeypatch, capsys): + # kill_all returned False because the process was already gone: a + # successful "nothing left to stop". Clear the stale record, report + # success with stopped=false / changed=false. + fake = _FakeConfig(("127.0.0.1", 8188, 4242)) + monkeypatch.setattr(cmdline, "ConfigManager", lambda: fake) + monkeypatch.setattr(cmdline.utils, "kill_all", lambda pid: False) + monkeypatch.setattr(cmdline.utils, "is_running", lambda pid: False) _force_renderer(OutputMode.JSON) cmdline.stop() @@ -131,7 +158,6 @@ def test_kill_failure_still_emits_envelope_stopped_false(self, monkeypatch, caps assert env["ok"] is True assert env["data"]["stopped"] is False assert env["changed"] is False - # The stale background entry is cleared regardless of the kill outcome. assert fake.removed is True def test_no_background_is_structured_error(self, monkeypatch, capsys): @@ -279,6 +305,21 @@ def test_invalid_port_is_structured_error(self, monkeypatch, capsys): assert env["error"]["code"] == "invalid_port" assert env["error"]["details"]["port"] == "notaport" + @pytest.mark.parametrize("bad_port", ["0", "70000", "-1"]) + def test_out_of_range_port_is_structured_error(self, monkeypatch, capsys, bad_port): + fake_cfg = type("C", (), {"background": None})() + monkeypatch.setattr(launch_mod, "ConfigManager", lambda: fake_cfg) + _force_renderer(OutputMode.JSON) + + with pytest.raises(typer.Exit): + launch_mod.background_launch(extra=["--port", bad_port]) + + env = _last_envelope(capsys) + _validator_for("envelope.json").validate(env) + assert env["ok"] is False + assert env["error"]["code"] == "invalid_port" + assert env["error"]["details"]["port"] == int(bad_port) + # -------------------------------------------------------------------------- # discovery registration