feat(cloud login): emit machine-readable login_url event in --json mode (BE-3365)#556
feat(cloud login): emit machine-readable login_url event in --json mode (BE-3365)#556mattmillerai wants to merge 2 commits into
Conversation
…de (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.
dosubot
Bot
added
size:M
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 5 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
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 <noreply@anthropic.com>
|
Review pass: all 5 review threads addressed and resolved (4 fixes in dfa1e12 — OSError fail-fast propagation, `rich.markup.escape` for IPv6 URLs, timeout-test URL ordering, dropped redundant flush; 1 by-design on the NDJSON streaming contract). All 52 tests in The two red checks (
Ready to merge once the infra CI issues clear. |
1 participant
ELI-5
When you run
comfy cloud login, the CLI prints a sign-in URL, opens yourbrowser, and waits for you to click through. In
--jsonmode (how an agent/MCPdrives the CLI) it used to print nothing — the URL was only shown to humans —
so the command just sat silently for up to
--timeoutseconds (default 300) andthen dumped one final result line. An agent had no URL to open, so it could never
complete the login.
This PR makes
comfy cloud loginemit the authorize URL as a machine-readablelogin_urlevent before it starts waiting, so an agent parent can read theURL off the pipe, open it (or hand it to the user), and let the login finish.
What changed
comfy_cli/cloud/command.py—_on_url: when the renderer is notpretty (global
--json/--json-stream/ agentic caller / non-TTY), emit alogin_urlevent carryingurl+timeout_s, then flush stdout so apipe-reading parent sees it before
run_loginblocks on the loopback callback.Pretty-mode prints are byte-for-byte unchanged.
docs/json-output.md: document thelogin_urlevent (event table + adedicated section) as part of the sign-in stream.
loginhelp text: note thelogin_urlevent under--json/--json-stream.tests/comfy_cli/cloud/test_login_command.py): JSON login streamsthe
login_urlevent before the final, session-redacted envelope; prettyoutput is unchanged (URL printed, no event line); an
OAuthTimeoutrenders asingle
ok=falseenvelope with codeoauth_timeout.No change to
run_loginitself —on_url_readyalready fires at the right moment.Judgment call (worth a reviewer's eye)
The ticket says to call
renderer.event("login_url", ...).Renderer.event()is ano-op unless the renderer is in NDJSON stream mode, and the global
--jsonflag resolves to single-envelope
JSONmode — so a bareevent()call would emitnothing and the acceptance test (
comfy --json cloud loginproduces the eventline) would fail. To make the event actually reach stdout I first call
renderer.force_stream(), exactly ascomfy run --jsondoes(
cmdline.py:781) to upgrade into the one NDJSON dialect. Net effect: under--json, login now emits an NDJSON stream (event line + terminaltype: "envelope"line) rather than a single envelope. That is the documented dialect(consumers discriminate the final line by
type), andforce_streamonly runsonce the URL is built (i.e. right before the command would otherwise block), so
non-machine paths are untouched.
Deliberately out of scope
discovery.py'sSTREAM_EVENT_SCHEMAS(which advertises, per command, a JSONschema its event lines validate against) is not updated — registering login
there implies authoring a
login_eventschema file, which is a separate additivechange. Kept out to keep this PR small and matched to the ticket's narrow doc
scope; happy to file a follow-up.
Negative-claim note
This change adds a capability (surfacing the URL for agents) and removes a
silent-block dead-end; it introduces no "not supported"/deny path, so no
capability-falsification was required.
Testing
uv run --extra dev pytest tests/comfy_cli/cloud/test_login_command.py→ 3 passed.uv run --extra dev pytest tests/comfy_cli/cloud tests/comfy_cli/auth tests/comfy_cli/output tests/comfy_cli/command/test_run_json.py→ 382 passed.ruff check/ruff format --checkclean on the touched files.