From b8c02556973e62e31e836e25542ff30b968aa24c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 16:58:21 -0700 Subject: [PATCH 1/2] feat(cloud login): emit machine-readable login_url event in --json mode (BE-3365) Under a non-pretty renderer (global --json / --json-stream / agentic / non-tty), comfy cloud login now surfaces the OAuth authorize URL as an event/1 line and flushes it before run_login blocks on the loopback callback, so an agent/MCP parent can open the URL instead of the command silently blocking for --timeout seconds and emitting only the final envelope. - _on_url upgrades the renderer to the NDJSON stream (matching comfy run --json) and emits {"type":"login_url","url":...,"timeout_s":...}; pretty-mode prints are unchanged. - Document the event in docs/json-output.md and the login help text. - Cover the JSON login stream (event before redacted envelope), unchanged pretty output, and the OAuthTimeout error envelope. --- comfy_cli/cloud/command.py | 31 +++-- docs/json-output.md | 16 +++ tests/comfy_cli/cloud/test_login_command.py | 146 ++++++++++++++++++++ 3 files changed, 185 insertions(+), 8 deletions(-) create mode 100644 tests/comfy_cli/cloud/test_login_command.py diff --git a/comfy_cli/cloud/command.py b/comfy_cli/cloud/command.py index 936a4d3e..60d9e122 100644 --- a/comfy_cli/cloud/command.py +++ b/comfy_cli/cloud/command.py @@ -39,7 +39,14 @@ # --------------------------------------------------------------------------- -@app.command("login", help="Sign in to Comfy Cloud via your browser (OAuth + PKCE).") +@app.command( + "login", + help=( + "Sign in to Comfy Cloud via your browser (OAuth + PKCE). Under --json/--json-stream " + "the authorize URL is emitted as a `login_url` event before the command blocks on the " + "browser callback, so an agent/MCP parent can open it; see docs/json-output.md." + ), +) @tracking.track_command("cloud") def login_cmd( no_browser: Annotated[ @@ -65,14 +72,22 @@ def login_cmd( rprint(f"Signing in to [bold cyan]Comfy Cloud[/bold cyan] ([dim]{base_url}[/dim])") def _on_url(url: str) -> None: - if not renderer.is_pretty(): + if renderer.is_pretty(): + if no_browser: + rprint("\nOpen this URL in your browser to sign in:") + rprint(f" [cyan]{url}[/cyan]\n") + else: + rprint("[dim]Opening browser… (if it doesn't appear, copy this URL)[/dim]") + rprint(f"[dim] {url}[/dim]") return - if no_browser: - rprint("\nOpen this URL in your browser to sign in:") - rprint(f" [cyan]{url}[/cyan]\n") - else: - rprint("[dim]Opening browser… (if it doesn't appear, copy this URL)[/dim]") - rprint(f"[dim] {url}[/dim]") + # Machine mode: surface the authorize URL as a `login_url` event so an + # agent/MCP parent driving `comfy --json cloud login` can open (or hand + # off) the URL. run_login then blocks up to `timeout` seconds on the + # loopback callback, so upgrade to the NDJSON stream and flush the line + # now — the parent must see the URL *before* we block on the wait. + renderer.force_stream() + renderer.event("login_url", url=url, timeout_s=timeout) + renderer.machine_stream.flush() try: result = run_login( diff --git a/docs/json-output.md b/docs/json-output.md index c10a53c4..32bd2362 100644 --- a/docs/json-output.md +++ b/docs/json-output.md @@ -110,6 +110,7 @@ line, ending the stream early. | `executed` | Node finished and reported its outputs | | `output` | One file-like output became available (`url`) | | `execution_error` | Server reported a node exception (error envelope follows) | +| `login_url` | `comfy cloud login`: OAuth authorize URL, before the browser-callback wait | Agents must ignore events whose `type` they do not recognise — new event kinds may be added in a backward-compatible manner. Agents must ignore @@ -266,6 +267,21 @@ envelope alone. {"schema": "event/1", "type": "execution_error", "prompt_id": "9b1c…", "details": {"node_id": "1", "exception_message": "API key invalid", "...": "..."}} ``` +### `login_url` + +Emitted by `comfy cloud login` under `--json`/`--json-stream` — this event is +part of the sign-in stream, not the `run` stream. It carries the OAuth +authorize URL as soon as it is built, and is flushed **before** the command +blocks (up to `timeout_s` seconds) waiting for the loopback browser callback, +so a parent process driving login headlessly can open (or forward) `url` in +time. If the callback never arrives, the terminal envelope is an +`oauth_timeout` error; on success it is the login envelope (`data.action: +"login"`, `data.session` with tokens redacted). + +```json +{"schema": "event/1", "type": "login_url", "url": "https://api.comfy.org/oauth/authorize?...", "timeout_s": 300} +``` + ## Success envelope On `--wait` success, `data` carries: diff --git a/tests/comfy_cli/cloud/test_login_command.py b/tests/comfy_cli/cloud/test_login_command.py new file mode 100644 index 00000000..08be0943 --- /dev/null +++ b/tests/comfy_cli/cloud/test_login_command.py @@ -0,0 +1,146 @@ +"""``comfy cloud login`` machine-output tests. + +The pretty renderer surfaces the OAuth authorize URL for headless users; these +tests pin the machine-mode contract added for agents/MCP: under ``--json`` the +URL is emitted as a ``login_url`` ``event/1`` line and flushed *before* +``run_login`` blocks on the loopback callback, so a pipe-reading parent can open +it. The final envelope (success or ``oauth_timeout`` error) still renders, with +the session redacted. + +Pattern: install a renderer in the mode under test, mock +``comfy_cli.cloud.command.run_login`` (so no browser / network / callback wait), +and capture stdout. +""" + +from __future__ import annotations + +import json + +import pytest +import typer + +from comfy_cli.cloud import command, oauth +from comfy_cli.output import Renderer, set_renderer +from comfy_cli.output.renderer import OutputMode + +_AUTHORIZE_URL = "https://example/oauth/authorize?response_type=code&code_challenge=abc123" +_ACCESS_TOKEN = "super-secret-access-token-do-not-leak" +_REFRESH_TOKEN = "super-secret-refresh-token-do-not-leak" + + +def _login_result() -> oauth.LoginResult: + tokens = oauth.TokenSet( + access_token=_ACCESS_TOKEN, + refresh_token=_REFRESH_TOKEN, + token_type="Bearer", + expires_in=3600, + expires_at=9999999999, + scope="mcp:tools:read mcp:tools:call", + ) + return oauth.LoginResult( + tokens=tokens, + client_id="comfy-cli", + base_url="https://testcloud.comfy.org", + resource="https://testcloud.comfy.org/mcp", + scope="mcp:tools:read mcp:tools:call", + redirect_uri="http://127.0.0.1:51234/callback", + ) + + +def _parse_lines(out: str) -> list[dict]: + return [json.loads(line) for line in out.splitlines() if line.strip()] + + +@pytest.fixture(autouse=True) +def _no_tracking(monkeypatch): + """Keep the ``@track_command`` decorator inert (no network / consent I/O).""" + monkeypatch.setattr("comfy_cli.tracking.track_event", lambda *a, **k: None) + + +def test_json_login_emits_login_url_event_before_envelope(monkeypatch, capsys): + """`comfy --json cloud login --no-browser` streams a `login_url` event + (carrying the authorize URL) ahead of the final, session-redacted envelope.""" + + def fake_run_login(**kwargs): + # run_login fires on_url_ready once the authorize URL is built, before + # it blocks on the loopback callback. Emulate exactly that ordering. + kwargs["on_url_ready"](_AUTHORIZE_URL) + return _login_result() + + monkeypatch.setattr(command, "run_login", fake_run_login) + set_renderer(Renderer(mode=OutputMode.JSON, command="cloud login", version="test")) + + command.login_cmd(no_browser=True, timeout=300) + + out = capsys.readouterr().out + lines = _parse_lines(out) + + # First the event, then the envelope — the parent must see the URL first. + assert lines[0]["schema"] == "event/1" + assert lines[0]["type"] == "login_url" + assert lines[0]["url"] == _AUTHORIZE_URL + assert lines[0]["timeout_s"] == 300 + + envelope = lines[-1] + assert envelope["type"] == "envelope" + assert envelope["ok"] is True + assert envelope["data"]["action"] == "login" + + # Ordering is explicit, not incidental. + types = [ln.get("type") for ln in lines] + assert types.index("login_url") < types.index("envelope") + + # Session is redacted: the raw tokens never reach stdout. + session = envelope["data"]["session"] + assert session["tokens_redacted"] is True + assert _ACCESS_TOKEN not in out + assert _REFRESH_TOKEN not in out + + +def test_pretty_login_prints_url_without_event_line(monkeypatch, capsys): + """Pretty mode is unchanged: the URL is printed for the human, and no + machine `login_url` event line is emitted.""" + + def fake_run_login(**kwargs): + kwargs["on_url_ready"](_AUTHORIZE_URL) + return _login_result() + + monkeypatch.setattr(command, "run_login", fake_run_login) + # Default renderer is pretty (fixture in conftest resets the singleton). + set_renderer(Renderer(mode=OutputMode.PRETTY, command="cloud login", version="test")) + + command.login_cmd(no_browser=True, timeout=300) + + out = capsys.readouterr().out + assert _AUTHORIZE_URL in out + # No machine event/envelope leaked onto stdout in pretty mode. + assert '"type": "login_url"' not in out + assert "event/1" not in out + + +def test_json_login_timeout_renders_oauth_timeout_envelope(monkeypatch, capsys): + """An `OAuthTimeout` from `run_login` surfaces as a single `ok=false` + envelope carrying the `oauth_timeout` code — no `login_url` line required.""" + + def fake_run_login(**kwargs): + raise oauth.OAuthTimeout( + "timed out waiting for browser callback after 300s", + hint="re-run `comfy cloud login` and complete the sign-in in your browser", + ) + + monkeypatch.setattr(command, "run_login", fake_run_login) + set_renderer(Renderer(mode=OutputMode.JSON, command="cloud login", version="test")) + + with pytest.raises(typer.Exit) as exc: + command.login_cmd(no_browser=True, timeout=300) + assert exc.value.exit_code == 1 + + out = capsys.readouterr().out + lines = _parse_lines(out) + assert len(lines) == 1 + + envelope = lines[0] + assert envelope["type"] == "envelope" + assert envelope["ok"] is False + assert envelope["error"]["code"] == "oauth_timeout" + assert "login_url" not in out From dfa1e12a39c945ea62fcd75d574534a8a9173369 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 17 Jul 2026 17:26:36 -0700 Subject: [PATCH 2/2] fix(cloud login): harden login_url event per Cursor review (BE-3365) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the cursor-review panel findings on #556: - Fail fast on a broken machine stream: run_login now re-raises OSError from the on_url_ready callback (a piped --json parent that hung up) instead of swallowing it and blocking the full `timeout` on a loopback callback nobody will complete. login_cmd and setup.py's _auth_browser both catch OSError and exit/return cleanly. (Medium) - Escape the authorize URL for Rich in pretty mode so an IPv6 base_url (`http://[::1]:8188`) can't trigger MarkupError. (Low) - Rewrite the timeout test to emit `login_url` before the `oauth_timeout` envelope — the real run_login fires on_url_ready before the callback wait, so the previous single-line assertion encoded an impossible ordering and left the headline guarantee untested. (Low) - Drop the redundant explicit machine_stream.flush(); renderer.event() already writes + flushes the line. (Nit) Add tests: run_login propagates OSError from the URL callback without blocking, and login_cmd fails fast when the login_url write breaks. Co-Authored-By: Claude Opus 4.8 --- comfy_cli/cloud/command.py | 21 ++++++-- comfy_cli/cloud/oauth.py | 7 +++ comfy_cli/command/setup.py | 6 +++ tests/comfy_cli/auth/test_oauth.py | 29 +++++++++++ tests/comfy_cli/cloud/test_login_command.py | 54 ++++++++++++++++++--- 5 files changed, 106 insertions(+), 11 deletions(-) diff --git a/comfy_cli/cloud/command.py b/comfy_cli/cloud/command.py index 60d9e122..02dfb1f5 100644 --- a/comfy_cli/cloud/command.py +++ b/comfy_cli/cloud/command.py @@ -20,6 +20,7 @@ from typing import Annotated import typer +from rich import markup as rich_markup from comfy_cli import tracking from comfy_cli.auth import store @@ -73,21 +74,26 @@ def login_cmd( def _on_url(url: str) -> None: if renderer.is_pretty(): + # Escape the URL for Rich: an IPv6 base_url (e.g. `http://[::1]:8188`) + # would otherwise make Rich parse `[::1]` as markup and raise. + safe_url = rich_markup.escape(url) if no_browser: rprint("\nOpen this URL in your browser to sign in:") - rprint(f" [cyan]{url}[/cyan]\n") + rprint(f" [cyan]{safe_url}[/cyan]\n") else: rprint("[dim]Opening browser… (if it doesn't appear, copy this URL)[/dim]") - rprint(f"[dim] {url}[/dim]") + rprint(f"[dim] {safe_url}[/dim]") return # Machine mode: surface the authorize URL as a `login_url` event so an # agent/MCP parent driving `comfy --json cloud login` can open (or hand # off) the URL. run_login then blocks up to `timeout` seconds on the - # loopback callback, so upgrade to the NDJSON stream and flush the line - # now — the parent must see the URL *before* we block on the wait. + # loopback callback, so upgrade to the NDJSON stream and emit the line + # now — `event()` writes + flushes, so the parent sees the URL *before* + # we block on the wait. If the write raises (the piped parent hung up), + # let it propagate: run_login re-raises OSError from the callback rather + # than blocking the full timeout on a stream nobody is reading. renderer.force_stream() renderer.event("login_url", url=url, timeout_s=timeout) - renderer.machine_stream.flush() try: result = run_login( @@ -108,6 +114,11 @@ def _on_url(url: str) -> None: details=e.details, ) raise typer.Exit(code=1) + except OSError: + # The machine stream broke while emitting `login_url` (the piped parent + # hung up), so run_login bailed instead of blocking the full timeout. + # We can't write an envelope to a dead stream — just fail fast. + raise typer.Exit(code=1) session = store.save_cloud_session( base_url=result.base_url, diff --git a/comfy_cli/cloud/oauth.py b/comfy_cli/cloud/oauth.py index f6459fa8..8d5bf2e7 100644 --- a/comfy_cli/cloud/oauth.py +++ b/comfy_cli/cloud/oauth.py @@ -450,6 +450,13 @@ def run_login( if on_url_ready is not None: try: on_url_ready(authorize_url) + except OSError: + # A broken output stream (e.g. a piped `--json` parent hung up) + # means we can no longer surface the URL — don't block the full + # `timeout_s` on a callback nobody will complete. Propagate so + # the caller can fail fast. Non-I/O callback errors (rendering, + # etc.) stay isolated below and must not break login. + raise except Exception: # noqa: BLE001 — callback errors must not break login pass diff --git a/comfy_cli/command/setup.py b/comfy_cli/command/setup.py index b1cedb02..22123150 100644 --- a/comfy_cli/command/setup.py +++ b/comfy_cli/command/setup.py @@ -336,6 +336,12 @@ def _on_url(url: str) -> None: if e.hint: pprint(f" [dim]{e.hint}[/dim]") return False + except OSError: + # A broken output stream (run_login re-raises OSError from the URL + # callback rather than blocking the full timeout on a dead terminal). + # In this interactive wizard the session is already unusable — degrade + # to a failed sign-in instead of an unhandled traceback. + return False store.save_cloud_session( base_url=result.base_url, diff --git a/tests/comfy_cli/auth/test_oauth.py b/tests/comfy_cli/auth/test_oauth.py index 253e6897..f4fddf71 100644 --- a/tests/comfy_cli/auth/test_oauth.py +++ b/tests/comfy_cli/auth/test_oauth.py @@ -354,6 +354,35 @@ def test_run_login_times_out_when_browser_never_returns(monkeypatch: pytest.Monk assert exc.value.code == "oauth_timeout" +def test_run_login_propagates_oserror_from_url_callback(monkeypatch: pytest.MonkeyPatch): + """A broken output stream in `on_url_ready` (e.g. a piped `--json` parent + that hung up) must fail fast — run_login re-raises the OSError instead of + swallowing it and blocking the full `timeout_s` on a callback wait nobody + will complete. Non-I/O callback errors stay isolated (tested elsewhere).""" + monkeypatch.setattr( + oauth, + "_post_json", + lambda url, body: {"client_id": "mcp-dyn-X", "redirect_uris": body["redirect_uris"]}, + ) + + def broken_callback(_url: str) -> None: + raise BrokenPipeError("parent hung up") + + start = time.monotonic() + with patch.object(oauth.webbrowser, "open", return_value=True): + with pytest.raises(OSError): + oauth.run_login( + base_url="https://testcloud.comfy.org", + resource="https://testcloud.comfy.org/mcp", + # A long timeout would be a multi-second hang if the OSError were + # swallowed; failing fast means we never reach the wait. + timeout_s=30.0, + open_browser=False, + on_url_ready=broken_callback, + ) + assert time.monotonic() - start < 5.0 # never blocked on the callback wait + + # --------------------------------------------------------------------------- # Store round-trip # --------------------------------------------------------------------------- diff --git a/tests/comfy_cli/cloud/test_login_command.py b/tests/comfy_cli/cloud/test_login_command.py index 08be0943..fe75f963 100644 --- a/tests/comfy_cli/cloud/test_login_command.py +++ b/tests/comfy_cli/cloud/test_login_command.py @@ -118,11 +118,16 @@ def fake_run_login(**kwargs): assert "event/1" not in out -def test_json_login_timeout_renders_oauth_timeout_envelope(monkeypatch, capsys): - """An `OAuthTimeout` from `run_login` surfaces as a single `ok=false` - envelope carrying the `oauth_timeout` code — no `login_url` line required.""" +def test_json_login_timeout_still_emits_login_url_before_error_envelope(monkeypatch, capsys): + """On timeout, the real `run_login` has already fired `on_url_ready` (before + it blocks on the callback wait), so a `--json` timeout emits the `login_url` + line *then* the `oauth_timeout` error envelope — the feature's headline + guarantee (URL surfaced before the block) holds even when the block times + out. Emulate that ordering, not an impossible URL-less timeout.""" def fake_run_login(**kwargs): + # Real run_login surfaces the URL, then the wait times out. + kwargs["on_url_ready"](_AUTHORIZE_URL) raise oauth.OAuthTimeout( "timed out waiting for browser callback after 300s", hint="re-run `comfy cloud login` and complete the sign-in in your browser", @@ -137,10 +142,47 @@ def fake_run_login(**kwargs): out = capsys.readouterr().out lines = _parse_lines(out) - assert len(lines) == 1 - envelope = lines[0] + # The URL is surfaced first, then the error envelope closes the stream. + assert lines[0]["type"] == "login_url" + assert lines[0]["url"] == _AUTHORIZE_URL + envelope = lines[-1] assert envelope["type"] == "envelope" assert envelope["ok"] is False assert envelope["error"]["code"] == "oauth_timeout" - assert "login_url" not in out + types = [ln.get("type") for ln in lines] + assert types.index("login_url") < types.index("envelope") + + +def test_json_login_fails_fast_when_login_url_write_breaks(monkeypatch, capsys): + """If the machine stream breaks while emitting `login_url` (a piped parent + hung up), `run_login` re-raises the `OSError` from the callback instead of + blocking the full timeout, and `login_cmd` fails fast with exit code 1 + rather than a silent 300s hang.""" + + class _BrokenStream: + def write(self, _s): + raise BrokenPipeError("parent hung up") + + def flush(self): + raise BrokenPipeError("parent hung up") + + blocked = {"waited": False} + + def real_ish_run_login(**kwargs): + # Mirror run_login's contract: fire the URL callback (which now writes to + # the broken stream) *before* the callback wait. A correct implementation + # never reaches the wait because the OSError propagates. + kwargs["on_url_ready"](_AUTHORIZE_URL) + blocked["waited"] = True # only reached if the OSError was swallowed + raise oauth.OAuthTimeout("timed out", hint="") + + monkeypatch.setattr(command, "run_login", real_ish_run_login) + renderer = Renderer(mode=OutputMode.JSON, command="cloud login", version="test") + renderer.machine_stream = _BrokenStream() + set_renderer(renderer) + + with pytest.raises(typer.Exit) as exc: + command.login_cmd(no_browser=True, timeout=300) + assert exc.value.exit_code == 1 + assert blocked["waited"] is False # never blocked on the wait