fix(output): emit a terminating error envelope for --host/--port usage errors in JSON/NDJSON modes - #687
Conversation
…e errors A `typer.BadParameter` raised during host/port resolution escaped straight to click, which prints a human usage panel on stderr and exits 2 with ZERO bytes on stdout. In JSON/NDJSON mode a machine consumer just saw the stream stop, while every other failure on the same commands ends with an `ok:false` envelope. It also hit auto-selected JSON mode (non-TTY stdout), so `comfy jobs ls --port 0 | cat` was silent too. Add `host_port.report_usage_error(renderer)` — a context manager that emits the missing terminating envelope (`host_port_invalid`, exit_code=2) when `renderer.is_json()` and then RE-RAISES, so click's usage-error contract (exit 2, message on stderr) is unchanged and pretty mode is untouched. Wrap every host/port guard/resolver block: `run`, `validate`, `upload`, `run-template`, all six `comfy jobs` sites, `comfy nodes`, and `transfer.execute_upload`'s defence-in-depth guard (which fires for direct library callers that never reach click). Register `host_port_invalid` in the error-code registry. Known remaining gap, deliberately not addressed here: `--port <non-integer>` is rejected by click's own type coercion during argv parsing, before any command body runs, so it still exits 2 with an empty stdout.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 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 (9)
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 7 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 3 |
| 🟢 Low | 2 |
| ⚪ Nit | 2 |
Panel: 8/8 reviewers contributed findings.
…-6660) Cursor panel findings on #687: - A broken stdout no longer displaces the usage error. `_write_json_line` lets `OSError`/`BrokenPipeError` propagate on purpose, so with a closed consumer (`comfy jobs ls --port 0 | head -1`) the pipe error replaced the in-flight `BadParameter`: click never rendered the usage panel and the exit code stopped being the documented 2. The envelope write is best effort now. - Redact `user:pass@` from the envelope message. `validate_host` formats with `{host!r}`, so `--host user:s3cret@server` landed verbatim in the JSON stream agent harnesses capture and persist. Reuses `local_address. _redact_userinfo`, already applied to the analogous host-parse warning. - `report_usage_error` takes `command=`, and every `comfy jobs` site passes the same subcommand its success envelope reports. The failure line said `jobs` while the success line said `jobs ls`, so a consumer dispatching on `command` saw two values for one subcommand. - Drop the wrapper in `_watch_ls`. `ls_cmd` rejects a non-pretty renderer with `json_incompatible` before `--watch` is reachable, so the helper could never emit there — dead code. Pretty mode wants click's panel, unchanged. - Say in the docstring what is NOT covered: click coerces argv before any callback runs, so `--port notaport` still exits 2 with empty stdout. The docstring implied full coverage; only the commit message carried the gap. - Broaden the `host_port_invalid` hint. The same validation covers the host in `config.background`, so a corrupted record tripped it with no bad flag passed and the hint pointed at flags the caller never used. It now names `comfy stop` for that case. Deferred: `execute_upload` writes to the process-wide renderer singleton, so a long-lived library caller that catches and continues is left with `_envelope_emitted` latched. That predates this PR (`upload_failed` in the same function already does it) and fixing it means giving `execute_upload` an injectable renderer — tracked as a follow-up, not patched here.
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
bigcat88
left a comment
There was a problem hiding this comment.
Approving. This closes a gap I'd flagged on #679 last week — I noted there that an invalid port produced a usage error with zero bytes on stdout and no envelope, and observed it was consistent with --host '' and --port 70000. Consistent, but consistently wrong, and this is the right fix.
Every wrapped site, measured (stdout bytes and the last-line envelope code):
main this PR
run --port 0 rc=2 0B — rc=2 391B host_port_invalid
run --host 'ev il/host' rc=2 0B — rc=2 410B host_port_invalid
validate --port 0 rc=2 0B — rc=2 396B host_port_invalid
jobs ls --port 0 rc=2 0B — rc=2 395B host_port_invalid
jobs status --port 0 rc=2 0B — rc=2 399B host_port_invalid
nodes ls --port 0 rc=2 0B — rc=2 393B host_port_invalid
upload --port 0 rc=2 0B — rc=2 394B host_port_invalid
run-template <real> --port 0 rc=2 0B — rc=2 400B host_port_invalid
The exit code stays 2 everywhere, so nothing that keys on the usage-error status changes.
The auto-selected-JSON case is the one I'd have most expected to be missed, and it isn't. With no --json flag at all and stdout redirected, jobs ls --port 0 and validate --port 0 both went from 0 bytes to a proper envelope. That's the shape an agent actually hits, since non-TTY stdout auto-selects JSON.
Pretty mode is untouched — 0 bytes on stdout on both branches, and the human message appears exactly once on stderr (641 bytes, identical). No double-print, which was the obvious way to get this wrong.
Your documented limit reproduces exactly as described: jobs ls --port abc is still rc=2 with 0 bytes on stdout, on both branches, because click coerces int during argv parsing before any command body runs. Right call to name it rather than widen the diff — that one needs a handler around app().
One note on my own test, in case it bites someone: run-template resolves the template name before host/port, so run-template default --port 0 fails with template_not_found and never reaches the guard. I had to use a real template (video_minimax_h3_t2v) to exercise that site. Worth making sure the test for it uses a resolvable name, or it'll pass for the wrong reason.
Full pytest on your branch against current main: 4461 passed, 31 skipped. The one failure is test_non_fast_deps_uses_global_python, which fails identically on main.
ELI-5
If you type a bad
--portor--host, comfy-cli stops with an error — but in JSON/NDJSON mode it stopped silently: the error went to stderr and stdout got nothing at all. A script or agent reading stdout just saw the stream end with no explanation. Now it always gets one last line saying what went wrong, exactly like every other failure does. Humans see no change.The bug
A
typer.BadParameterraised during host/port resolution escaped to click, which prints a human usage panel on stderr and exits 2 with zero bytes on stdout. Every other failure on those same commands emits a terminatingok:falseenvelope, so machine consumers saw the stream stop with no diagnosis.Confirmed on
mainbefore the fix:comfy run --json --workflow wf.json --port 0→ exit 2, empty stdout. Same for an invalid--host.--jsonflag, since non-TTY stdout auto-selects JSON inRenderer.resolve:comfy jobs ls --port 0 | catandcomfy validate --workflow wf.json --port 0 | catwere silent too.Contrast on the identical path:
comfy run --json --workflow wf.json --where bogus→ exit 1 and a final{"ok": false, ..., "error": {"code": "where_invalid", ...}}line.The fix
A new
host_port.report_usage_error(renderer)context manager emits the missing terminating envelope (host_port_invalid) and then re-raises, so click still converts the exception into the usage-errorSystemExit(2). Three deliberate properties:renderer.is_json()— true for JSON and NDJSON, because single-envelope JSON mode has the identical gap. Notis_stream(), notnot is_pretty().renderer.error()does not raise, so nothing needs catching;exit_code=2is passed through only so the renderer's recorded exit code is truthful.It takes the renderer as a parameter so
host_port.pykeeps depending ontyperalone. In non-CLI library use the singleton defaults to pretty, sois_json()is False and the helper is a no-op.Wrapped call sites:
run,validate,upload(cmdline),run-template(templates), all six_resolve_host_portsites injobs,nodes's_get_graph, andtransfer.execute_upload's defence-in-depth guard — which matters because direct library callers (comfy-mcp's_with_target) never reach click at all, so the envelope is the only machine-readable signal they get. The renderer's emit-once guard means the doubled upload guards cannot write two envelopes.host_port_invalidis registered incomfy_cli/error_codes.py; the registry test enforces raised↔registered in both directions.Verification
ruff check .clean,ruff format --checkclean on every touched file, and the full suite green: 4348 passed, 37 skipped. New tests cover exit-2 + last-line-envelope forrun --json,validate(JSON viaCOMFY_OUTPUT),jobs ls(auto-selected JSON, no flag),nodes,upload(bad host and bad port), andrun-template; plus pretty mode emitting nothing on stdout with the message appearing exactly once, the directexecute_uploadlibrary call, and two unit tests pinning that the helper is a no-op for a pretty renderer and lets a non-BadParameterexception through untouched.Also exercised live against the real CLI — every one of the seven wrapped sites emits exactly the intended envelope and exits 2, and pretty mode's stdout is empty with the message printed once.
Judgment calls and known limits
--port <non-integer>is still silent on stdout.comfy jobs ls --port abcis rejected by click's owninttype coercion during argv parsing, before any command body runs, so no in-body context manager can see it. Verified: exit 2, zero bytes on stdout. Fixing it needs a handler aroundapp()inmain(), which is a different change from this one — flagging it rather than quietly widening scope.BadParametersites are deliberately untouched (the mutual-exclusion validator, the nightly-commit guard). They are a possible follow-up, not this change.EPIPEthe envelope write raises instead of being swallowed._write_json_linedeliberately does not catchOSError(its comment explains why:comfy cloud logindepends onBrokenPipeErrorpropagating), so a hung-up reader surfacesBrokenPipeErrorrather than exit 2. That is the pre-existing, documented behavior of everyrenderer.error()call site, and nothing can be delivered to a closed pipe anyway — so this follows the existing contract rather than inventing a local exception to it.commandfield comes fromrenderer.command, set in the entry callback, so no plumbing was needed.