feat: emit envelope/1 from launch --background and stop under --json#510
feat: emit envelope/1 from launch --background and stop under --json#510mattmillerai wants to merge 3 commits into
launch --background and stop under --json#510Conversation
`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
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 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 (5)
📝 WalkthroughWalkthroughLifecycle commands now honor JSON rendering. ChangesLifecycle JSON output
Sequence Diagram(s)sequenceDiagram
participant background_launch
participant launch_and_monitor
participant Renderer
participant ComfyUI
background_launch->>launch_and_monitor: start background process
ComfyUI->>launch_and_monitor: write GUI success marker
launch_and_monitor->>Renderer: emit launch payload
Renderer-->>background_launch: return JSON envelope
background_launch->>ComfyUI: exit after successful startup
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
dosubot
Bot
added
size:L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@comfy_cli/cmdline.py`:
- Around line 1130-1148: Prevent JSON stdout pollution at all three sites: in
comfy_cli/cmdline.py lines 1130-1148, gate both stop-status rprint calls behind
renderer.is_json(); in comfy_cli/command/launch.py lines 276-291, gate the Panel
and “Execution error” prints so JSON mode only uses renderer.error(); and in
comfy_cli/command/launch.py lines 394-420, gate the success banner with
get_renderer().is_json() or a shared renderer reference so only
get_renderer().emit() runs in JSON mode. Strengthen tests in
tests/comfy_cli/command/test_launch_stop_json.py to assert JSON responses
produce exactly one stdout line.
In `@tests/comfy_cli/command/test_launch_stop_json.py`:
- Around line 51-61: The _validator_for function still constructs the deprecated
jsonschema.RefResolver; replace it with a referencing.Registry populated with
the discovered schema resources and pass that registry to Draft202012Validator.
Preserve resolution for both each schema’s $id and its filename while removing
the RefResolver usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6ad785ac-9891-4f42-87ec-9749cbd837d7
📒 Files selected for processing (7)
comfy_cli/cmdline.pycomfy_cli/command/launch.pycomfy_cli/discovery.pycomfy_cli/error_codes.pycomfy_cli/schemas/launch.jsoncomfy_cli/schemas/stop.jsontests/comfy_cli/command/test_launch_stop_json.py
![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 |
|---|---|
| 🟠 High | 1 |
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…solver 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 <noreply@anthropic.com>
…ge; guard emit 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 <noreply@anthropic.com>
|
Heads-up for the merger: the red Windows Specific Commands check is an environmental dependency-install failure, not a defect in this PR. The This is the classic Windows locked-DLL problem: uv tries to reinstall Evidence it is not from this PR:
Local verification on this branch: |
|
Update: I re-ran the failed Windows Specific Commands job (run 29305977176) and it reproduced the identical failure — So this is a deterministic Windows runner file-lock, not a transient flake a re-run will clear. Root cause: This is unrelated to this PR: the diff only touches the |
|
CI note: the red "Windows Specific Commands" check is a pre-existing uv-on-Windows infra flake, not caused by this diff. It fails in the That is uv unable to replace
All review threads are resolved and the rest of CI is green. Re-ran the flaky job 3× without it clearing (Windows runners appear consistently affected today). Fixing the underlying uv/Windows install lock is out of scope for this launch/stop-JSON PR — this is safe to merge once the flake clears on a re-run. |
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
|
1 participant
ELI-5
When you run a comfy command with
--json, you're telling it "give me a machine-readable answer, not pretty text for a human." Every command honors that by printing oneenvelope/1JSON object — exceptlaunchandstop, which quietly ignored--jsonand printed human text with a success exit code and no JSON. A program watching for the envelope (the local MCP, CI) couldn't tell a successful launch/stop from a broken response, so it treated success as failure and retried actions that shouldn't be retried. This PR makeslaunch --backgroundandstopemit the envelope like everyone else.Fixes #509 (the root cause behind half of the "stop/launch report failure while the action succeeded" bug).
What changed
comfy --json stopnow emits{ok, command: "stop", data: {host, port, stopped}}.comfy --json launch --backgroundnow emits{ok, command: "launch", data: {host, port, pid, background}}at the success marker (right beforeos._exit, using the renderer's flushed write so the line survives the hard exit).--port, port in use, startup failed, no workspace) also emit a schema-valid error envelope in JSON mode instead of exiting with no document.comfy_cli/schemas/stop.jsonandlaunch.json, registerscomfy stop/comfy launchinCOMMAND_SCHEMAS(socomfy discoveradvertises the shapes), and adds the corresponding lifecycle error codes to the registry.Pretty (non-
--json) output is unchanged — every new emit is JSON-mode-only (renderer.emitis a no-op in pretty mode; error envelopes are guarded byis_json(), keeping the originalrprintred text).Scope notes / judgment calls
comfy launchis intentionally not touched. It blocks on the ComfyUI process and exits with the child's return code — there is no discrete "success" moment to emit an envelope at. Only--background, which returns control to the caller, gets one.changedis set tostopped/Truefor the mutating success paths, matching the envelope'schangedconvention.Tests
tests/comfy_cli/command/test_launch_stop_json.py:envelope.json+stop.json/launch.json.launch_and_monitorsuccess branch (fake child writes the "To see the GUI go to:" marker;os._exitpatched) so the emit-before-exit path is exercised end-to-end.no_background,launch_failed,background_already_running,invalid_port).Full suite green (2583 passed, 37 skipped);
ruff format/checkclean. The existing registration/error-code ratchet tests pass with the new commands + codes registered.