feat(run): add --json NDJSON output mode for agents#455
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds v1 NDJSON machine-readable event streaming to ChangesNDJSON Output Feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #455 +/- ##
==========================================
+ Coverage 81.79% 83.02% +1.22%
==========================================
Files 43 45 +2
Lines 5982 6668 +686
==========================================
+ Hits 4893 5536 +643
- Misses 1089 1132 +43
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bigcat88
force-pushed
the
dev/json-protocol
branch
from
942edac to
a8d8ace
Compare
bigcat88
changed the title
`comfy run --json` switches the CLI into a machine-readable mode that emits one JSON event per line on stdout, terminated with a `completed` or `failed` event. Designed for LLM agents and automation that need a parseable view of a workflow execution. Wire format (see docs/json-output.md for the full v1 contract): - 8 event types: converted, queued, node_cached, node_executing, node_progress, node_executed, completed, failed - 17 stable error.kind values covering every failure path - schema_version: 1 on every event for forward compatibility - ASCII-safe (ensure_ascii=True), per-line flushed - queued carries a node manifest so piped consumers can render UI immediately without parsing the workflow file - node_progress carries class_type+title so stateless consumers don't need to track a prior node_executing - executed_node_ids is the union of nodes observed via node_executing or node_executed (covers intermediate compute nodes that never fire the server's executed WS message) Implementation: - JsonEmitter class owns state (start_time, prompt_id, client_id, workflow, output aggregation) and writes events - Bounded timeouts on every network operation (pre-flight HTTP probe, ws.connect, /prompt POST, ws.recv) so the contract's terminal-event guarantee holds even when the server hangs - Duck-typed output filter: any dict with a `filename` key surfaces as an Output, regardless of the ComfyUI category bucket - execution_interrupted WS handled as `interrupted` error.kind - --verbose is a no-op in JSON mode (would otherwise corrupt the stream) Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
bigcat88
force-pushed
the
dev/json-protocol
branch
from
a8d8ace to
327b197
Compare
dosubot
Bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/command/run.py`:
- Around line 455-467: The except WebSocketTimeoutException handler should
capture the exception (e.g., except WebSocketTimeoutException as e:) and
re-raise typer.Exit with explicit chaining (raise typer.Exit(code=1) from e);
also shorten the overlong print/f-string by splitting it into a variable or
concatenated strings so no line exceeds 120 chars (the long pprint f-string that
references timeout and the suggestion text should be wrapped or assigned to a
short variable before calling pprint).
- Around line 305-315: The file existence check currently calls
os.path.isfile(workflow) using the raw CLI argument; change it to use the
expanded absolute path stored in workflow_name so tilde and relative paths are
handled correctly — replace the os.path.isfile(workflow) check with
os.path.isfile(workflow_name) and ensure the error paths that call
emitter.emit_failed("workflow_not_found", ...) and pprint(...) reference
workflow (or workflow_name) consistently before raising typer.Exit so messages
report the same, expanded path.
- Around line 317-325: The pre-flight probe in execute() ignores the caller's
timeout and calls check_comfy_server_running() without passing timeout, causing
it to use its default 5s; update the call to check_comfy_server_running(host,
port, timeout=timeout) (or equivalent positional arg order used in that
function) so the execute(timeout=...) parameter is forwarded and the probe
respects the requested timeout while preserving the existing json_mode/emitter
and exit behavior.
In `@docs/json-output.md`:
- Around line 520-593: The fenced code blocks showing NDJSON examples (e.g., the
blocks starting with lines like {"event":"converted","schema_version":1,...},
{"event":"queued",...}, and the various failure examples containing
"event":"failed" / error objects) should include a language specifier for syntax
highlighting; update each opening fence from ``` to ```json (or ```jsonl) for
those example blocks so the rendered docs highlight the JSON/NDJSON content.
In `@tests/comfy_cli/test_env_checker.py`:
- Around line 57-59: The test currently only checks that the "timeout" key
exists on mock_get.call_args.kwargs; change that assertion to verify the actual
value is correct by asserting mock_get.call_args.kwargs["timeout"] == 5.0
(keeping the existing mock_get.assert_called_once() and URL assertion intact) so
the test fails if the timeout regresses.
🪄 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
Run ID: 6d2465ed-fe61-4f5d-bac7-2ebe74b604e8
📒 Files selected for processing (7)
comfy_cli/cmdline.pycomfy_cli/command/run.pycomfy_cli/env_checker.pydocs/json-output.mdtests/comfy_cli/command/test_run.pytests/comfy_cli/command/test_run_json.pytests/comfy_cli/test_env_checker.py
Contributor
Author
|
Additional findings from the Agent that does not have clue about this feature(clear memory): Telemetry(point 2) is critical, agent tried 3 times with constant failures.. |
The first-run consent prompt blocks subprocess pipes (corrupts --json output) and CI runs. Skipping the prompt entirely would also skip the event, leaving a permanent blind spot for agentic usage. When stdin or stdout is not a TTY and consent has not yet been recorded, enable tracking for the current process and persist a stable anonymous user_id so repeat agentic usage from the same machine attributes to one identity. The consent flag itself stays unset so a later interactive run can still prompt the human; if they consent, init_tracking reuses the existing user_id. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
ComfyUI's dict-keyed-by-node-id shape forced consumers to iterate via `.items()` and made `for w in event["validation_warnings"]` (the intuitive pattern) yield bare node-id strings. The empty case also had to be `null` to distinguish "no warnings" from an empty container, which crashes any consumer that doesn't guard the access. Switch both `queued.validation_warnings` and `validation_error.node_errors` to an array of self-contained records where each entry embeds `node_id` as a field. The empty case is now an empty array, so iteration is always safe and the shape matches the convention used by LSP diagnostics, GraphQL errors, and similar event-stream protocols. Update the doc, examples, and tests to match. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
`comfy run` accepts both ComfyUI API format and exported UI format JSON, but the help text and the --json contract doc both read as if API format were the only input. An agent reading `comfy run --help` had no reason to believe UI exports would work and could waste effort converting them upstream. Update: - the command-level help, --workflow help, and --json help to mention both formats and the client-side conversion; - the json-output.md Overview with a dedicated "Workflow input format" paragraph that points at the existing `converted` event; - the user-facing `workflow_format_invalid` message to name both expected formats instead of just "API workflow JSON". Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Two related changes that together make `local_path` populated more often when the agent is on the same machine, while guaranteeing it stays null when it shouldn't: * `is_local_host` recognises `127.0.0.1`, `localhost`, `::1`, `[::1]`, and `0.0.0.0` (case-insensitive, string-match only — no DNS). * `_resolve_run_endpoint` no longer punishes overrides that match the recorded background. Passing `--host 127.0.0.1` (or `localhost`, via loopback aliasing) or `--port <bg_port>` that matches the background still produces `local_paths=True`. Mismatched overrides still disqualify, since we don't know which workspace the other endpoint serves. * `_build_output_object` and `format_image_path` both re-check `is_local_host(self.host)` before filling `local_path`, so a future Cloud code path that accidentally inherits `local_paths=True` can never surface a phantom filesystem path. Update `docs/json-output.md` Local paths section to spell out the loopback aliasing, the case-insensitive match, the workspace-lookup prerequisite, and to drop the "loopback" misnomer (the set includes `0.0.0.0`, which is a wildcard). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Agent feedback: `local_path` was effectively always null in practice because the previous logic required `comfy launch --background` to have been used. Agents launching ComfyUI manually (or against an already-running instance) never saw the field populated even though the file was sitting right there on the same machine. Replace the `--background` prerequisite with an empirical check. We still require the host to be a known same-machine address (Cloud and remote stay null), and we still rely on the workspace manager to resolve a candidate path, but the decisive gate is now whether a regular file actually exists at the computed path. This makes `local_path` an honest promise: when it's non-null, the file is there; when the file isn't, the field is null instead of a phantom path. As a side effect the `local_paths` constructor argument loses its load-bearing role; it remains accepted for API stability but the value is no longer consulted. Also surface the attempted endpoint and an override hint in the pre-flight `connection_error` messages — agents that take defaults silently used to get a bare "cannot connect" without knowing the CLI had picked 127.0.0.1:8188 for them. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Follow-up to "populate local_path whenever the file actually exists", addressing three review findings: * workspace_manager.get_workspace_path() has side effects on the stale-recent code path — it prints a warning and writes the config. In JSON mode the rich-formatted warning would land mid-stream and corrupt NDJSON, and the config rewrite would happen once per output event. Cache the resolved path on WorkflowExecution so the call happens at most once per run. * Server-controlled subfolder / filename could escape the workspace via `..` segments or a leading `/`, because os.path.join collapses or rebases on those. Confine the candidate path to <ws>/<file_type>/ before the existence check. * `0.0.0.0` is a wildcard bind, not a connect address. Linux quietly resolves it to a loopback, but macOS and Windows can't reach http://0.0.0.0:port. Substitute the canonical loopback at execute() entry so server probe, /prompt POST, and emitted /view URLs are portable. Also tighten the doc: the local_path schema row no longer claims "can't verify local access" (the check is now an on-disk existence test), and a new lifetime caveat warns that `type == "temp"` paths are read-immediately-only since ComfyUI may clean them up. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Agent testing kept turning up the same complaint: `local_path` was null on every output, even with ComfyUI running locally, because the heuristic for resolving ComfyUI's output directory (workspace_manager + existence check) doesn't survive real-world deployments — manual launches, alternate install paths, multi-install machines, containers with bind-mounted volumes. The field promised something we couldn't reliably deliver. ComfyUI exposes no API for its output directory, so the heuristic was the best we could do. Make the contract honest: every output object now carries only `url`, and agents always go through it. That's how Replicate, Modal, Horde, Cargo and friends already work, and it's how a future Cloud target will have to work anyway — so agents write one URL-fetch path that's universal instead of two paths that drift. Removed: `is_local_host`, `_LOCAL_HOSTS`, the `local_paths` parameter on `execute()` / `WorkflowExecution`, `_resolve_workspace_path`, `_candidate_local_path`, `_resolve_run_endpoint`, `_hosts_equivalent`, and ~120 LOC of tests pinning their behaviour. Kept: a minimal local-path resolver inside `format_image_path` so plain `comfy run` (non-JSON, human terminal output) still prints a clickable path when ComfyUI is on a loopback and the file resolves. The text-mode helper is contained and doesn't feed the JSON contract. `schema_version` stays at 1 — v1 has not shipped to users yet, so this is field removal during development, not a breaking change. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Two small follow-ups to the local_path removal: * `format_image_path` calls `workspace_manager.get_workspace_path()`, which can print a warning and rewrite the config on the stale-recent path. Memoize per WorkflowExecution so a workflow with N outputs doesn't fire those side effects N times. In text mode this is only a stdout noise / disk churn nit (not a JSON stream corruption risk like in the previous JSON contract), but the fix is one tiny helper. * The docstring still leaned on the phrase "local_path" which now reads as if it refers to the JSON contract field we just removed. Reword to "absolute filesystem path" so the helper's purpose is clear without referencing the deprecated name. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Agents working with UI-format workflow exports had no way to preview the conversion without paying GPU time or importing `comfy_cli.workflow_to_api` from Python (which still needs a live ComfyUI for `/object_info`). `--print-prompt` closes that gap: it runs the full pre-flight up through conversion, prints the workflow body that *would* have been POSTed to `/prompt`, and exits 0 without ever hitting the queue. Behaviour: * API-format input: no server hit at all (the server probe is skipped when `--print-prompt` is set). Works against an offline ComfyUI. * UI-format input: still fetches `/object_info` for the converter; unreachable host surfaces as the usual `connection_error`. * In JSON mode emits a new terminal `prompt_preview` event carrying just the workflow graph — no `client_id`, no `extra_data` (so the `--api-key` value doesn't leak to stdout). * In text mode pretty-prints the workflow JSON. * Pre-flight failures (workflow_not_found, workflow_format_invalid, conversion_error, etc.) still go through the normal `failed` path and exit 1. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The prior one-line comment read "API-format input doesn't need a server at all" without acknowledging that UI-format input does still need /object_info for the converter. The new wording spells out both cases: API works offline end-to-end, UI surfaces the same connection_error kind a few lines later (just from fetch_object_info instead of the explicit probe). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The pre-flight existence check used the raw CLI argument while the actual open() used the expanded path. A scripted caller passing literal `~/wf.json` (or anything with shell variables / a leading `~` that wasn't shell-expanded) got a misleading `workflow_not_found` even when the file existed at the resolved location. Use the already-computed `workflow_name` (which goes through `os.path.expanduser` + `os.path.abspath`) for both the check and the error message, so the user sees the actual path the CLI tried. Regression flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
`check_comfy_server_running()` defaults its `timeout` to 5.0s, but the call site in `execute()` was passing only `(port, host)`. A user running against a slow-to-respond (but otherwise live) ComfyUI with `--timeout 60` got a misleading "not running" report from the probe because the underlying GET timed out at 5s while the rest of the pipeline was happy to wait. Pass `timeout=timeout` through so the probe respects the same bound as `/prompt` POST and the websocket operations. Regression flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The existing test asserted only that `timeout` was passed; the value was treated as implementation detail. But the 5.0s default IS the user-visible "is the server up?" contract — if it silently dropped to 1s, real users on slow hosts would see spurious negatives, and this test wouldn't catch it. Assert the exact value, and add a sibling test for the caller-override path so both directions are pinned. Regression flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The inline per-event examples earlier in the doc already used \`\`\`json, but the long NDJSON archetypes in the Examples section dropped the tag. Adds it for the seven affected blocks (success, --no-wait, workflow_not_found, validation_error, execution_error, timeout, interrupted) so rendered docs (GitHub, IDE previews) get syntax highlighting on every JSON-looking line, and markdownlint MD040 is satisfied. Regression flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
* `prompt_tracking_consent` (non-interactive branch) called `config_manager.set(...)` without guarding the disk write. On a fresh CI / sandbox with a read-only or missing `~/.config/comfy-cli`, the resulting `FileNotFoundError` / `PermissionError` propagated out of the typer entry callback BEFORE `comfy run` started — agents got a Python traceback on stderr and no terminal `failed` event on stdout, violating the documented stream contract. Wrap the write in a best-effort try/except so the in-memory `_session_only_tracking` state still takes effect and this process tracks normally. * `on_error` passed `node_id` straight through to `emit_failed` from the server's WS payload. Every other `node_id`-bearing emitter coerces to `str` defensively (line 175, 188, 204, 218) — only `execution_error` was missing the coercion, so a server that emitted an int `node_id` would have produced a contract-violating int field. Latent today (ComfyUI always sends a string) but cheap insurance. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Final pre-merge review surfaced a handful of places where the doc overstated, understated, or mis-described what the code does: * `completed.elapsed_seconds` row claimed "from prompt POST" but the clock starts at JsonEmitter __init__ (top of execute()) — same reference point as `failed.elapsed_seconds`. Aligned the wording. * `validation_error` example had the fallback message string (`"Workflow failed validation"`), but with the example's populated `node_errors` the code would pull and emit the inner `errors[0].message` (`"Value not in list"`). Fixed the example. * `connection_error` trigger description listed only "URLError / pre-queue socket.timeout" — the code also catches `TimeoutError` and other `OSError` paths from the /prompt POST. Broadened. * `object_info_unavailable` trigger said "HTTP error" but the kind also fires on HTTP 200 with an unparseable body (line 311 in run.py). Broadened. * `title` fallback chain in every event row stopped at `class_type`, but `get_title()` also falls back to `node_id` when class_type is missing/empty/non-str. Documented the full chain. * `subfolder` and `type` defaults when the server omits them weren't documented; agents seeing `"output"` couldn't distinguish server-said vs CLI-defaulted. Noted both. * `exception_type` can be `""` when the server omits it. Noted. * `--no-wait` makes `queued` the terminal event for the invocation, but the event table marked only `completed`/`failed`/`prompt_preview` as terminal. Added a conditional ✓ to the `queued` row. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Agent feedback: --print-prompt is one-shot — to debug a conversion you have to run twice (once to preview, once to execute), and the two runs hit /object_info separately so they can in principle disagree. There's also no audit trail of what the CLI actually submitted for runs that fail server-side or produce unexpected outputs. Make the prompt_preview event a default, always-on part of the --json stream. It's emitted immediately after the optional `converted` event and immediately before `queued`, on every run regardless of input format. Agents get full provenance for free; the run-twice debugging pattern goes away. prompt_preview becomes non-terminal in the normal flow and terminal only under --print-prompt (which keeps its existing "preview and exit" semantics). The body is the workflow graph — no client_id, no extra_data, so the --api-key value still never appears in the stream. Doc: event-table marker switched from terminal-always to terminal-in- --print-prompt; stream-archetypes table gains the new event in every non-pre-flight row; six example NDJSON snippets gained the line so they match what the implementation now emits; --json help text mentions the audit-trail property. Tests: TestPromptPreviewAlwaysEmitted pins the always-on behaviour (API input, UI input, and the no-secrets-in-body invariant); the no-wait shape test updated to assert prompt_preview-before-queued. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The 30s default tripped agents on cold-start runs of big-model workflows (Flux dev fp16, Wan-2.1-i2v-14B, etc.) — the queued → first-`node_executing` gap is dominated by checkpoint load on cold disk + cold VRAM, which routinely exceeds 30s for FP16 models in the >10 GB range. Once the first event arrives, per-event gaps drop to milliseconds and the timeout is effectively irrelevant for the rest of the run. 120s comfortably covers the load-from-cold case for current popular checkpoints while still catching a genuinely stuck server an order of magnitude faster than the typical workflow's wall-clock execution time. Agents that need more can still set `--timeout` explicitly. No semantic change otherwise — same per-event meaning, same fallback on stuck recv(), same value flowed through to the HTTP / WS handshakes. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/command/run.py`:
- Around line 835-862: The WebSocket handlers assume message and data are dicts
and directly index keys which can raise on scalar/array payloads; update
on_message to first verify message is a mapping/object (and return True if not),
then extract data safely via a dict or {} only when isinstance(message, dict)
and isinstance(message.get("data"), dict), and similarly in on_executing ensure
data is a dict before accessing data["node"] (use data.get("node") or an
isinstance check) and handle missing node by returning False; add these type
guards around uses of self.prompt_id, msg_type dispatching, and data["node"] to
avoid exceptions tearing down the run.
🪄 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
Run ID: 2192579a-6e8d-4106-bf61-210124618934
📒 Files selected for processing (8)
comfy_cli/cmdline.pycomfy_cli/command/run.pycomfy_cli/tracking.pydocs/json-output.mdtests/comfy_cli/command/test_run.pytests/comfy_cli/command/test_run_json.pytests/comfy_cli/test_env_checker.pytests/comfy_cli/test_tracking.py
`on_message` indexed `message["data"]` (gated by `"data" in message`, which raises TypeError on non-iterable scalars like None) and `on_executing` indexed `data["node"]` (raises KeyError if the server sends an `executing` frame without it). A misbehaving sender — bad proxy, future protocol change, or even a custom node misusing the WS — could tear the recv loop down with an uncaught TypeError/KeyError that propagates all the way out of `comfy run`, leaving the agent with a Python traceback on stderr and no terminal `failed` event on stdout. Violates the documented stream contract. Defensive guards: * on_message: isinstance(message, dict) AND isinstance(data, dict), else return True (skip frame, keep recv'ing). * on_executing: `"node" not in data` returns True (skip — a missing key is a protocol violation but not necessarily "done"; let the next properly-shaped frame decide). Regression flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/command/run.py`:
- Around line 446-455: The current flow returns after
emitter.emit_prompt_preview(workflow) when both json_mode and print_prompt are
set, so the stream never emits a terminal event; update the logic so that when
json_mode and print_prompt are both true you call the emitter's terminal event
(e.g., emitter.emit_completed(workflow) or emitter.emit_failed(...) as
appropriate) immediately after emitter.emit_prompt_preview(workflow) and before
returning; keep the existing non-json print path unchanged.
- Around line 886-895: on_cached currently assumes data["nodes"] is iterable and
will crash on malformed payloads; validate and normalize execution_cached.nodes
before iterating: in on_cached, retrieve nodes = data.get("nodes") and if it's
not a list/tuple/set or otherwise iterable of node identifiers, convert single
values to a single-item list or safely skip non-iterables, and ensure each
element is not None before calling self.remaining_nodes.discard(n),
self.log_node("Cached", n), and self.emitter.emit_node_cached(n); after the loop
still call self.update_overall_progress() so malformed payloads don't tear down
the run.
- Around line 815-833: The _build_output_object function currently only returns
a web view URL; update it to also populate a local_path when the file exists on
disk by constructing the expected filesystem path (using the node's
workspace/root + subfolder + filename), checking os.path.exists (or Path.exists)
and, if present, adding "local_path": "<absolute_path>" to the returned dict
(leave it out or set to None when not found); keep existing fields and leave
emitter.get_class_type(node_id) and emitter.get_title(node_id) as-is so the
object includes both URL and local filesystem location for loopback/workspace
runs.
In `@tests/comfy_cli/command/test_run_json.py`:
- Around line 1265-1282: The test test_output_object_has_no_local_path_field is
too strict about banning local_path; instead, remove the assertion that
"local_path" must not exist and assert required fields only: ensure out0
contains a "url" and that out0["url"].startswith("http://"); if "local_path" is
present on out0 (after ex.on_executed), validate it conservatively (e.g. is a
non-empty string or matches expected filesystem path shape) rather than failing
the test—adjust the assertions around out0 accordingly.
- Around line 61-84: The test helper _run_execute_capture currently calls
execute() directly and bypasses the CLI entrypoint, so add a new test that
invokes the CLI entrypoint (via the Typer/Click runner) in a
non-TTY/non-interactive mode with --json to replicate pre-execute behavior; call
the app entrypoint (rather than execute()) and assert the first stdout line is
valid JSON and that no telemetry/consent prompt text appears in stdout (i.e.,
ensure strings like "telemetry" or "consent" are absent), so the suite catches
the non-interactive telemetry prompt regression that _run_execute_capture skips.
🪄 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
Run ID: 31d13ef1-11bc-4339-ae26-e149a2e21933
📒 Files selected for processing (2)
comfy_cli/command/run.pytests/comfy_cli/command/test_run_json.py
The event-reference table and stream-archetype table already mark
prompt_preview as the terminal event under --print-prompt, but the
prose in three other places ("Stream shape", "Process-level
termination", and the Examples preamble) still only mentioned
completed/failed. A reader scanning the prose could conclude that
--json --print-prompt violates the terminal-event contract, when in
fact the contract treats prompt_preview as a third terminal kind in
that mode (and a successful exit 0, since no execution was attempted).
Bring the prose into line with the tables, including the exit-code
note and the Stability section's "exit code is always 0 or 1" recap.
The alternative — emitting `completed` after `prompt_preview` in
--print-prompt mode — would be a category error: `completed` carries
elapsed_seconds, outputs, executed_node_ids whose semantics require
that the workflow actually ran. Emitting `completed` with empty
executed_node_ids for a never-started run would lie to agents
dispatching on event kind. Keeping `prompt_preview` as the terminal
event is the honest design.
Doc-only nit flagged by CodeRabbit on PR #455.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The existing JSON tests call execute() directly via _run_execute_capture and bypass the typer entry callback chain (where consent prompt / decorator / config init happen pre-execute). That's exactly where the original first-run-corrupts-stdout bug lived. If anyone adds a stray print() or pprint() in cmdline.py, none of the existing unit tests would catch it. Two integration tests via typer.testing.CliRunner (which is non-TTY by default — i.e., the agent scenario): * test_cli_json_print_prompt_emits_clean_ndjson: smoke, default config state. Every stdout line parses as JSON with `event` and `schema_version`. Consent prompt text strings absent. * test_cli_json_with_fresh_consent_state_stays_clean: the actual regression scenario — fresh tmp config dir (consent never recorded), mocked Mixpanel client. Verifies the session-only tracking branch fires silently and stdout stays NDJSON-clean. Both use --print-prompt so the tests don't need to mock /object_info or websocket plumbing. The CliRunner integration seam is what's being tested, not the workflow lifecycle. Gap flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
`test_output_object_has_no_local_path_field` was pinning the absence
of a field that never existed pre-PR — JSON output mode is new, so
there's no historical baseline where local_path was emitted. The
test caught a narrow scenario (someone re-adding `"local_path": None`
to `_build_output_object`) but at the cost of being a regression test
for something that wasn't a regression.
The deprecation rationale is still load-bearing in
`docs/json-output.md` ("Agents should rely on `url` exclusively") and
the contract section that lists the output object's fields. Reviewers
considering a future re-introduction would land in the doc first,
not be ambushed by an oddly-named "must NOT have" test.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/json-output.md`:
- Around line 655-660: The Stability section currently contradicts earlier
contract text by omitting the `queued`/`prompt_preview` success exit codes and
mis-describing stream guarantees; update the text so the exit code mapping
explicitly includes `queued` and `prompt_preview` as success exits (in addition
to `completed` -> 0 and `failed` -> 1) and correct the stream separation
sentence to state that clean NDJSON is guaranteed on stdout (and that stderr may
contain progress/other logs), and ensure these changes are applied where `exit
code mapping` and `stdout/stderr separation` are mentioned as well as where
`schema_version: 1` and `v1 streams` are referenced so the Stability section
matches the earlier event/field contract.
🪄 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
Run ID: 5190ea1f-8d6e-4352-aec2-6b73cd015f1f
📒 Files selected for processing (2)
docs/json-output.mdtests/comfy_cli/command/test_run_json.py
Two inconsistencies between the Stability section's "What is stable" bullets and the Overview section earlier in the doc: * The exit-code bullet only mentioned `completed`→0 and `failed`→1, dropping the `queued` (--no-wait) and `prompt_preview` (--print-prompt) success paths that the Overview already documents. * The stdout/stderr separation bullet said "NDJSON on stdout, no progress on stderr" — the second clause is technically true but uninformative; the actual contract (per the Overview) is that stdout is NDJSON-only with no human-readable progress bar / ANSI, and stderr carries Python errors / library warnings that agents should not parse. Expand both bullets to match what the Overview says, so the contract sings one tune throughout. Doc-only nit flagged by CodeRabbit on PR #455. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
cleanup Four findings from a fresh full-PR review: * `_build_output_object` stored `node_id` directly from the server WS frame without `str()` coercion. The top-level `node_executed.node_id` IS coerced (in `emit_node_executed`), but each item inside `outputs[]` was passing through whatever shape the server sent. If ComfyUI ever sends an int `node` field, agents would see a contract violation (`node_id` documented as str). One-line fix. * Rename `error.kind = "interrupted"` → `"execution_interrupted"`. Parallels the existing `execution_error` kind, matches ComfyUI's source WS message name (the `msg_type` dispatch already uses `"execution_interrupted"`), and removes the only "bare past- participle" kind from the taxonomy. v1 hasn't shipped, contract is still mutable. * Test `test_connection_lost_on_malformed_frame` was misleadingly named — body asserts `event == "completed"` (the run survives a malformed frame), NOT a `connection_lost` failure. Renamed to `test_malformed_frame_is_skipped_run_completes`. * `test_object_info_unavailable_on_http_error` had a dead `mock_open.side_effect = _make_http_error(500, ...)` line immediately overwritten by a manually-built 503 HTTPError two lines later. Removed. Also adds a regression test pinning the outputs[i].node_id coercion. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Seven inconsistencies / gaps a fresh full-PR review surfaced — all doc-only, all in the same file: * `prompt_preview` was described as "Always emitted in --json mode" but the pre-flight failure archetype shows the stream is just `failed` with no prompt_preview. Reworded to spell out the actual invariant (emitted on every stream where the workflow was loaded, parsed, and converted — i.e., everywhere except the five pre-flight failure kinds that fail before conversion). * The `prompt_preview.prompt` field was described as "prompt body" (event table), "workflow body" (prose), and "workflow graph" (field table) within ~50 lines. Unified to "workflow graph" since the value is a node-id-keyed dict, not an HTTP body. * The `execution_error` row's `title (str)` extras cell didn't describe the fallback chain, even though every other place `title` appears spells it out as `_meta.title → class_type → node_id`. Inlined the chain in the cell. * `class_type` and `exception_type` are both open-valued (any custom node, any Python exception) but only `category` and `type` carried the **Open set** marker. Added the marker plus a one-line description to both (`class_type` in `queued.nodes` row, since that's its first authoritative table mention; `exception_type` in its own subsection). * `node_progress.value` / `max` default to `0` when the server omits them — the code uses `data.get(..., 0)` — but the field table didn't say so. Added the default. * `subfolder` said "omits or empties"; `type` said "omits". Both use the same `or`-fallback in code, so both treat missing AND empty identically. Aligned the wording. * The 7-bit ASCII / `ensure_ascii=True` invariant was in the Overview but absent from the "What is stable" bullet list under Stability. Added a bullet so anyone scanning the contract guarantees sees it. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Four follow-ups from a fresh full-PR review:
* The earlier doc-only commit `16a278c` unified the field terminology
to "workflow graph" inside `docs/json-output.md`, but missed five
occurrences outside it: three in `cmdline.py` flag help (--json,
--print-prompt, --api-key), one in a `run.py` code comment, and one
in a test-class docstring. Finished the rename.
(`--api-key` help also rephrased from "the prompt body" to "POST
/prompt request body" — that one was about the HTTP envelope, not
the workflow graph itself, and "prompt body" elided the distinction.)
* `prompt_preview` rewording in the prior commit enumerated five
"pre-flight failure kinds" (workflow_not_found, workflow_invalid_json,
workflow_read_error, workflow_format_invalid, workflow_empty), but
the code actually fails before prompt_preview in nine paths — also
`connection_error`, `object_info_unavailable`, `conversion_error`,
`conversion_crash`. Switched the enumeration to a generic reference
to the "Failure pre-flight" stream archetype, which the table
already covers correctly.
* All six long NDJSON example archetypes carried humanized titles
("Nano Banana 2", "Save Image") in their `queued.nodes` / per-node
events / output objects, but the corresponding `prompt_preview`
objects had no `_meta.title` on either node — meaning the code's
fallback chain would emit `"GeminiNanoBanana2"` / `"SaveImage"`
instead. Added `_meta.title` to both nodes in every prompt_preview
example so the titles downstream are reproducible.
* `validation_error` extras row said "(array of dict; see below)" but
the actual shape section is 27 lines further down (past
`exception_type` and `traceback` subsections). Replaced with an
explicit markdown anchor link to the shape section.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
…th archaeology Two doc polish items from the latest review pass: * `class_type` was marked **Open set** only in the `node_cached` row, even though it appears in five other rows with identical semantics (queued.nodes, node_progress, node_executed, output object, execution_error). Repeating the marker on every row would be noisy. Instead, added a short paragraph at the top of the Event reference section that names the four open-set fields (`class_type`, `category`, `type`, `exception_type`) and explains the convention once. Each field still carries its canonical **Open set** marker at one description; the new paragraph tells readers it applies consistently across every row. * The `local_path` removal paragraph in the "Fetching output bytes" section read as CHANGELOG-style archaeology — five lines listing why we don't emit it. New readers of a reference doc just need to know the field doesn't exist; two lines covers that. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Three small fixes: * `cmdline.py`'s `--timeout` was annotated `int | None` but nothing in the code handles `None` — the default is `120` and typer never emits `None` for an int option with a default. Tighten to `int`. * `execute()` and `WorkflowExecution.__init__` still had the old `timeout=30` default, even though the user-facing `--timeout` was bumped to `120` in commit `dd2c80a`. Tests pass `timeout` explicitly in every code path that cares about its value, so this is mostly cosmetic — but it removes a confusing dual source of truth. * `test_run_json.py:362` still said "submitted body" in a comment; the workflow-graph rename in commit `c1e1fed` missed this one. Aligned with the rest of the renamed sites. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/command/run.py`:
- Around line 877-885: The node IDs coming from the websocket must be normalized
to strings before mutating execution state: convert node_id to str immediately
after reading data["node"], then use that string when discarding from
remaining_nodes, when assigning self.current_node, and when calling log_node and
emitter.emit_node_executing (refer to the block using current_node,
remaining_nodes, update_overall_progress, log_node, and
emitter.emit_node_executing). Apply the same normalization to the other handler
block that mutates state (the block around where node completion is
processed—the one referenced by the reviewer as also applying to lines 892-895)
so every place uses str(node_id) consistently before any state changes.
🪄 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
Run ID: ca769c86-03e1-4eb3-be6c-17bc8ed05ac7
📒 Files selected for processing (4)
comfy_cli/cmdline.pycomfy_cli/command/run.pydocs/json-output.mdtests/comfy_cli/command/test_run_json.py
* Introduce JsonEmitter.fail() helper that emits a failed event, prints the red text-mode message, and returns typer.Exit(1) for the caller to raise. Migrates ~10 sites; preserves the explicit if-json-mode pattern where text-mode needs multi-colour Rich markup. * Hoist _MAX_BODY_PREVIEW constant (replaces 5x [:500] literals and fixes 2x [:200] mismatches in HTTP body previews). * Add WorkflowExecution._view_url() to deduplicate /view URL building. * Inline get_node_title() into the two callers (emitter.get_title). * docs: canonicalize the title-fallback rule and Open-set convention into single paragraphs, replacing 6+ in-table repetitions. * tests: parametrize 5 HTTP-status routing tests, extract a single _make_workflow_execution factory, drop a stale docstring bullet, and pin JsonEmitter.fail()'s three behaviours (JSON emission, text-mode wrap, rich_message override). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
…pt body json.loads(bytes) runs encoding detection before parsing — a body whose first bytes look like a UTF-16/UTF-32 BOM (e.g. b"\x00\x01\xff\xfe...") steers it down the UTF-16 decode path, which raises UnicodeDecodeError on invalid sequences. The previous handler only caught JSONDecodeError, so the exception escaped: no terminal failed event was emitted (breaking the contract for exit 1) and a multi-KB Rich traceback was printed to stderr (breaking the "stderr stays clean in JSON mode" guarantee). Plausible trigger in the wild is a misconfigured reverse proxy serving a UTF-16-encoded HTML error page on the /prompt route. Apply the same fix to _handle_submit_http_error where the pattern is mirrored. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Without this, KeyboardInterrupt would escape execute() and Python's default handler would tear the process down with exit code 130 — but the JSON stream had no terminal event, violating the exit-code-non-zero-implies-failed-event invariant. Reuse the existing execution_interrupted kind: from a consumer's perspective, the remediation is identical to a server-side /interrupt and the protocol stays smaller. Broaden the doc row to cover both sources. The new handler sits inside execute()'s existing try block so it only catches Ctrl-C during connect / queue / watch_execution. Ctrl-C during the pre-flight (file read, server probe, conversion) is still uncaught — a narrow window, accepted for now. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--jsonflag tocomfy runthat switches stdout to NDJSON (one event per line, terminated withcompletedorfailed).error.kindvalues; every event carriesschema_version: 1.ensure_ascii=True), per-line flushed, no Rich/ANSI on stdout.queuedincludes anodesmanifest so piped consumers can render per-node UI without parsing the workflow file.node_progresscarriesclass_typeandtitleso stateless consumers don't need to track a priornode_executing.executed_node_idsaggregates every node the executor touched, not just nodes that emit the server'sexecutedWS message.ws.connect,/promptPOST, andws.recvso the terminal-event guarantee holds even when the server hangs.docs/json-output.md; contract tests intests/comfy_cli/command/test_run_json.py.