From 4e961bec7f70594412e64a40ed15eafcb6d832a1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 09:32:09 -0700 Subject: [PATCH 1/4] feat(workflow): surface subgraph-interior nodes and their mode in ls-nodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `workflow ls-nodes` iterated only `workflow["nodes"]` — the top level — where a subgraph instance is a single opaque node. The nodes it actually executes live under `definitions.subgraphs[].nodes`, and `workflow_to_api` expands them and then drops any that are muted/bypassed, so the graph ran without a node and no reader of ls-nodes could tell. Emit them under a NEW sibling key `data.subgraph_nodes[]` rather than appending to `data.nodes[]`: consumers render every `nodes[]` entry verbatim and pin that listing, and `count` keeps its top-level-only meaning. Each interior row carries path/instance/id/type/title plus `mode` only when the node is disabled, matching the top-level label-only-when-set convention. Paths use the same `/` addressing as `workflow slots`, so they compose directly into a slot address. The walk respects cql.engine's `_MAX_SUBGRAPH_DEPTH` and additionally refuses to re-enter a definition already open on the current path, so a corrupt document whose definition reaches itself while branching cannot cost branches**32 rows. --- comfy_cli/command/workflow_edit.py | 99 ++++++++++- comfy_cli/schemas/workflow.json | 43 +++++ comfy_cli/skills/comfy/SKILL.md | 9 + tests/comfy_cli/command/test_ls_nodes_mode.py | 164 ++++++++++++++++++ 4 files changed, 314 insertions(+), 1 deletion(-) diff --git a/comfy_cli/command/workflow_edit.py b/comfy_cli/command/workflow_edit.py index 39e039740..54978948b 100644 --- a/comfy_cli/command/workflow_edit.py +++ b/comfy_cli/command/workflow_edit.py @@ -550,6 +550,84 @@ def reset_doc_cmd( # --------------------------------------------------------------------------- +def _subgraph_interior_rows(workflow: dict) -> list[dict]: + """Rows for the nodes INSIDE every live subgraph instance, flattened. + + ``workflow["nodes"]`` only carries the top level: a subgraph instance is a + single opaque node whose ``type`` is its definition's UUID, and the nodes it + actually executes live under ``definitions.subgraphs[].nodes``. Those + interiors are the ones ``workflow_to_api`` expands and then drops when they + are muted/bypassed — so without this a reader sees a graph that silently + runs without a node, with nothing in ``ls-nodes`` to explain it. + + Addresses match ``comfy workflow slots`` exactly (``/``, + nesting with ``/``), so a path from here is directly usable as a slot + address prefix. Emitted under the sibling ``subgraph_nodes`` key, never + appended to ``nodes`` — consumers render ``nodes[]`` verbatim and pin it. + """ + from comfy_cli.cql.engine import _MAX_SUBGRAPH_DEPTH, _SUBGRAPH_PATH_SEP, _subgraph_defs_by_id + + defs_by_id = _subgraph_defs_by_id(workflow) + if not defs_by_id: + return [] + interior: list[dict] = [] + + def _def_for(node: dict) -> dict | None: + """The subgraph definition a node instantiates, or None for a plain node. + + Every key in ``defs_by_id`` is a non-empty ``str``, so a node whose + ``type`` is missing or non-string can never be an instance — and looking + one up unguarded would either coerce ``None`` into the matchable string + ``"None"`` or raise ``TypeError`` on an unhashable (list/dict) ``type``. + """ + node_type = node.get("type") + return defs_by_id.get(node_type) if isinstance(node_type, str) else None + + def walk(nodes: Any, prefix: str, instance: str, depth: int, seen_defs: frozenset[int]) -> None: + # Same depth cap the slot walker uses (cql.engine), for the same reason: + # a hand-written or corrupt document can nest subgraphs pathologically. + if depth > _MAX_SUBGRAPH_DEPTH: + return + for n in nodes or []: + if not isinstance(n, dict): + continue + node_id = n.get("id") + # Stringified exactly as the slot walker stringifies it, so the two + # commands' addresses stay byte-identical; ``id`` stays verbatim. + path = f"{prefix}{_SUBGRAPH_PATH_SEP}{str(n.get('id', ''))}" + row = { + "path": path, + "instance": instance, + "id": node_id, + "type": n.get("type"), + "title": n.get("title") or (n.get("properties") or {}).get("Node name for S&R"), + } + # Label-only-when-set, exactly as the top-level rows do. + if (label := _MODE_LABELS.get(n.get("mode"))) is not None: + row["mode"] = label + interior.append(row) + sg = _def_for(n) + # The depth cap alone bounds a *linear* cycle at 32 rows, but a + # definition that reaches itself while ALSO branching would cost + # branches**32 rows before that cap fires. A definition already open + # on this path can only be a cycle (ComfyUI cannot author one), so + # stop there; a def reused on sibling paths is unaffected. + if sg is not None and id(sg) not in seen_defs: + walk(sg.get("nodes"), path, instance, depth + 1, seen_defs | {id(sg)}) + + for n in workflow.get("nodes") or []: + if not isinstance(n, dict): + continue + sg = _def_for(n) + if sg is None: + continue + instance = str(n.get("id", "")) + # Recurse whether or not the instance itself is muted: the consumer keys + # on the root row's own mode, which `nodes[]` already carries. + walk(sg.get("nodes"), instance, instance, 1, frozenset({id(sg)})) + return interior + + @tracking.track_command("workflow") def ls_nodes_cmd( file: Annotated[str, typer.Argument(help="Frontend-format workflow JSON.")], @@ -575,7 +653,17 @@ def ls_nodes_cmd( if (label := _MODE_LABELS.get(n.get("mode"))) is not None: row["mode"] = label rows.append(row) - payload = {"workflow": str(p), "count": len(rows), "nodes": rows} + interior = _subgraph_interior_rows(workflow) + # `count`/`nodes` keep their exact pre-existing meaning (top level only) — + # interiors go under a NEW sibling key, because consumers render every + # `nodes[]` entry verbatim and pin that listing. + payload = { + "workflow": str(p), + "count": len(rows), + "nodes": rows, + "subgraph_nodes": interior, + "subgraph_count": len(interior), + } if renderer.is_pretty(): from rich.table import Table @@ -586,6 +674,15 @@ def ls_nodes_cmd( for r in rows: tbl.add_row(str(r["id"]), str(r["type"]), str(r["title"] or "")) renderer.console().print(tbl) + if interior: + sub = Table(show_header=True, header_style="bold", title="subgraph interiors") + sub.add_column("path", no_wrap=True) + sub.add_column("type") + sub.add_column("title", style="dim") + sub.add_column("mode", style="dim") + for r in interior: + sub.add_row(r["path"], str(r["type"]), str(r["title"] or ""), r.get("mode", "")) + renderer.console().print(sub) renderer.emit(payload, command="workflow ls-nodes") diff --git a/comfy_cli/schemas/workflow.json b/comfy_cli/schemas/workflow.json index 45667d510..ad2387ef7 100644 --- a/comfy_cli/schemas/workflow.json +++ b/comfy_cli/schemas/workflow.json @@ -7,6 +7,49 @@ "workflow": { "type": "string" }, "count": { "type": "integer" }, "slots": { "type": "array" }, + "nodes": { + "type": "array", + "description": "ls-nodes: one row per TOP-LEVEL node — a subgraph instance is a single opaque row here. `count` is the length of this array.", + "items": { + "type": "object", + "properties": { + "id": { "description": "node id as serialized in the workflow" }, + "type": { "type": ["string", "null"] }, + "title": { "type": ["string", "null"] }, + "mode": { + "type": "string", + "enum": ["mute", "bypass"], + "description": "present ONLY when the node is disabled (litegraph mode 2/4); absent for a normally-executing node" + } + } + } + }, + "subgraph_nodes": { + "type": "array", + "description": "ls-nodes: one row per node INSIDE a live subgraph instance (definitions.subgraphs[].nodes, recursively) — these execute but never appear in `nodes`. Empty when the workflow has no subgraph instances.", + "items": { + "type": "object", + "properties": { + "path": { + "type": "string", + "description": "`/` address, nesting with `/` (e.g. \"10/3/7\") — the same addressing `comfy workflow slots` uses, so it composes into a slot address" + }, + "instance": { "type": "string", "description": "id of the TOP-LEVEL subgraph instance this row is reached through" }, + "id": { "description": "interior node id as serialized in the subgraph definition" }, + "type": { "type": ["string", "null"] }, + "title": { "type": ["string", "null"] }, + "mode": { + "type": "string", + "enum": ["mute", "bypass"], + "description": "present ONLY when the interior node is disabled; workflow_to_api drops such a node after expansion, so nothing else reveals it" + } + } + } + }, + "subgraph_count": { + "type": "integer", + "description": "ls-nodes: length of `subgraph_nodes` (kept separate from `count`, which stays top-level-only)." + }, "notes": { "type": "array", "description": "notes: the Note/MarkdownNote documentation nodes the workflow carries, in graph order (top-level nodes first, then each subgraph definition).", diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index 4517faa3d..f276bc49e 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -392,6 +392,15 @@ comfy --json workflow delete-node wf.json 7 $CAT # removes comfy --json workflow ls-nodes wf.json # id / type / title (no catalog needed) ``` +`ls-nodes` emits `data.nodes[]` for the TOP LEVEL only (a subgraph instance is one +opaque row), plus a sibling `data.subgraph_nodes[]` for the nodes INSIDE every live +subgraph instance — the ones that actually execute. Each interior row carries +`path` / `instance` / `id` / `type` / `title`, and `mode` (`mute` | `bypass`) only +when the node is disabled, same as the top-level rows. `path` uses the same +`/` addressing as `comfy workflow slots` (nesting with `/`, e.g. +`10/3/7`), so it composes directly into a slot address. `data.count` stays the +top-level count; `data.subgraph_count` counts the interior rows. + **Building more than one or two nodes? Use `apply` — one batch, one catalog load, and `as` aliases so you never capture a minted id by hand:** diff --git a/tests/comfy_cli/command/test_ls_nodes_mode.py b/tests/comfy_cli/command/test_ls_nodes_mode.py index f6997b61d..58f6e6693 100644 --- a/tests/comfy_cli/command/test_ls_nodes_mode.py +++ b/tests/comfy_cli/command/test_ls_nodes_mode.py @@ -73,3 +73,167 @@ def test_ls_nodes_unchanged_when_no_modes_set(patched_graph, tmp_path, capsys): path = _write(tmp_path, _base_workflow()) env = _run(["ls-nodes", str(path)], capsys) assert all("mode" not in r for r in env["data"]["nodes"]), env["data"]["nodes"] + + +# --------------------------------------------------------------------------- +# subgraph interiors — `data.subgraph_nodes[]` +# +# `workflow["nodes"]` is the TOP LEVEL only: a subgraph instance is one opaque +# node whose `type` is its definition UUID, and the nodes it actually executes +# live under `definitions.subgraphs[].nodes`. `workflow_to_api` expands those +# interiors and then DROPS a muted/bypassed one — so the graph runs without the +# node and, before this, no reader of `ls-nodes` could tell. +# +# They are emitted under a NEW sibling key, never appended to `nodes[]`: the +# cloud agent renders every `nodes[]` entry as a model-visible line and pins +# that listing. +# --------------------------------------------------------------------------- + +SG_UUID = "8f1e0a2c-0000-4000-8000-000000000001" +SG_UUID_2 = "8f1e0a2c-0000-4000-8000-000000000002" + + +def _sg_workflow(instance_ids=(10,), interior_mode=MODE_MUTED) -> dict: + """Top-level EmptyLatentImage 7 plus one instance per id, all pointing at a + single definition whose interior node 9 carries ``interior_mode``.""" + wf = { + "last_node_id": 60, + "last_link_id": 0, + "nodes": [ + {"id": 7, "type": "EmptyLatentImage", "pos": [0, 0], "widgets_values": [512, 512, 1]}, + ], + "links": [], + "definitions": { + "subgraphs": [ + { + "id": SG_UUID, + "name": "Text to Image", + "inputs": [], + "nodes": [ + {"id": 9, "type": "CLIPTextEncode", "mode": interior_mode, "widgets_values": ["a cat"]}, + {"id": 11, "type": "VAEDecode", "mode": 0}, + ], + "links": [], + } + ] + }, + } + for nid in instance_ids: + wf["nodes"].append({"id": nid, "type": SG_UUID, "pos": [100, 0]}) + return wf + + +def _env(tmp_path, wf: dict, capsys) -> dict: + path = _write(tmp_path, wf) + env = _run(["ls-nodes", str(path)], capsys) + assert env["ok"] is True, env + return env + + +def test_interior_node_mode_is_reported(patched_graph, tmp_path, capsys): + """(i) a live instance 10 whose definition has interior node 9 with mode 2.""" + env = _env(tmp_path, _sg_workflow(), capsys) + by_path = {r["path"]: r for r in env["data"]["subgraph_nodes"]} + assert by_path["10/9"]["instance"] == "10" + assert by_path["10/9"]["id"] == 9 + assert by_path["10/9"]["mode"] == "mute" + assert by_path["10/9"]["type"] == "CLIPTextEncode" + # a normally-executing interior stays label-free, same as the top-level rows + assert "mode" not in by_path["10/11"], by_path["10/11"] + + +def test_top_level_nodes_unchanged_by_interiors(patched_graph, tmp_path, capsys): + """`nodes[]` keeps its exact meaning — the instance stays ONE opaque row and + no interior is appended. The cloud agent pins this listing.""" + env = _env(tmp_path, _sg_workflow(), capsys) + assert [r["id"] for r in env["data"]["nodes"]] == [7, 10] + assert env["data"]["count"] == 2 + assert env["data"]["subgraph_count"] == len(env["data"]["subgraph_nodes"]) == 2 + + +def test_nested_instance_two_levels_deep(patched_graph, tmp_path, capsys): + """(ii) 10/3/7, two levels deep, bypassed.""" + wf = _sg_workflow() + wf["definitions"]["subgraphs"][0]["nodes"].append({"id": 3, "type": SG_UUID_2}) + wf["definitions"]["subgraphs"].append( + { + "id": SG_UUID_2, + "name": "Inner", + "inputs": [], + "nodes": [{"id": 7, "type": "KSampler", "mode": MODE_BYPASS}], + "links": [], + } + ) + env = _env(tmp_path, wf, capsys) + by_path = {r["path"]: r for r in env["data"]["subgraph_nodes"]} + assert by_path["10/3/7"]["mode"] == "bypass" + assert by_path["10/3/7"]["instance"] == "10", "instance stays the TOP-LEVEL id" + assert by_path["10/3/7"]["id"] == 7 + # the nested instance itself is a row too, so a reader can see the chain + assert by_path["10/3"]["type"] == SG_UUID_2 + + +def test_two_instances_of_same_definition_both_emit(patched_graph, tmp_path, capsys): + """(iii) 10 and 11 share one definition — both must appear, addressed apart.""" + env = _env(tmp_path, _sg_workflow(instance_ids=(10, 11)), capsys) + by_path = {r["path"]: r for r in env["data"]["subgraph_nodes"]} + assert by_path["10/9"]["mode"] == by_path["11/9"]["mode"] == "mute" + assert by_path["10/9"]["instance"] == "10" + assert by_path["11/9"]["instance"] == "11" + + +def test_no_definitions_emits_empty_list(patched_graph, tmp_path, capsys): + """(iv) the 99% workflow: the key is always present, and empty.""" + env = _env(tmp_path, _base_workflow(), capsys) + assert env["data"]["subgraph_nodes"] == [] + assert env["data"]["subgraph_count"] == 0 + assert env["data"]["count"] == 2 + + +def test_self_referencing_definition_terminates(patched_graph, tmp_path, capsys): + """(v) a definition that contains an instance of ITSELF must not recurse + forever. ComfyUI cannot author this; a hand-written or corrupt document can.""" + wf = _sg_workflow() + wf["definitions"]["subgraphs"][0]["nodes"].append({"id": 5, "type": SG_UUID}) + env = _env(tmp_path, wf, capsys) + paths = [r["path"] for r in env["data"]["subgraph_nodes"]] + assert "10/5" in paths + assert len(paths) == len(set(paths)), "addresses must stay unique" + assert len(paths) < 100, f"cycle was not bounded: {len(paths)} rows" + + +def test_depth_cap_stops_a_long_nesting_chain(patched_graph, tmp_path, capsys): + """A chain of distinct definitions deeper than `_MAX_SUBGRAPH_DEPTH` is + truncated at the cap rather than walked to the bottom.""" + from comfy_cli.cql.engine import _MAX_SUBGRAPH_DEPTH + + depth = _MAX_SUBGRAPH_DEPTH + 5 + uuids = [f"8f1e0a2c-0000-4000-8000-{i:012d}" for i in range(depth)] + subgraphs = [] + for i, u in enumerate(uuids): + inner = [{"id": 1, "type": "VAEDecode"}] + if i + 1 < depth: + inner.append({"id": 2, "type": uuids[i + 1]}) + subgraphs.append({"id": u, "name": f"L{i}", "inputs": [], "nodes": inner, "links": []}) + wf = { + "last_node_id": 60, + "last_link_id": 0, + "nodes": [{"id": 10, "type": uuids[0], "pos": [0, 0]}], + "links": [], + "definitions": {"subgraphs": subgraphs}, + } + env = _env(tmp_path, wf, capsys) + levels = {r["path"].count("/") for r in env["data"]["subgraph_nodes"]} + assert max(levels) == _MAX_SUBGRAPH_DEPTH, sorted(levels) + + +def test_malformed_definitions_do_not_crash(patched_graph, tmp_path, capsys): + """Non-dict entries in `nodes`/`subgraphs`, and an instance whose `type` + resolves to nothing, are skipped rather than raising.""" + wf = _sg_workflow() + wf["nodes"].append("not-a-node") + wf["nodes"].append({"id": 12, "type": ["unhashable"]}) + wf["definitions"]["subgraphs"].append("not-a-subgraph") + wf["definitions"]["subgraphs"][0]["nodes"].append(None) + env = _env(tmp_path, wf, capsys) + assert [r["path"] for r in env["data"]["subgraph_nodes"]] == ["10/9", "10/11"] From c0a8de919268c234628a2305701e0ac9097dc280 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 10:26:04 -0700 Subject: [PATCH 2/4] fix(workflow): bound and shape-check the ls-nodes subgraph walk (review of #845) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review findings, each reproduced against the branch before fixing. The walk was bounded against CYCLES and LINEAR nesting but not against BRANCHING: 32 distinct definitions each holding two instances of the next repeat no definition on any path and never exceed `_MAX_SUBGRAPH_DEPTH`, yet expand a few-KB file into ~2**32 rows (12 such levels already measured at 8189), all held in memory and then serialized. Add a total row budget and, when it fires, say so — `data.subgraph_truncated` marks the listing incomplete rather than letting a consumer read a short list as a whole graph. `ls-nodes` is the one workflow command with no catalog and no validation gate in front of it, so a corrupt file must still get an envelope. Four shapes raised uncaught exceptions: a definition's `nodes` as a truthy scalar, a truthy non-dict `properties` (a falsy `[]` never reached the bug), an unhashable `mode` — `_MODE_LABELS.get` hashes its argument — and `definitions`/`subgraphs` as scalars, which this new call site fed to cql.engine's `_subgraph_defs_by_id` unscreened. The mode and title guards are factored into `_mode_label`/ `_node_title` and reused by the top-level row builder, which carried the same two hazards. Pretty mode passed workflow-file text to `Table.add_row`, which reads a `str` as Rich MARKUP: an unbalanced `[/]` in a title crashed the render with MarkupError, `[link=...]` rendered a live OSC 8 hyperlink, and control bytes reached the terminal. Route both tables through `sanitize_markup`, matching the contract in tests/comfy_cli/command/test_pretty_print_sanitize.py; it coerces non-strings, so it replaces the bare `str()` calls at exact display parity. Interior paths now join like the slot walker — separator only after a non-empty prefix — so an instance with no `id` yields `9`, not the unresolvable `/9`. Schema: `nodes` no longer declares `"type": "array"`. workflow.json is shared by the whole `workflow` group, and `workflow compose` emits `nodes` as an integer node COUNT, so the new declaration made every compose payload schema-invalid (verified with jsonschema against both payloads). The `items` subschema still constrains the array form. `type`/`title` drop their `["string","null"]` constraint for the same honesty reason: both are copied verbatim from the file, so a corrupt document could make ls-nodes violate its own published contract. Docs no longer claim every interior row executes: the listing deliberately INCLUDES muted and bypassed nodes — that is the point, since workflow_to_api drops them silently — so deciding what runs means checking `mode` on the row and on every ancestor along its `path`. Not fixed here, deferred with rationale on the threads: cql.engine's `_subgraph_defs_by_id` registers a definition's cosmetic `name` as a fallback key, so a subgraph named after a real node class captures every node of that class. It predates this PR, affects `workflow slots` identically, and mirroring it is what keeps the two commands' addresses byte-identical. --- comfy_cli/command/workflow_edit.py | 120 +++++++++-- comfy_cli/schemas/workflow.json | 17 +- comfy_cli/skills/comfy/SKILL.md | 10 +- tests/comfy_cli/command/test_ls_nodes_mode.py | 187 ++++++++++++++++++ 4 files changed, 312 insertions(+), 22 deletions(-) diff --git a/comfy_cli/command/workflow_edit.py b/comfy_cli/command/workflow_edit.py index 54978948b..e462a0076 100644 --- a/comfy_cli/command/workflow_edit.py +++ b/comfy_cli/command/workflow_edit.py @@ -27,6 +27,7 @@ _parse_value, ) from comfy_cli.output import get_renderer, rprint +from comfy_cli.output.sanitize import sanitize_markup # Shared option aliases — the edit commands (add-node/set-widget/connect/ # delete-node/capture/apply/foreach) all take the same catalog + CRDT-stamping @@ -545,12 +546,49 @@ def reset_doc_cmd( # _MODE_MUTED / _MODE_BYPASS. _MODE_LABELS = {2: "mute", 4: "bypass"} +# Ceiling on the interior rows one `ls-nodes` will emit. The depth cap plus the +# per-path `seen_defs` set bound *cycles* and *linear* nesting, but neither +# bounds a BRANCHING acyclic definition graph: 32 distinct definitions that each +# hold two instances of the next never repeat a definition on any path and never +# exceed the depth cap, yet expand a few-KB file into ~2**32 rows (measured: 12 +# such levels already yield 8189). Every row is accumulated in memory, then +# serialized to JSON and rendered, so the depth cap alone is not a work bound. +# Real graphs are nowhere near this — the largest subgraph-heavy workflows run +# to low hundreds of interior nodes — so a document that reaches this ceiling is +# corrupt or hostile, and `subgraph_truncated` tells the consumer so. +_MAX_SUBGRAPH_INTERIOR_ROWS = 10_000 + + +def _mode_label(node: dict) -> str | None: + """The ``mute``/``bypass`` label for a node, or None when it executes. + + ``_MODE_LABELS.get`` HASHES its argument, so a node serialized with + ``"mode": []`` or ``{}`` would raise ``TypeError: unhashable type`` and abort + the command with a traceback instead of an error envelope. Only an ``int`` + can be a litegraph mode; ``bool`` is an int subclass but matches neither 2 + nor 4, so it falls through to None like any other non-mode value. + """ + mode = node.get("mode") + return _MODE_LABELS.get(mode) if isinstance(mode, int) else None + + +def _node_title(node: dict) -> Any: + """A node's display title, falling back to its S&R name. + + ``properties`` is a dict in every ComfyUI save, but ls-nodes reads an + ARBITRARY file: a truthy non-dict (``"properties": ["a"]``) survives an + ``or {}`` guard and then raises ``AttributeError`` on ``.get``. Returned + verbatim rather than coerced — see the schema note on `title`. + """ + props = node.get("properties") + return node.get("title") or (props.get("Node name for S&R") if isinstance(props, dict) else None) + # ls-nodes — recover node ids/types (so an agent can address minted nodes) # --------------------------------------------------------------------------- -def _subgraph_interior_rows(workflow: dict) -> list[dict]: +def _subgraph_interior_rows(workflow: dict) -> tuple[list[dict], bool]: """Rows for the nodes INSIDE every live subgraph instance, flattened. ``workflow["nodes"]`` only carries the top level: a subgraph instance is a @@ -564,13 +602,29 @@ def _subgraph_interior_rows(workflow: dict) -> list[dict]: nesting with ``/``), so a path from here is directly usable as a slot address prefix. Emitted under the sibling ``subgraph_nodes`` key, never appended to ``nodes`` — consumers render ``nodes[]`` verbatim and pin it. + + Returns ``(rows, truncated)``; ``truncated`` is True when the walk stopped at + ``_MAX_SUBGRAPH_INTERIOR_ROWS`` and the listing is therefore incomplete. + + Every container is shape-checked before use. ls-nodes reads an ARBITRARY + file — it is the one workflow command with no catalog and no validation gate + in front of it — so a corrupt document must yield an error envelope or a + short listing, never a traceback. """ from comfy_cli.cql.engine import _MAX_SUBGRAPH_DEPTH, _SUBGRAPH_PATH_SEP, _subgraph_defs_by_id + # `_subgraph_defs_by_id` assumes `definitions` is a dict whose `subgraphs` is + # a list (true of every ComfyUI save). Hand it `"definitions": 5` and it + # raises AttributeError; `"subgraphs": 5`, TypeError. Screen both here rather + # than in cql.engine, so this stays a change to ls-nodes' own contract. + defs = workflow.get("definitions") + if not isinstance(defs, dict) or not isinstance(defs.get("subgraphs"), list): + return [], False defs_by_id = _subgraph_defs_by_id(workflow) if not defs_by_id: - return [] + return [], False interior: list[dict] = [] + truncated = False def _def_for(node: dict) -> dict | None: """The subgraph definition a node instantiates, or None for a plain node. @@ -584,26 +638,38 @@ def _def_for(node: dict) -> dict | None: return defs_by_id.get(node_type) if isinstance(node_type, str) else None def walk(nodes: Any, prefix: str, instance: str, depth: int, seen_defs: frozenset[int]) -> None: + nonlocal truncated # Same depth cap the slot walker uses (cql.engine), for the same reason: # a hand-written or corrupt document can nest subgraphs pathologically. if depth > _MAX_SUBGRAPH_DEPTH: return - for n in nodes or []: + # A definition's `nodes` is a list in every save; `"nodes": 1` survives + # `or []` (it is truthy) and then raises TypeError on iteration. + if not isinstance(nodes, list): + return + for n in nodes: + if len(interior) >= _MAX_SUBGRAPH_INTERIOR_ROWS: + truncated = True + return if not isinstance(n, dict): continue node_id = n.get("id") # Stringified exactly as the slot walker stringifies it, so the two # commands' addresses stay byte-identical; ``id`` stays verbatim. - path = f"{prefix}{_SUBGRAPH_PATH_SEP}{str(n.get('id', ''))}" + # Joined the way the slot walker joins, too: prepending the separator + # unconditionally would turn an instance with no `id` (prefix "") + # into `/9`, whose leading empty segment resolves as no slot address. + node_path = str(n.get("id", "")) + path = f"{prefix}{_SUBGRAPH_PATH_SEP}{node_path}" if prefix else node_path row = { "path": path, "instance": instance, "id": node_id, "type": n.get("type"), - "title": n.get("title") or (n.get("properties") or {}).get("Node name for S&R"), + "title": _node_title(n), } # Label-only-when-set, exactly as the top-level rows do. - if (label := _MODE_LABELS.get(n.get("mode"))) is not None: + if (label := _mode_label(n)) is not None: row["mode"] = label interior.append(row) sg = _def_for(n) @@ -615,7 +681,10 @@ def walk(nodes: Any, prefix: str, instance: str, depth: int, seen_defs: frozense if sg is not None and id(sg) not in seen_defs: walk(sg.get("nodes"), path, instance, depth + 1, seen_defs | {id(sg)}) - for n in workflow.get("nodes") or []: + top = workflow.get("nodes") + for n in top if isinstance(top, list) else []: + if truncated: + break if not isinstance(n, dict): continue sg = _def_for(n) @@ -623,9 +692,12 @@ def walk(nodes: Any, prefix: str, instance: str, depth: int, seen_defs: frozense continue instance = str(n.get("id", "")) # Recurse whether or not the instance itself is muted: the consumer keys - # on the root row's own mode, which `nodes[]` already carries. + # on the root row's own mode, which `nodes[]` already carries. An + # interior instance's own row carries ITS mode too, so a caller deciding + # what actually executes joins the modes along a row's `path` prefixes — + # see the `subgraph_nodes` schema description. walk(sg.get("nodes"), instance, instance, 1, frozenset({id(sg)})) - return interior + return interior, truncated @tracking.track_command("workflow") @@ -642,7 +714,7 @@ def ls_nodes_cmd( row = { "id": n.get("id"), "type": n.get("type"), - "title": n.get("title") or (n.get("properties") or {}).get("Node name for S&R"), + "title": _node_title(n), } # ComfyUI disables a node without deleting it: mode 4 = bypass (input # passes through), mode 2 = mute/never (dropped from execution). Both are @@ -650,10 +722,10 @@ def ls_nodes_cmd( # from a live one — and would "repair" a graph that is merely bypassed, # or call a workflow runnable while a required node is muted. # Emitted only when set, so a normal node stays a single clean row. - if (label := _MODE_LABELS.get(n.get("mode"))) is not None: + if (label := _mode_label(n)) is not None: row["mode"] = label rows.append(row) - interior = _subgraph_interior_rows(workflow) + interior, truncated = _subgraph_interior_rows(workflow) # `count`/`nodes` keep their exact pre-existing meaning (top level only) — # interiors go under a NEW sibling key, because consumers render every # `nodes[]` entry verbatim and pin that listing. @@ -664,15 +736,25 @@ def ls_nodes_cmd( "subgraph_nodes": interior, "subgraph_count": len(interior), } + if truncated: + # Only when set, like `mode`: a normal listing carries no such key, and + # a consumer that must not act on a partial graph checks for it. + payload["subgraph_truncated"] = True if renderer.is_pretty(): from rich.table import Table + # Every cell below is copied verbatim out of the workflow file, and + # `Table.add_row` interprets a `str` as Rich MARKUP: an unbalanced `[/]` + # in a title raises MarkupError mid-render, `[link=...]` renders a live + # OSC 8 hyperlink, and raw control bytes reach the terminal. + # `sanitize_markup` escapes the markup and strips the escapes, and + # coerces non-strings, so it replaces the bare `str()` calls too. tbl = Table(show_header=True, header_style="bold") tbl.add_column("id", no_wrap=True) tbl.add_column("type") tbl.add_column("title", style="dim") for r in rows: - tbl.add_row(str(r["id"]), str(r["type"]), str(r["title"] or "")) + tbl.add_row(sanitize_markup(r["id"]), sanitize_markup(r["type"]), sanitize_markup(r["title"] or "")) renderer.console().print(tbl) if interior: sub = Table(show_header=True, header_style="bold", title="subgraph interiors") @@ -681,8 +763,18 @@ def ls_nodes_cmd( sub.add_column("title", style="dim") sub.add_column("mode", style="dim") for r in interior: - sub.add_row(r["path"], str(r["type"]), str(r["title"] or ""), r.get("mode", "")) + sub.add_row( + sanitize_markup(r["path"]), + sanitize_markup(r["type"]), + sanitize_markup(r["title"] or ""), + r.get("mode", ""), # ours, from _MODE_LABELS — never file text + ) renderer.console().print(sub) + if truncated: + rprint( + f"[yellow]![/yellow] subgraph interiors truncated at " + f"{_MAX_SUBGRAPH_INTERIOR_ROWS} rows; the listing is incomplete" + ) renderer.emit(payload, command="workflow ls-nodes") diff --git a/comfy_cli/schemas/workflow.json b/comfy_cli/schemas/workflow.json index ad2387ef7..8d69efe3c 100644 --- a/comfy_cli/schemas/workflow.json +++ b/comfy_cli/schemas/workflow.json @@ -8,14 +8,13 @@ "count": { "type": "integer" }, "slots": { "type": "array" }, "nodes": { - "type": "array", - "description": "ls-nodes: one row per TOP-LEVEL node — a subgraph instance is a single opaque row here. `count` is the length of this array.", + "description": "ls-nodes: an ARRAY with one row per TOP-LEVEL node — a subgraph instance is a single opaque row here, and `count` is the length of that array. Deliberately carries no `type` keyword: this schema is shared by the whole `workflow` command group, and `compose`/`decompose` reuse the `nodes` key for an INTEGER node count. The `items` subschema below therefore constrains only the array form.", "items": { "type": "object", "properties": { "id": { "description": "node id as serialized in the workflow" }, - "type": { "type": ["string", "null"] }, - "title": { "type": ["string", "null"] }, + "type": { "description": "class_type as serialized in the workflow; a string (or absent/null) in any well-formed file. Copied verbatim rather than coerced, so a hand-edited or corrupt file is reported as it actually is." }, + "title": { "description": "display title, else the `Node name for S&R` property; same verbatim-copy caveat as `type`" }, "mode": { "type": "string", "enum": ["mute", "bypass"], @@ -26,7 +25,7 @@ }, "subgraph_nodes": { "type": "array", - "description": "ls-nodes: one row per node INSIDE a live subgraph instance (definitions.subgraphs[].nodes, recursively) — these execute but never appear in `nodes`. Empty when the workflow has no subgraph instances.", + "description": "ls-nodes: one row per node INSIDE a live subgraph instance (definitions.subgraphs[].nodes, recursively) — nodes that never appear in `nodes`. Empty when the workflow has no subgraph instances. This listing INCLUDES disabled nodes: a row's own `mode` says whether it is muted/bypassed, and because a nested instance gets a row of its own, a caller deciding what actually EXECUTES must also check the `mode` of every ancestor along the row's `path` prefixes (a live `10/3/7` under a muted `10/3` is dropped by workflow_to_api, as is any interior under a disabled top-level instance — whose mode is on its `nodes[]` row).", "items": { "type": "object", "properties": { @@ -36,8 +35,8 @@ }, "instance": { "type": "string", "description": "id of the TOP-LEVEL subgraph instance this row is reached through" }, "id": { "description": "interior node id as serialized in the subgraph definition" }, - "type": { "type": ["string", "null"] }, - "title": { "type": ["string", "null"] }, + "type": { "description": "class_type as serialized in the definition; same verbatim-copy caveat as the `nodes[]` `type`" }, + "title": { "description": "display title, else the `Node name for S&R` property; same verbatim-copy caveat as `type`" }, "mode": { "type": "string", "enum": ["mute", "bypass"], @@ -50,6 +49,10 @@ "type": "integer", "description": "ls-nodes: length of `subgraph_nodes` (kept separate from `count`, which stays top-level-only)." }, + "subgraph_truncated": { + "type": "boolean", + "description": "ls-nodes: present and true ONLY when the interior walk hit its row ceiling, so `subgraph_nodes` is INCOMPLETE and `subgraph_count` is a floor rather than a total. Reachable only from a corrupt or hostile definition graph (a branching one can expand a few-KB file into billions of rows); a normal workflow never carries this key." + }, "notes": { "type": "array", "description": "notes: the Note/MarkdownNote documentation nodes the workflow carries, in graph order (top-level nodes first, then each subgraph definition).", diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index f276bc49e..916f856aa 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -394,13 +394,21 @@ comfy --json workflow ls-nodes wf.json # id / type `ls-nodes` emits `data.nodes[]` for the TOP LEVEL only (a subgraph instance is one opaque row), plus a sibling `data.subgraph_nodes[]` for the nodes INSIDE every live -subgraph instance — the ones that actually execute. Each interior row carries +subgraph instance — the ones `nodes[]` never shows you. Each interior row carries `path` / `instance` / `id` / `type` / `title`, and `mode` (`mute` | `bypass`) only when the node is disabled, same as the top-level rows. `path` uses the same `/` addressing as `comfy workflow slots` (nesting with `/`, e.g. `10/3/7`), so it composes directly into a slot address. `data.count` stays the top-level count; `data.subgraph_count` counts the interior rows. +Interior rows are a LISTING, not an execution plan: they include muted and bypassed +nodes, which is the point — `workflow_to_api` silently drops those, so this is the +only place a reader sees them. To decide what actually runs, check `mode` on the row +AND on every ancestor: a live `10/3/7` under a muted `10/3` (or under a disabled +instance `10`, whose `mode` is on its `nodes[]` row) does not execute. Rarely, +`data.subgraph_truncated: true` appears — the walk hit its row ceiling on a corrupt +definition graph, and the listing is incomplete. + **Building more than one or two nodes? Use `apply` — one batch, one catalog load, and `as` aliases so you never capture a minted id by hand:** diff --git a/tests/comfy_cli/command/test_ls_nodes_mode.py b/tests/comfy_cli/command/test_ls_nodes_mode.py index 58f6e6693..cfc845051 100644 --- a/tests/comfy_cli/command/test_ls_nodes_mode.py +++ b/tests/comfy_cli/command/test_ls_nodes_mode.py @@ -237,3 +237,190 @@ def test_malformed_definitions_do_not_crash(patched_graph, tmp_path, capsys): wf["definitions"]["subgraphs"][0]["nodes"].append(None) env = _env(tmp_path, wf, capsys) assert [r["path"] for r in env["data"]["subgraph_nodes"]] == ["10/9", "10/11"] + + +# --------------------------------------------------------------------------- +# Robustness of the interior walk (review of PR #845). +# +# `ls-nodes` is the one workflow command with no catalog and no validation gate +# in front of it — an agent points it at whatever JSON it was handed. Every case +# below raised an uncaught exception before the fix, so the CLI's contract (an +# error envelope, never a traceback) was broken by a corrupt file rather than a +# hostile one. Each was reproduced against the pre-fix walker. +# --------------------------------------------------------------------------- + + +def _wf_with_interior(interior_nodes) -> dict: + """One live instance 10 whose definition's `nodes` is exactly what's given.""" + return { + "last_node_id": 60, + "last_link_id": 0, + "nodes": [{"id": 10, "type": SG_UUID, "pos": [0, 0]}], + "links": [], + "definitions": {"subgraphs": [{"id": SG_UUID, "name": "S", "inputs": [], "nodes": interior_nodes}]}, + } + + +@pytest.mark.parametrize( + "wf, why", + [ + (_wf_with_interior(1), "a definition's `nodes` is a truthy scalar: survives `or []`, TypeError on iteration"), + ( + _wf_with_interior([{"id": 9, "type": "X", "properties": ["a"]}]), + "truthy non-dict `properties`: survives `or {}`, AttributeError on .get " + "(a falsy `[]` never reached the bug)", + ), + ( + _wf_with_interior([{"id": 9, "type": "X", "mode": []}]), + "unhashable `mode`: _MODE_LABELS.get hashes its argument -> TypeError", + ), + ( + {"nodes": [], "links": [], "definitions": 5}, + "`definitions` is not a dict: AttributeError inside _subgraph_defs_by_id", + ), + ( + {"nodes": [], "links": [], "definitions": {"subgraphs": 5}}, + "`subgraphs` is a truthy scalar: TypeError iterating it", + ), + ], +) +def test_malformed_shapes_emit_an_envelope_not_a_traceback(patched_graph, tmp_path, capsys, wf, why): + env = _env(tmp_path, wf, capsys) + assert env["ok"] is True, why + # A malformed container is SKIPPED, never guessed at: the walk yields no row + # for it rather than inventing one. + assert isinstance(env["data"]["subgraph_nodes"], list), why + + +def test_unhashable_mode_on_a_top_level_node_does_not_crash(patched_graph, tmp_path, capsys): + """The same hash hazard on the pre-existing top-level row builder: an + unhashable `mode` must fall through to "no label", not abort the command.""" + wf = _base_workflow() + wf["nodes"][0]["mode"] = [] + env = _env(tmp_path, wf, capsys) + assert all("mode" not in r for r in env["data"]["nodes"]), env["data"]["nodes"] + + +def test_interior_path_has_no_leading_separator_when_instance_has_no_id(patched_graph, tmp_path, capsys): + """An instance with no `id` gave `/9`, whose empty leading segment resolves + as no slot address. Join like the slot walker: separator only after a + non-empty prefix.""" + wf = _wf_with_interior([{"id": 9, "type": "CLIPTextEncode"}]) + del wf["nodes"][0]["id"] + env = _env(tmp_path, wf, capsys) + assert [r["path"] for r in env["data"]["subgraph_nodes"]] == ["9"] + + +def test_branching_definition_graph_is_bounded_and_flagged(patched_graph, tmp_path, capsys): + """The depth cap and the per-path `seen_defs` set bound CYCLES and LINEAR + nesting, but not BRANCHING: definitions that each hold two instances of the + next repeat no definition on any path and stay under the depth cap, yet + expand a few-KB file into ~2**depth rows. Only a total row budget bounds it, + and the consumer has to be told the listing is short.""" + from comfy_cli.command.workflow_edit import _MAX_SUBGRAPH_INTERIOR_ROWS + + uuids = [f"8f1e0a2c-0000-4000-8000-{i:012d}" for i in range(20)] + subgraphs = [] + for i, u in enumerate(uuids): + inner = [{"id": 1, "type": "VAEDecode"}] + if i + 1 < len(uuids): # two instances of the NEXT definition -> 2**i growth + inner += [{"id": 2, "type": uuids[i + 1]}, {"id": 3, "type": uuids[i + 1]}] + subgraphs.append({"id": u, "name": f"L{i}", "inputs": [], "nodes": inner, "links": []}) + wf = { + "last_node_id": 60, + "last_link_id": 0, + "nodes": [{"id": 10, "type": uuids[0], "pos": [0, 0]}], + "links": [], + "definitions": {"subgraphs": subgraphs}, + } + env = _env(tmp_path, wf, capsys) + assert len(env["data"]["subgraph_nodes"]) == _MAX_SUBGRAPH_INTERIOR_ROWS + assert env["data"]["subgraph_count"] == _MAX_SUBGRAPH_INTERIOR_ROWS + assert env["data"]["subgraph_truncated"] is True + + +def test_ordinary_workflow_carries_no_truncation_key(patched_graph, tmp_path, capsys): + """`subgraph_truncated` is label-only-when-set, like `mode`.""" + env = _env(tmp_path, _sg_workflow(), capsys) + assert "subgraph_truncated" not in env["data"], env["data"] + + +# --------------------------------------------------------------------------- +# Pretty mode: node text is workflow-file text, and Rich reads a `str` cell as +# MARKUP. `ls-nodes` is pointed at files an agent did not author (downloaded +# templates, a peer's export), so a title carrying `[/]` crashed the render with +# MarkupError and `[link=...]` rendered a live OSC 8 hyperlink. Same contract as +# tests/comfy_cli/command/test_pretty_print_sanitize.py, applied to both tables. +# --------------------------------------------------------------------------- + +HOSTILE = "\x1b[2J\x1b]0;PWNED\x07boom [link=https://attacker.example]click[/link]" +UNBALANCED_TITLE = "title [/] oops" + + +@pytest.fixture +def pretty_stream(monkeypatch): + """A pretty renderer over a stream Rich treats as a tty — `force_terminal` + is what makes Rich emit OSC 8 at all, so without it half the hazard hides.""" + import io + + from comfy_cli.output.renderer import OutputMode, Renderer, set_renderer + + monkeypatch.setenv("FORCE_COLOR", "1") + monkeypatch.setenv("COLUMNS", "300") + stream = io.StringIO() + r = Renderer.resolve(is_stdout_tty=True, env={}, caller=None) + r.mode = OutputMode.PRETTY + r.pretty_stream = stream + set_renderer(r) + return stream + + +def _render_pretty(tmp_path, wf: dict) -> str: + from typer.testing import CliRunner + + from comfy_cli.command import workflow as workflow_cmd + + path = _write(tmp_path, wf) + result = CliRunner().invoke(workflow_cmd.app, ["ls-nodes", str(path)], standalone_mode=False) + assert result.exception is None, result.exception + return result + + +def _assert_inert(out: str) -> None: + import re + + assert "\x1b]8;" not in out, "markup was rendered into a live OSC 8 hyperlink" + assert "\x1b]0;" not in out, "OSC 0 window-title sequence reached the terminal" + assert "\x1b[2J" not in out, "CSI 2J screen-clear reached the terminal" + residue = re.sub(r"\x1b\[[0-9;]*m", "", out) # Rich's own SGR styling + assert "\x1b" not in residue, f"escape byte survived: {residue!r}" + + +def test_pretty_top_level_row_text_is_inert(patched_graph, pretty_stream, tmp_path): + wf = _base_workflow() + wf["nodes"][0]["title"] = HOSTILE + _render_pretty(tmp_path, wf) + _assert_inert(pretty_stream.getvalue()) + + +def test_pretty_interior_row_text_is_inert(patched_graph, pretty_stream, tmp_path): + wf = _wf_with_interior([{"id": 9, "type": "CLIPTextEncode", "title": HOSTILE}]) + _render_pretty(tmp_path, wf) + _assert_inert(pretty_stream.getvalue()) + + +@pytest.mark.parametrize( + "wf_factory", + [ + pytest.param(lambda: _wf_with_interior([{"id": 9, "type": UNBALANCED_TITLE}]), id="interior-type"), + pytest.param( + lambda: _wf_with_interior([{"id": 9, "type": "X", "title": UNBALANCED_TITLE}]), id="interior-title" + ), + pytest.param(lambda: _wf_with_interior([{"id": UNBALANCED_TITLE, "type": "X"}]), id="interior-path"), + ], +) +def test_pretty_unbalanced_markup_does_not_crash_the_render(patched_graph, pretty_stream, tmp_path, wf_factory): + """An unbalanced `[/]` raises rich.errors.MarkupError mid-render, so the CLI + dies while merely printing a table.""" + _render_pretty(tmp_path, wf_factory()) + assert "oops" in pretty_stream.getvalue() From 56557238be5ea406ad9710a23816c3b3ecde4a2c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 10:30:52 -0700 Subject: [PATCH 3/4] test: drop the misleading return from the pretty-render helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The helper's contract is the assertion inside it — a MarkupError surfaces as `result.exception`, not a non-zero exit — and no caller used the value, which was annotated `str` while returning a `Result`. --- tests/comfy_cli/command/test_ls_nodes_mode.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/comfy_cli/command/test_ls_nodes_mode.py b/tests/comfy_cli/command/test_ls_nodes_mode.py index cfc845051..6712290f4 100644 --- a/tests/comfy_cli/command/test_ls_nodes_mode.py +++ b/tests/comfy_cli/command/test_ls_nodes_mode.py @@ -375,7 +375,9 @@ def pretty_stream(monkeypatch): return stream -def _render_pretty(tmp_path, wf: dict) -> str: +def _render_pretty(tmp_path, wf: dict) -> None: + """Render `ls-nodes` into the pretty stream. The assertion is the point: a + MarkupError surfaces as `result.exception`, not as a non-zero exit.""" from typer.testing import CliRunner from comfy_cli.command import workflow as workflow_cmd @@ -383,7 +385,6 @@ def _render_pretty(tmp_path, wf: dict) -> str: path = _write(tmp_path, wf) result = CliRunner().invoke(workflow_cmd.app, ["ls-nodes", str(path)], standalone_mode=False) assert result.exception is None, result.exception - return result def _assert_inert(out: str) -> None: From 69da7578a5d93314bf413d68c91793cebecadeb6 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 3 Sep 2026 10:57:12 -0700 Subject: [PATCH 4/4] fix(workflow): flag ls-nodes truncation only when a listable row is dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review of #845, two findings. The row-budget check ran BEFORE the `isinstance(n, dict)` shape filter, so a definition whose tail is junk we drop either way — `nodes: [ok, ok, 7, "x"]` at the ceiling — set `subgraph_truncated` even though nothing reportable was omitted. `subgraph_truncated` is a promise that a listable row is missing and `subgraph_count` is a floor; a consumer that re-reads the graph on that signal was being sent back for junk. Move the check below the filter. Its companion case (a real third node IS dropped) is pinned too, so moving the test cannot quietly disarm it. Second: the ceiling was documented in three places as reachable only from a "corrupt or hostile" definition graph. That is a verdict on the file the flag cannot support — 10k reachable interior rows is an OUTPUT ceiling, and a legitimately huge workflow hits it exactly as a branching-blowup one does. Schema, SKILL.md and the constant's comment now describe the condition (the listing exceeded the ceiling, so it is short) without classifying the input. Also: SKILL.md now tells a reader what to DO on truncation — re-read in smaller pieces — instead of only that the graph was bad. --- comfy_cli/command/workflow_edit.py | 14 +++++++---- comfy_cli/schemas/workflow.json | 2 +- comfy_cli/skills/comfy/SKILL.md | 4 ++-- tests/comfy_cli/command/test_ls_nodes_mode.py | 23 +++++++++++++++++++ 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/comfy_cli/command/workflow_edit.py b/comfy_cli/command/workflow_edit.py index e462a0076..2ae9f2542 100644 --- a/comfy_cli/command/workflow_edit.py +++ b/comfy_cli/command/workflow_edit.py @@ -554,8 +554,10 @@ def reset_doc_cmd( # such levels already yield 8189). Every row is accumulated in memory, then # serialized to JSON and rendered, so the depth cap alone is not a work bound. # Real graphs are nowhere near this — the largest subgraph-heavy workflows run -# to low hundreds of interior nodes — so a document that reaches this ceiling is -# corrupt or hostile, and `subgraph_truncated` tells the consumer so. +# to low hundreds of interior nodes — but this is an output ceiling, not a +# verdict on the file: a legitimately huge graph reaches it the same way a +# corrupt or hostile one does, and `subgraph_truncated` tells the consumer only +# that the listing is short. _MAX_SUBGRAPH_INTERIOR_ROWS = 10_000 @@ -648,11 +650,15 @@ def walk(nodes: Any, prefix: str, instance: str, depth: int, seen_defs: frozense if not isinstance(nodes, list): return for n in nodes: + if not isinstance(n, dict): + continue + # Checked AFTER the shape filter: `subgraph_truncated` promises a + # REPORTABLE row was omitted, and a malformed entry would never have + # produced one. Testing first would flag a listing as short because + # the tail of the document was junk we drop either way. if len(interior) >= _MAX_SUBGRAPH_INTERIOR_ROWS: truncated = True return - if not isinstance(n, dict): - continue node_id = n.get("id") # Stringified exactly as the slot walker stringifies it, so the two # commands' addresses stay byte-identical; ``id`` stays verbatim. diff --git a/comfy_cli/schemas/workflow.json b/comfy_cli/schemas/workflow.json index 8d69efe3c..03a6d5aa1 100644 --- a/comfy_cli/schemas/workflow.json +++ b/comfy_cli/schemas/workflow.json @@ -51,7 +51,7 @@ }, "subgraph_truncated": { "type": "boolean", - "description": "ls-nodes: present and true ONLY when the interior walk hit its row ceiling, so `subgraph_nodes` is INCOMPLETE and `subgraph_count` is a floor rather than a total. Reachable only from a corrupt or hostile definition graph (a branching one can expand a few-KB file into billions of rows); a normal workflow never carries this key." + "description": "ls-nodes: present and true ONLY when the interior walk hit its row ceiling, so `subgraph_nodes` is INCOMPLETE and `subgraph_count` is a floor rather than a total. Any listing whose reachable interior rows exceed the ceiling sets it — usually a branching definition graph, which can expand a few-KB file into billions of rows, but a legitimately huge workflow reaches it the same way. Ordinary workflows are far below the ceiling and never carry this key." }, "notes": { "type": "array", diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index 916f856aa..ed0cc3e07 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -406,8 +406,8 @@ nodes, which is the point — `workflow_to_api` silently drops those, so this is only place a reader sees them. To decide what actually runs, check `mode` on the row AND on every ancestor: a live `10/3/7` under a muted `10/3` (or under a disabled instance `10`, whose `mode` is on its `nodes[]` row) does not execute. Rarely, -`data.subgraph_truncated: true` appears — the walk hit its row ceiling on a corrupt -definition graph, and the listing is incomplete. +`data.subgraph_truncated: true` appears — the walk hit its row ceiling, so the listing +is incomplete and `subgraph_count` is a floor; re-read the graph in smaller pieces. **Building more than one or two nodes? Use `apply` — one batch, one catalog load, and `as` aliases so you never capture a minted id by hand:** diff --git a/tests/comfy_cli/command/test_ls_nodes_mode.py b/tests/comfy_cli/command/test_ls_nodes_mode.py index 6712290f4..6e065d518 100644 --- a/tests/comfy_cli/command/test_ls_nodes_mode.py +++ b/tests/comfy_cli/command/test_ls_nodes_mode.py @@ -345,6 +345,29 @@ def test_ordinary_workflow_carries_no_truncation_key(patched_graph, tmp_path, ca assert "subgraph_truncated" not in env["data"], env["data"] +def test_malformed_tail_at_the_row_ceiling_does_not_flag_truncation(patched_graph, tmp_path, capsys, monkeypatch): + """`subgraph_truncated` promises a REPORTABLE row was omitted. + + The budget check ran BEFORE the shape filter, so a definition whose last + entries are junk we drop either way — `nodes: [ok, ok, 7, "x"]` at a ceiling + of 2 — reported the listing as short when nothing listable was left.""" + monkeypatch.setattr(workflow_edit, "_MAX_SUBGRAPH_INTERIOR_ROWS", 2) + wf = _wf_with_interior([{"id": 1, "type": "VAEDecode"}, {"id": 2, "type": "VAEDecode"}, 7, "x"]) + env = _env(tmp_path, wf, capsys) + assert len(env["data"]["subgraph_nodes"]) == 2 + assert "subgraph_truncated" not in env["data"], env["data"] + + +def test_a_dropped_row_at_the_ceiling_still_flags_truncation(patched_graph, tmp_path, capsys, monkeypatch): + """The other half of the same check: a real third node IS omitted, so the + flag must still fire — moving the budget test must not disarm it.""" + monkeypatch.setattr(workflow_edit, "_MAX_SUBGRAPH_INTERIOR_ROWS", 2) + wf = _wf_with_interior([{"id": i, "type": "VAEDecode"} for i in (1, 2, 3)]) + env = _env(tmp_path, wf, capsys) + assert len(env["data"]["subgraph_nodes"]) == 2 + assert env["data"]["subgraph_truncated"] is True + + # --------------------------------------------------------------------------- # Pretty mode: node text is workflow-file text, and Rich reads a `str` cell as # MARKUP. `ls-nodes` is pointed at files an agent did not author (downloaded