feat(run): convert UI workflows to API format client-side via /object_info#450
Merged
Conversation
Adds comfy_cli/workflow_to_api.py: a pure-Python converter that takes
a UI-format (litegraph) workflow and the running server's /object_info
response and produces an API-format ("prompt") workflow ready for
execution. Translates the JSON-in/JSON-out algorithm of Seth A.
Robinson's /workflow/convert custom node (Unlicense) to consume the
HTTP /object_info schema instead of importing ComfyUI's nodes module.
Conversion covers:
* Recursive subgraph expansion with link ID remapping
* PrimitiveNode value inlining
* Reroute / GetNode / SetNode pass-through tracing
* Mode 4 (bypass) passthrough with strict isValidConnection matching
and a first-linked-input fallback to match the reference converter
* Mode 2 (mute) exclusion plus orphan link cleanup (the reference
leaves a dangling reference here; the executor would reject it)
* LoadImageOutput / Note exclusion
* OUTPUT_NODE preservation for sinks without downstream consumers
* Combo value case-insensitive normalization
* Schema-driven default value population
* control_after_generate filtering before widget mapping
* COMFY_DYNAMICCOMBO_V3 sub-input expansion
* rgthree-style lora dict widget absorption
* List widget value wrapping in {__value__: [...]} to disambiguate
from [node_id, slot] link references
* Warning when legacy 'group nodes' (workflow> prefix) are encountered
Hardened against malformed input: non-dict node/input/output entries,
non-numeric link slots, garbage in definitions etc. are skipped with a
warning rather than crashing. Per-node emission is wrapped so a bug in
one node cannot torpedo the whole conversion.
Test coverage (62 tests):
* Format detection, UUID detection
* Each special-case node type
* Schema-aware widget mapping, defaults, combo normalization
* Subgraph expansion
* Frontend parity behaviors (list wrap, orphan cleanup, isValidConnection)
* Malformed input hardening
* Fixture-based regression against a real SD 1.5 workflow + a trimmed
/object_info, asserting byte-identical output to the reference
/workflow/convert response
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The Part 1 flow POSTed UI workflows to /workflow/convert on the ComfyUI server, which only exists if a specific custom node is installed. This switches to fetching /object_info (always available on stock ComfyUI) and running the conversion locally via the new workflow_to_api module. * fetch_object_info(host, port, timeout) GETs /object_info and parses the response, with clean exits on HTTPError, URLError, TimeoutError and JSONDecodeError. * execute() detects UI format, fetches /object_info, then calls convert_ui_to_api. WorkflowConversionError surfaces as a typed user-facing message; any other unexpected exception is caught and surfaced as an experimental-feature crash report pointing at the issue tracker (full traceback only with --verbose). * Drop WorkflowConverterUnavailable, convert_ui_workflow_via_server and the help-text fallback — no longer needed. Tests follow the rewrite: dropped TestConvertUiWorkflowViaServer, added TestFetchObjectInfo, rewrote TestExecuteUiWorkflow against the new fetch_object_info + local converter flow, added a catch-all exception test that asserts a clean exit code 1. Smoke tested end-to-end against stock ComfyUI (no SethRobinson custom node, /workflow/convert returns 405) with SD 1.5, SDXL Turbo and a Z-Image-Turbo subgraph workflow. Parity sweep across 51 frontend blueprint workflows: 46 byte-identical to the reference converter; 5 differ only in cosmetic _meta.title for V3 nodes whose /object_info.display_name is None (the executor ignores _meta.title). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
|
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:
📝 WalkthroughWalkthroughFetch /object_info from the running server, convert ComfyUI "UI-format" workflows locally with convert_ui_to_api(), and queue the resulting API prompt for execution; added conversion error handling, SD1.5 fixtures, and comprehensive unit and CLI tests. A tiny rhyme — convert, don't post; safer and faster than ghost-host! ChangesWorkflow Conversion Implementation and Integration
🚥 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
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 #450 +/- ##
==========================================
+ Coverage 79.59% 80.65% +1.06%
==========================================
Files 35 36 +1
Lines 4479 5216 +737
==========================================
+ Hits 3565 4207 +642
- Misses 914 1009 +95
🚀 New features to boost your workflow:
|
_build_link_map ran twice: once before _rewrite_links_for_subgraphs and again right after. The first result was immediately shadowed and never read (the rewrite function doesn't take a link_map argument). Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
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: 3
🤖 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/workflow_to_api.py`:
- Around line 223-229: The loop that builds defs from subgraphs uses sg.get(...)
and later code assumes sg["inputs"]/sg["outputs"] are dicts; add defensive type
checks so malformed entries won't raise AttributeError: ensure each sg is a dict
(already checked) and verify sg.get("id") is a str-like key before assigning to
defs[sg_id], and before dereferencing inputs/outputs validate that
sg.get("inputs") and sg.get("outputs") are dicts (using isinstance(..., dict))
and skip or ignore entries that fail these checks; apply the same pattern to the
other locations referenced (the blocks operating on subgraphs at the ranges that
use sg.get("inputs")/sg.get("outputs") around the defs/subgraph processing) so
all uses of sg.get(...) are guarded against non-dict values.
- Around line 476-489: In _build_link_map, validate the extracted link_id before
using it as a dict key: skip the entry (continue) if link_id is not a simple
numeric/hashable ID (e.g., not an int) or otherwise not hashable to avoid
TypeError; perform this check after extracting link_id from link[:6] and before
assigning into link_map so malformed link IDs (lists/dicts/other non-hashables)
are safely ignored.
- Around line 534-546: The code assumes widgets[0] (var_name) is always a
hashable scalar before using it as a dict key in set_sources/get_vars; add a
guard in the function that computes set_sources and get_vars (the block handling
node_type "SetNode" and "GetNode") to verify var_name is a valid hashable scalar
(e.g., instance of str or int) and that widgets is a non-empty sequence before
using widgets[0]; if the check fails, skip that node (continue) so malformed
input (list/dict) doesn't raise TypeError when assigning to set_sources or
get_vars.
🪄 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: efe922f9-5dc7-4ec9-b10b-fe60047c667e
📒 Files selected for processing (7)
comfy_cli/command/run.pycomfy_cli/workflow_to_api.pytests/comfy_cli/command/test_run.pytests/comfy_cli/fixtures/sd15_expected_api.jsontests/comfy_cli/fixtures/sd15_object_info.jsontests/comfy_cli/fixtures/sd15_ui_workflow.jsontests/comfy_cli/test_workflow_to_api.py
…tries Subgraph expansion runs before the per-node try/except wrapper in convert_ui_to_api, so any AttributeError there propagates up to the caller. The run.py catch-all still produces a clean exit, but bailing out of the entire conversion on a single malformed subgraph def is worse UX than skipping it and converting the rest. Defensive isinstance checks added in three spots: * _collect_subgraph_defs: only accept str sg.id (the id is later used as a dict key and matched against node_type strings) * _outer_slot_to_input_idx: skip non-dict entries in sg.inputs and outer_node.inputs * _expand_one_subgraph: skip non-dict entries in sg.inputs and sg.outputs Confirmed via fuzz: 8/8 previously-crashing inputs now exit cleanly (0 nodes emitted from the malformed subgraph, conversion of the rest proceeds). Two new tests cover the cases. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
comfy_cli/workflow_to_api.py (3)
545-556:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
widgets_values[0]before using it as a variable key.Malformed
GetNode/SetNodepayloads can still put a list/dict inwidgets_values[0];set_sources[var_name] = ...then raisesTypeErrorduring preprocessing and aborts conversion. Restrict this to a supported scalar before storing it, or this gremlin keeps sneaking through.Minimal fix
if not isinstance(widgets, list) or not widgets: continue var_name = widgets[0] + if not isinstance(var_name, str) or not var_name: + continue if node_type == "SetNode": for inp in node.get("inputs") or []: if not isinstance(inp, dict): continue🤖 Prompt for 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. In `@comfy_cli/workflow_to_api.py` around lines 545 - 556, The code currently assigns var_name = widgets[0] and uses it as a dict key for set_sources and get_vars without validating its type; guard against malformed payloads by validating widgets[0] is a supported scalar (e.g. isinstance(var_name, str) or int and not a list/dict/None) before using it, and skip/continue when it isn't; update the branches that reference var_name (the SetNode handling that writes set_sources and the GetNode handling that writes get_vars) to perform this check and only store valid scalar keys.
487-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip malformed top-level link IDs in
_build_link_map.
link_map[link_id] = ...still assumeslink_idis a safe dict key. A malformed workflow link like[{}, 1, 0, 2, 0, "LATENT"]raises immediately and bypasses the per-node isolation this feature advertises. Same tune, different rune.Minimal fix
def _build_link_map(links: list) -> dict[int, dict]: link_map: dict[int, dict] = {} for link in links: if not isinstance(link, (list, tuple)) or len(link) < 6: continue link_id, src_id, src_slot, tgt_id, tgt_slot, link_type = link[:6] + if not isinstance(link_id, int): + continue link_map[link_id] = { "source_id": src_id, "source_slot": src_slot, "target_id": tgt_id, "target_slot": tgt_slot, "type": link_type, } return link_map🤖 Prompt for 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. In `@comfy_cli/workflow_to_api.py` around lines 487 - 499, The current _build_link_map function assumes link_id is a valid int key and will raise if a link's first element is malformed (e.g. a dict); to fix it, after unpacking link_id, validate it (e.g. if not isinstance(link_id, int): continue) before assigning into link_map, ensuring malformed top-level link IDs are skipped; update the function _build_link_map and its use of link_map to only insert when link_id passes the validation.
267-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden subgraph keying against malformed names and link IDs.
inp.get("name"),old_id,lid, and(origin_id, origin_slot)are all promoted to dict keys here. If a malformed subgraph definition supplies a list/dict for any of them, conversion still dies withTypeErrorbefore the node-level rescue path — a tiny hash-crash imp. Skip non-string names and non-int / unhashable link identifiers in these helpers.Suggested guard rails
def _outer_slot_to_input_idx(outer_node: dict, sg_def: dict) -> dict[int, int]: """Map the outer node's input slots to subgraph-definition input indices.""" - sg_input_names: dict[Any, int] = {} + sg_input_names: dict[str, int] = {} for idx, inp in enumerate(sg_def.get("inputs") or []): - if isinstance(inp, dict): - sg_input_names[inp.get("name")] = idx + if not isinstance(inp, dict): + continue + name = inp.get("name") + if isinstance(name, str): + sg_input_names[name] = idx mapping: dict[int, int] = {} for outer_idx, outer_input in enumerate(outer_node.get("inputs") or []): if not isinstance(outer_input, dict): continue name = outer_input.get("name") - if name in sg_input_names: + if isinstance(name, str) and name in sg_input_names: mapping[outer_idx] = sg_input_names[name] return mapping @@ for link in internal_links: if isinstance(link, dict): old_id = link.get("id") - if old_id is not None: - link_id_remap[old_id] = next_id - next_id += 1 - internal_link_map[old_id] = link + if not isinstance(old_id, int): + continue + link_id_remap[old_id] = next_id + next_id += 1 + internal_link_map[old_id] = link @@ targets = [] for lid in in_def.get("linkIds") or []: + if not isinstance(lid, int): + continue link = internal_link_map.get(lid) if isinstance(link, dict): targets.append((link.get("target_id"), link.get("target_slot"))) @@ for lid in out_def.get("linkIds") or []: + if not isinstance(lid, int): + continue link = internal_link_map.get(lid) if isinstance(link, dict): - output_sources[(link.get("origin_id"), link.get("origin_slot"))] = idx + try: + output_sources[(link.get("origin_id"), link.get("origin_slot"))] = idx + except TypeError: + continueAlso applies to: 300-327
🤖 Prompt for 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. In `@comfy_cli/workflow_to_api.py` around lines 267 - 277, The code currently keys sg_input_names and mapping using values like inp.get("name"), old_id, lid and tuple (origin_id, origin_slot) which can be non-hashable/malformed and raise TypeError; update the loops that build sg_input_names and mapping (the blocks that enumerate sg_def.get("inputs") and outer_node.get("inputs")) to first validate types: only accept string names for inp.get("name") and outer_input.get("name") and only accept integer or otherwise hashable scalar IDs for link identifiers (old_id, lid, origin_id/origin_slot) before using them as dict keys or in tuples, skipping entries that fail validation so the higher-level node-level rescue can handle malformed subgraph defs. Ensure the same guards are applied to the analogous construction later in the file (the similar loop around the other mapping between link ids).
🤖 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/workflow_to_api.py`:
- Around line 954-973: _filter_control_values currently drops control tokens
solely by string match against _CONTROL_AFTER_GENERATE_VALUES, which removes
legitimate STRING/COMBO values; change it to be schema-aware by accepting the
corresponding widget/input specs (or types) alongside widget_values and only
treat a following control token as removable when the associated input spec for
the preceding value is a seed-like INT (or otherwise marked to allow
control-after-generate). Update the function signature (e.g., add widget_specs
or widget_types param), use the spec for the current index to decide whether a
next-string in _CONTROL_AFTER_GENERATE_VALUES should be skipped, and preserve
any tokens that belong to non-seed schemas (STRING/COMBO). Ensure references to
_filter_control_values, widget_values, and _CONTROL_AFTER_GENERATE_VALUES are
used to locate and implement the change.
---
Duplicate comments:
In `@comfy_cli/workflow_to_api.py`:
- Around line 545-556: The code currently assigns var_name = widgets[0] and uses
it as a dict key for set_sources and get_vars without validating its type; guard
against malformed payloads by validating widgets[0] is a supported scalar (e.g.
isinstance(var_name, str) or int and not a list/dict/None) before using it, and
skip/continue when it isn't; update the branches that reference var_name (the
SetNode handling that writes set_sources and the GetNode handling that writes
get_vars) to perform this check and only store valid scalar keys.
- Around line 487-499: The current _build_link_map function assumes link_id is a
valid int key and will raise if a link's first element is malformed (e.g. a
dict); to fix it, after unpacking link_id, validate it (e.g. if not
isinstance(link_id, int): continue) before assigning into link_map, ensuring
malformed top-level link IDs are skipped; update the function _build_link_map
and its use of link_map to only insert when link_id passes the validation.
- Around line 267-277: The code currently keys sg_input_names and mapping using
values like inp.get("name"), old_id, lid and tuple (origin_id, origin_slot)
which can be non-hashable/malformed and raise TypeError; update the loops that
build sg_input_names and mapping (the blocks that enumerate sg_def.get("inputs")
and outer_node.get("inputs")) to first validate types: only accept string names
for inp.get("name") and outer_input.get("name") and only accept integer or
otherwise hashable scalar IDs for link identifiers (old_id, lid,
origin_id/origin_slot) before using them as dict keys or in tuples, skipping
entries that fail validation so the higher-level node-level rescue can handle
malformed subgraph defs. Ensure the same guards are applied to the analogous
construction later in the file (the similar loop around the other mapping
between link ids).
🪄 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: 159c073c-c151-466e-aff8-7686270a7135
📒 Files selected for processing (2)
comfy_cli/workflow_to_api.pytests/comfy_cli/test_workflow_to_api.py
_collect_get_set_mappings runs before the per-node try/except wrapper in convert_ui_to_api. If a SetNode's widgets_values[0] is a list or dict, ``set_sources[var_name] = ...`` raises TypeError and aborts the whole conversion. A GetNode with an unhashable widgets_values[0] also crashes the tracer later (``var_name in self.set_sources``) — caught by the per-node wrapper but still skips otherwise-valid downstream nodes. Reject any var_name that isn't a non-empty string up front. One new test covers list / dict / None / empty-string cases. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
The naive filter dropped any ``"fixed"/"increment"/"decrement"/"randomize"`` occurrence in widgets_values, even when it was a legitimate COMBO/STRING value rather than the synthetic marker emitted after a seed-like INT. Every later widget value then slid out of alignment, silently corrupting the prompt. (SethRobinson's reference converter has the same bug.) When a schema is available the filter now walks input_order in widget order and only consumes a control marker if it directly follows an input whose spec sets ``control_after_generate: True``. Without a schema, falls back to the legacy positional string-match heuristic so unknown node types behave the same as before. Verified with a fictional node whose COMBO option is literally "fixed" — the value is now preserved instead of being stripped. KSampler-style seed widgets still strip their control marker. Parity sweep against the reference converter unchanged: 46/51 byte-identical + 5 cosmetic title-only diffs. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Following a systematic comparison against the frontend's graphToPrompt and the litegraph helpers it uses, three gaps were closed: 1. forceInput / defaultInput: ``forceInput: True`` (and its deprecated alias ``defaultInput``) demotes a widget-type input to a connection-only slot. The frontend doesn't render a widget for it and saved workflows have no entry in widgets_values for it. We were treating it as a widget, consuming a value slot that didn't exist and shifting every later widget value into the wrong input. Updated ``_is_widget_input`` to recognize both keys. 2. Muted/bypassed subgraph instances: if the outer subgraph instance node is muted (mode 2) or bypassed (mode 4) the frontend skips ``getInnerNodes()`` entirely — the inner subgraph never enters the prompt. We were unconditionally expanding regardless of mode, silently keeping nodes the user explicitly told to skip. The instance now stays in the node list so the regular mode-check excludes it from emission; bypass-with-external-wires routes through ``trace_bypassed`` the same as a regular bypassed node. 3. (Already done in the previous commit.) control_after_generate filtering is schema-aware. 3 new test classes cover the new behaviors. Parity sweep against the reference converter unchanged at 46/51 byte-identical + 5 cosmetic title-only diffs. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
CLIPTextEncode-family text inputs (32 in stock ComfyUI) declare
``dynamicPrompts: True`` in their schema. The frontend's
``processDynamicPrompt`` resolves ``{red|blue} hat`` to one of the
alternatives at /prompt time. The backend treats the syntax literally,
so a workflow saved with ``{a|b}`` syntax and executed via comfy-cli
would tokenize the braces and pipes as literal characters and produce
junk output. Neither SethRobinson's reference converter nor the
backend handles this.
Ported faithfully from packages/shared-frontend-utils/src/formatUtil.ts:
* Strips ``/* ... */`` and ``// ...`` C-style comments
* Picks one alternative at random from each top-level ``{a|b|...}``
group; recurses into the picked option (so nested groups resolve)
* ``\\{``, ``\\}``, ``\\|`` escape their literal characters
Applied during widget collection when the schema declares
``dynamicPrompts: True`` and the value is a string. The resolution is
genuinely random — matching the frontend's ``Math.random()`` — so two
consecutive runs of a workflow with ``{a|b}`` may produce different
prompts. Tests pin ``random.choice`` deterministically.
13 new tests cover: comments, single/nested/multiple groups, empty
alternatives, escapes, unterminated groups, end-to-end via
convert_ui_to_api with and without the dynamicPrompts flag, and
non-string passthrough. Parity sweep against the reference converter
unchanged at 46/51 byte-identical + 5 cosmetic title-only diffs.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/workflow_to_api.py`:
- Around line 862-867: When building api_node, resolve the node's effective
schema once and reuse it everywhere instead of re-accessing
object_info[node_type]; determine the effective key the same way
_collect_widget_inputs() and _get_ordered_input_names() do (i.e. honor
properties["Node name for S&R"] / alias) and store it in a local variable (e.g.,
resolved_schema or effective_schema) and use that for _meta title, defaults and
combo normalization; update the helper signatures (_collect_widget_inputs,
_get_ordered_input_names and any callers around the api_node construction) to
accept or return the resolved schema or effective node key so all pieces
consistently use the same schema (also apply the same change to the similar
block around lines 904-927).
- Around line 1169-1170: The bug is that _get_widget_name_order is still called
with the original widget_values after you call _filter_control_values, so
dynamic-combo name resolution can read the wrong slot; fix it by passing the
filtered list into _get_widget_name_order (i.e. replace the second call site to
use the variable returned by _filter_control_values such as filtered) while
keeping the other parameters (node_type, node, object_info) the same so name
ordering uses the post-filtered widget list.
- Around line 349-350: expanded["inputs"] is built by calling
_rewrite_internal_input on every element of inner.get("inputs", []) which
crashes if inputs contains non-dict values because _rewrite_internal_input
assumes a dict; filter inner inputs to only dicts before rewriting (e.g.,
replace the comprehension in the expanded["inputs"] assignment with one that
includes only items where isinstance(inp, dict)) so malformed entries like None
or integers are skipped and per-node try/except can handle remaining issues;
reference _rewrite_internal_input, expanded["inputs"], and inner.get("inputs",
[]) when making the change.
🪄 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: a725ef51-6fb9-427e-ac86-53e610105eab
📒 Files selected for processing (2)
comfy_cli/workflow_to_api.pytests/comfy_cli/test_workflow_to_api.py
_get_widget_name_order's internal widget_idx is the position in the value list it receives. We were handing it the raw widgets_values even though the mapping step below iterates the filtered list (with control_after_generate markers stripped). For nodes that pair a control_after_generate seed widget with a V3 dynamic combo, the combo's _dynamic_combo_sub_inputs lookup landed on the control marker instead of the actual selector, the selected option was never identified, and every sub-input value below it was silently dropped from the prompt. 38 stock API nodes are vulnerable (Bria*, ByteDance*, Grok*, Kling*, Meshy*, Recraft*, Reve*, Vidu*, Wan2*, HappyHorse*, Tencent*, Quiver*). Reproduced with a minimal schema where shape.side dropped to None after seed+control_marker, and confirmed fixed by passing filtered. One new test pins the behavior. Parity sweep unchanged at 46/51 byte-identical + 5 cosmetic title-only diffs. 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/workflow_to_api.py`:
- Around line 316-322: The loop over internal_links uses link.get("id") as a
dict key without guarding its type, causing collisions for None or TypeError for
unhashable ids; fix by checking the id is an int (e.g., isinstance(old_id, int))
before mutating link_id_remap and internal_link_map so both assignments happen
only when old_id is a valid hashable integer; update the block around
internal_links, leaving references to link_id_remap, internal_link_map,
_expand_one_subgraph, and convert_ui_to_api unchanged otherwise.
🪄 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: ae38fa0a-c26d-4b35-923b-fd3d4640ed4e
📒 Files selected for processing (2)
comfy_cli/workflow_to_api.pytests/comfy_cli/test_workflow_to_api.py
_expand_one_subgraph used link.get("id") as a dict key in three places
without isinstance checks:
1. The initial pass that builds link_id_remap and internal_link_map.
``internal_link_map[old_id] = link`` was dedented out of the
``old_id is not None`` guard, so two links missing "id" collided
under the None key (silently dropping the later one), and an
unhashable id (list/dict) raised TypeError mid-loop.
2. The link-emission pass: ``new_id = link_id_remap.get(old_id, ...)``
crashes on unhashable old_id.
3. _rewrite_internal_input: ``internal_link_map.get(link_id)`` and
``link_id in link_id_remap`` crash on unhashable link_id from an
inner node's input definition.
All three run before the per-node try/except wrapper in
convert_ui_to_api, so any TypeError there aborted the whole
conversion despite our broader hardening contract.
Restrict to ``isinstance(old_id, int)`` / ``isinstance(link_id, int)``
at each site — standard ComfyUI link IDs are ints. Two new tests
cover unhashable subgraph link IDs and unhashable inner-node link
references. Parity sweep unchanged at 46/51 byte-identical + 5
cosmetic title-only diffs.
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/workflow_to_api.py`:
- Around line 555-556: The guard that checks link membership (using link_id =
inputs[0].get("link") and conditions like "link_id not in link_map") can raise
TypeError when link is an unhashable or unexpected type (e.g., [] or {}); update
the checks to first validate the shape/type of the link value before membership
tests — e.g., ensure link_id is a hashable scalar (string/int) or otherwise a
valid key type, and treat any other types as missing/invalid so the node is
skipped; apply the same hardening to the other pre-pass site that references lid
and link_map (the block around lines 581-586) so malformed link values no longer
abort the entire conversion.
🪄 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: fdfd5947-643b-40c2-ba95-5f381463c67f
📒 Files selected for processing (2)
comfy_cli/workflow_to_api.pytests/comfy_cli/test_workflow_to_api.py
CodeRabbit pointed at _collect_reroute_sources and the SetNode branch
in _collect_get_set_mappings — both run before the per-node try/except
wrapper, both do ``link_id in link_map`` / ``link_map.get(link_id)``,
and both crash with TypeError on a saved ``link: []`` or ``link: {}``,
aborting the entire conversion.
Sweeping audit found four more dict-key uses with the same shape:
* _expand_one_subgraph subgraph input ``linkIds`` loop
* _expand_one_subgraph subgraph output ``linkIds`` loop
* _build_api_node's per-input ``tracers.link_map`` lookup (this one
was already inside the per-node try/except, so it only ever
truncated one node — but hardening it keeps the contract uniform)
All five now isinstance-check link_id / lid as int before the
membership test, matching the surrounding pattern. One new test
covers Reroute, SetNode, and subgraph linkIds with unhashable values
in a single sweep.
Parity sweep against the reference converter unchanged at 46/51
byte-identical + 5 cosmetic title-only diffs.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
_collect_widget_inputs (and the helpers it composes) consulted the schema via _schema_for, which honors the ``Node name for S&R`` property alias the same way the frontend does. But _meta.title, _collect_default_inputs, _normalize_combo_values, and the dead-branch heuristic in _collect_excluded looked up object_info[node_type] directly. For a node carrying an S&R alias to a different class: * _meta.title got the raw type name instead of the aliased display_name * defaults weren't filled in (schema lookup missed) * combo values weren't normalized (e.g., "EULER" stayed as-is and the server validator rejected it) * a schemaless dead-branch node could be wrongly excluded Resolve schema once at the top of _build_api_node and thread it through. _collect_default_inputs and _normalize_combo_values now take the resolved schema directly. _collect_excluded uses _schema_for inline. The S&R alias is rare in modern workflows (724/724 nodes in the stock ComfyUI blueprints have S&R == type), so parity sweep against SethRobinson is unchanged at 46/51 byte-identical + 5 cosmetic title-only diffs. A 4-case test class pins the aliased behavior on each of the affected paths. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Both fixes come from a self-review pass. Neither has a known
real-world trigger in stock ComfyUI (verified by scanning every V3
combo option and schema in /object_info), but both follow the
same defensive contract as the rest of the converter.
1. _dynamic_combo_sub_inputs no longer crashes when a V3 combo
option's ``inputs`` field is a non-dict (string, list, int, ...).
Before, ``sub_def.get(section)`` raised AttributeError; the
per-node try/except caught it but silently dropped the entire
node. Now we degrade to "no sub-inputs" and keep the node.
2. Extract _schema_input_def(schema) and use it in every helper
that previously did ``schema.get("input") or {}`` followed by
``.get(section)``. The raw ``or {}`` pattern returns the value
as-is when it's truthy, so a malformed schema with
``"input": [1,2,3]`` would AttributeError on ``.get``. Same
reasoning applied to ``schema.get("input_order")``.
3 new tests cover the V3 combo malformed-inputs case and
schema.input / schema.input_order with list / string / int.
Parity sweep against the reference converter unchanged at 46/51
byte-identical + 5 cosmetic title-only diffs.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
trace_reroute, trace_get_set, and trace_bypassed were tail-recursive through the chain of nodes they walk. Python's default recursion limit (1000) meant chains of ~997 hops triggered RecursionError; the per-node try/except wrapper then caught it and silently dropped the downstream consumer from the prompt — a soft failure mode that produces a wrong-but-runnable workflow with no user-visible signal. The reference converter has the same bug (verified — chains of 999 reroutes return HTTP 500), but its endpoint surfaces the error explicitly. Our silent drop is materially worse for debugging. All three tracers are tail-recursive: each recursive call is the last expression of the function, with only an updated (src_id, src_slot) pair. They convert mechanically into ``while True`` loops that mutate src_id/src_slot in place. The cycle-detection seen-set remains scoped to each top-level call. trace_bypassed's internal calls to trace_get_set / trace_reroute already iterate over their own chains (no recursion compounding). The _seen optional parameter is dropped from all three since the only external callers used the 2-arg form. 3 new regression tests pin chain lengths well past the old crash threshold (N=2000) for each tracer. Cycle handling is unchanged (verified on Reroute<->Reroute and Set/Get cycles). Parity sweep against the reference converter unchanged at 46/51 byte-identical + 5 cosmetic title-only diffs. Live SD 1.5 smoke test succeeds. Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Cross-checked against comfy-cloud-mcp-server's converter (TypeScript
port of graphToPrompt with 455 oracle fixtures from real cloud
workflows). Two real divergences identified and fixed:
1. ``*`` / empty-string wildcard input type:
The frontend doesn't render a widget for these — they're
connection-only slots (canonical case: ``PreviewAny.source: ["*", {}]``).
Our ``_is_widget_input`` was treating them as widgets because the
lowercase-fallback condition ``not input_type.isupper()`` returns
``True`` for strings with no cased characters. The bug consumed a
widgets_values slot for the wildcard and shifted every later widget
into the wrong input.
2. Implicit ``control_after_generate`` for ``seed`` / ``noise_seed``:
The frontend's ``useIntWidget`` composable adds the companion widget
for inputs literally named ``seed`` or ``noise_seed`` even when the
schema doesn't declare ``control_after_generate: True``. Our filter
only checked the schema flag, so explicit seeds worked but every
implicit (workflow-saved-with-companion) case slid by one.
New ``_has_control_after_generate_companion`` helper uses peek-based
detection: only consumes the next value if it's actually one of
``fixed`` / ``increment`` / ``decrement`` / ``randomize``, so older
workflows saved before the companion existed still convert correctly.
Cloud-mcp oracle parity: 248 → 251 perfect matches (+3 fixtures).
Stock ComfyUI blueprint parity unchanged at 46/51 + 5 cosmetic.
6 new tests cover both behaviors, including the edge cases where
peek-detection prevents over-eager stripping.
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Two coupled changes to match graphToPrompt()'s emission policy
exactly:
1. Drop the dead-branch exclusion heuristic in _collect_excluded.
The old rule dropped any node whose outputs weren't connected
downstream (unless the schema marked it as output_node or it had
no schema + connected inputs). That excluded legitimate sources
like an unwired LoadAudio that the frontend's graphToPrompt()
still emits — costing 20+ real cloud workflows their LoadAudio /
VAEDecode / etc. nodes when round-tripped through our converter.
The frontend (executionUtil.ts) and cloud-mcp-server's
shouldIncludeInOutput emit every non-virtual, non-muted,
non-bypassed node regardless of downstream wiring. The executor
only runs nodes reachable from sinks (SaveImage etc.) — leftover
unwired nodes are harmless in the prompt JSON.
2. Add MarkdownNote to _UI_ONLY_NODE_TYPES.
Once dead-branch exclusion is gone, MarkdownNote nodes (purely
frontend documentation, no Python class) would start being
emitted. Cloud-mcp-server lists it explicitly in
VIRTUAL_NODE_TYPES; we match.
Measured against cloud-mcp-server's 455-fixture oracle:
before: 251 exact + 18 functional, 434/455 would-execute (95.4%)
after: 255 exact + 20 functional, 454/455 would-execute (99.8%)
^^^^^^^^^ matches cloud-mcp's
own MIN_WOULD_EXECUTE
gate
Stock ComfyUI blueprint parity (vs SethRobinson reference):
before: 46 perfect + 5 cosmetic title-only + 0 real diff
after: 43 perfect + 5 cosmetic title-only + 3 real diff
The 3 stock regressions emit a VAEDecode whose output isn't
wired downstream. They still execute correctly — the decode
runs and the result is discarded. SethRobinson's converter
dropped these via the same heuristic we're removing; the
frontend keeps them.
3 tests updated/replaced (test_dead_branch_excluded →
test_unwired_node_still_emitted, similar for the S&R-alias
dead-branch case), 2 new tests added (markdown_note_excluded,
unwired_load_node_still_emitted).
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.
Summary
Part 2 of #446. Replaces the server-endpoint-based conversion landed in #448 with a fully client-side converter, so
comfy run --workflowno longer needs SethRobinson'scomfyui-workflow-to-api-converter-endpointcustom node installed on the target server.comfy_cli/workflow_to_api.pydoes the conversion in pure Python given a UI workflow + the running server's/object_inforesponse.comfy runnow GETs/object_info(always available on stock ComfyUI) and converts locally.convert_ui_workflow_via_server,WorkflowConverterUnavailable, help-text fallback) is removed.Approach
The conversion is a faithful Python port of SethRobinson's
/workflow/convertalgorithm (Unlicense), restructured to consume/object_infoschema dicts in place of importing ComfyUI's in-processnodesmodule. It also incorporates behaviors from the frontend'sgraphToPrompt(executionUtil.ts) where the two diverged — strictisValidConnection-based bypass matching, list-widget-value wrapping ({__value__: [...]}), final orphan-link cleanup. The frontend was used as the reference of last resort because it defines what ComfyUI actually accepts.Conversion covers: subgraph expansion (nested, with link ID remapping), PrimitiveNode value inlining, Reroute / GetNode / SetNode pass-through, bypass mode 4 (with strict type matching + first-linked fallback for SethRobinson parity), mute mode 2 with orphan cleanup, LoadImageOutput exclusion, OUTPUT_NODE preservation, combo case normalization, schema-driven defaults,
control_after_generatefiltering, V3 dynamic combo sub-inputs, rgthree-style lora dict widgets, list widget value wrapping. Detects legacy 'group nodes' (workflow>prefix) and warns rather than silently mishandling them.Robustness
Marked as experimental in the user-facing crash message. Hardened against malformed input — non-dict node/input/output entries, non-numeric link slots, garbage
definitions, non-listwidgets_valuesetc. are skipped with a log warning rather than crashing. Per-node emission is wrapped so a bug in handling one node cannot torpedo the whole conversion. The CLI catches unexpected converter exceptions and surfaces a friendly "this is experimental, please report" message instead of a Python traceback.Fuzz-tested with 18 malformed-input cases: before hardening, 8 crashed with Python tracebacks; after, all 18 exit cleanly with a typed
WorkflowConversionErroror a partial conversion.Verification
/object_info, asserting byte-identical output to the reference/workflow/convertresponse._meta.titlefor V3 nodes whose/object_info.display_nameisNone(the executor ignores_meta.title).[muted_node_id, 0]reference that the executor would reject; we (and the frontend's final cleanup pass) strip it./workflow/convertreturns 405): SD 1.5 and SDXL Turbo UI workflows execute successfully and produce images. Subgraph workflow (Z-Image-Turbo) converts and queues correctly.Known limitations
workflow>prefix,extra.groupNodes) are detected and warned about but not expanded. None of the 51 sample workflows use them; ComfyUI's frontend has deprecated them in favor of subgraphs._meta.titlemay show the class name instead of the display name for V3 nodes whose/object_info.display_nameisNone. This is a ComfyUI server-side reporting quirk, not something we can fix from the client. The executor ignores_meta.title.Test plan
comfy run --workflow <UI-format file>against a stock ComfyUI install (no SethRobinson node)