diff --git a/comfy_cli/command/workflow_edit.py b/comfy_cli/command/workflow_edit.py index 39e03974..2ae9f254 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,11 +546,166 @@ 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 — 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 + + +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) -> 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 + 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. + + 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 [], 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. + + 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: + 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 + # 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 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 + node_id = n.get("id") + # Stringified exactly as the slot walker stringifies it, so the two + # commands' addresses stay byte-identical; ``id`` stays verbatim. + # 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": _node_title(n), + } + # Label-only-when-set, exactly as the top-level rows do. + if (label := _mode_label(n)) 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)}) + + 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) + 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. 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, truncated + + @tracking.track_command("workflow") def ls_nodes_cmd( file: Annotated[str, typer.Argument(help="Frontend-format workflow JSON.")], @@ -564,7 +720,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 @@ -572,20 +728,59 @@ 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) - payload = {"workflow": str(p), "count": len(rows), "nodes": rows} + 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. + payload = { + "workflow": str(p), + "count": len(rows), + "nodes": rows, + "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") + 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( + 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 45667d51..03a6d5aa 100644 --- a/comfy_cli/schemas/workflow.json +++ b/comfy_cli/schemas/workflow.json @@ -7,6 +7,52 @@ "workflow": { "type": "string" }, "count": { "type": "integer" }, "slots": { "type": "array" }, + "nodes": { + "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": { "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"], + "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) — 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": { + "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": { "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"], + "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)." + }, + "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. 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", "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 4517faa3..ed0cc3e0 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -392,6 +392,23 @@ 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 `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, 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 f6997b61..6e065d51 100644 --- a/tests/comfy_cli/command/test_ls_nodes_mode.py +++ b/tests/comfy_cli/command/test_ls_nodes_mode.py @@ -73,3 +73,378 @@ 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"] + + +# --------------------------------------------------------------------------- +# 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"] + + +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 +# 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) -> 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 + + 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 + + +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()