From 937c8adf0ecf9539a6cd15a9de27b688d070f56c Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:24:54 -0700 Subject: [PATCH 01/10] wip: cap enum option lists, cap nodes show choices, select row queries Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/command/nodes.py | 66 ++++++- comfy_cli/command/workflow.py | 25 ++- comfy_cli/cql/engine.py | 118 ++++++++--- comfy_cli/selector.py | 198 ++++++++++++++++++- tests/comfy_cli/test_selector_conformance.py | 26 +++ tests/data/selector_conformance.json | 65 ++++++ 6 files changed, 461 insertions(+), 37 deletions(-) create mode 100644 tests/comfy_cli/test_selector_conformance.py create mode 100644 tests/data/selector_conformance.json diff --git a/comfy_cli/command/nodes.py b/comfy_cli/command/nodes.py index 7d842ecc0..49666dd31 100644 --- a/comfy_cli/command/nodes.py +++ b/comfy_cli/command/nodes.py @@ -156,6 +156,48 @@ def _category_matches(category: str | None, pat: str) -> bool: # --------------------------------------------------------------------------- +#: A combo input's ``choices`` longer than this are capped in ``nodes show`` / +#: ``nodes search --expand-top``. A loader's choices are every installed file +#: (600+ LoRAs on cloud, ~31KB in one show), and a caller wiring the node +#: needs one of them, by a name it usually already has. +CHOICES_INLINE_MAX = 20 + + +def _cap_choices(payload: dict[str, Any]) -> dict[str, Any]: + """Cap every long ``choices`` list in a show payload (recursing into + dynamic-combo sub-inputs) to its first :data:`CHOICES_INLINE_MAX` entries, + with ``choices_total`` and ``choices_truncated``. Adds a top-level + ``choices_note`` naming how to read or filter the full list, once.""" + capped: list[str] = [] + + def walk(inputs: Any) -> None: + if not isinstance(inputs, list): + return + for entry in inputs: + if not isinstance(entry, dict): + continue + choices = entry.get("choices") + if isinstance(choices, list) and len(choices) > CHOICES_INLINE_MAX: + entry["choices_total"] = len(choices) + entry["choices"] = choices[:CHOICES_INLINE_MAX] + entry["choices_truncated"] = True + capped.append(str(entry.get("name"))) + for option in entry.get("dynamic_options") or []: + if isinstance(option, dict): + walk(option.get("inputs")) + + walk(payload.get("inputs")) + if capped: + name = payload.get("name") or "" + first = capped[0] + payload["choices_note"] = ( + f"{', '.join(capped)}: only the first {CHOICES_INLINE_MAX} choices are listed (see choices_total). " + f"Check or find one with `comfy nodes show {name} --select " + f'\'inputs.#(name=="{first}").choices.#(%"**")#\'`; --all-choices lists every choice.' + ) + return payload + + @app.command( "ls", help="List node classes. Filter via --produces/--accepts/--category/--pack/--label or boolean flags.", @@ -374,9 +416,18 @@ def show_cmd( typer.Option( "--select", show_default=False, - help="Project the payload: dot path (inputs.0.name), wildcard (inputs.#.name), comma multi-select.", + help="Project the payload: dot path (inputs.0.name), wildcard (inputs.#.name), comma multi-select, " + 'row query (inputs.#(name=="ckpt_name").choices). Projects the full schema, every choice included.', ), ] = None, + all_choices: Annotated[ + bool, + typer.Option( + "--all-choices", + help=f"List every choice of a combo input. By default a list longer than {CHOICES_INLINE_MAX} is cut " + "to its first entries, with `choices_total`.", + ), + ] = False, ): renderer = get_renderer() _stale: dict = {} @@ -457,6 +508,8 @@ def show_cmd( from comfy_cli.selector import emit_selected return emit_selected(renderer, payload, select, command="nodes show") + if not all_choices: + _cap_choices(payload) if renderer.is_pretty(): from rich.table import Table @@ -528,6 +581,14 @@ def search_cmd( ), ), ] = 0, + all_choices: Annotated[ + bool, + typer.Option( + "--all-choices", + help=f"With --expand-top: list every choice of a combo input (default: the first {CHOICES_INLINE_MAX}, " + "with `choices_total`).", + ), + ] = False, include_deprecated: IncludeDeprecatedOpt = False, input_path: Annotated[ str | None, @@ -667,7 +728,8 @@ def search_cmd( } ) continue - expanded.append({"class_type": m.id, **graph.morphism_to_dict(resolved)}) + schema = graph.morphism_to_dict(resolved) + expanded.append({"class_type": m.id, **(schema if all_choices else _cap_choices(schema))}) payload["expanded"] = expanded if _stale: diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index 4a4214c96..f7d7089ae 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -1591,6 +1591,7 @@ def validate_api_workflow( port: int | None = None, input_path: str | None = None, command: str = "workflow validate", + full_options: bool = False, ) -> None: """Validate an API-format workflow without submitting it. @@ -1764,7 +1765,10 @@ def validate_api_workflow( wf_data = converted converted_from_ui = True - result = graph.validate_workflow(wf_data) + from comfy_cli.cql.engine import full_enum_options + + with full_enum_options(full_options): + result = graph.validate_workflow(wf_data) # When the caller handed us a CANVAS graph, they have never seen the # flattened ids the lowering mints for subgraph interiors (`57:3`) — their @@ -1881,7 +1885,8 @@ def _invalid_workflow_error(result: dict[str, Any]) -> dict[str, Any] | None: for error in errors[:5]: line = f"node {error.get('node_id') or '?'}: {error.get('message', '')}" suggestions = error.get("suggestions") or [] - if suggestions: + # An enum message already names its closest options ("— closest: …"). + if suggestions and "closest:" not in line: line += f" (did you mean: {', '.join(str(s) for s in suggestions)}?)" hint_parts.append(line) # The code is a registered catch-all raised for every verdict, so it alone @@ -1958,9 +1963,23 @@ def validate_cmd( str | None, typer.Option("--input", show_default=False, help="Path to a saved object_info JSON (offline mode)."), ] = None, + full_options: Annotated[ + bool, + typer.Option( + "--full-options", + help="List every option of a rejected enum value as `valid_options`. By default an error names the " + "closest options and `option_count`, and carries the whole list only when it is short.", + ), + ] = False, ): validate_api_workflow( - workflow, where=where, host=host, port=port, input_path=input_path, command="workflow validate" + workflow, + where=where, + host=host, + port=port, + input_path=input_path, + command="workflow validate", + full_options=full_options, ) diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index 25f51fe35..655947e61 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -19,6 +19,9 @@ import urllib.parse import urllib.request from collections import defaultdict +from collections.abc import Iterator +from contextlib import contextmanager +from contextvars import ContextVar from dataclasses import dataclass, field, replace from typing import Any @@ -73,6 +76,70 @@ # and stays writable, while the injected button/player/viewport slots (these # two plus the ``PREVIEW_3D`` ``image`` of ``_PREVIEW_3D_CLASSES``) are refused # by every write surface — see ``_WidgetEntry.frontend_injected``. +# --------------------------------------------------------------------------- +# Enum option listings on findings +# --------------------------------------------------------------------------- +# +# An enum finding used to carry the WHOLE option list, twice (``suggestions`` +# and ``valid_options``). On a model loader that is every installed file: one +# validate of a graph with eleven bad filenames over a 377-file folder came to +# ~340K tokens in production. The closest few options and the count are what a +# caller acts on; the full list stays one explicit request away. + +#: An option list this short is cheap and is carried whole as ``valid_options``. +ENUM_INLINE_MAX = 12 +#: How many closest options a finding names. +ENUM_SUGGEST_MAX = 5 + +_FULL_ENUM_OPTIONS: ContextVar[bool] = ContextVar("comfy_full_enum_options", default=False) + + +@contextmanager +def full_enum_options(enabled: bool = True) -> Iterator[None]: + """Within this block, findings carry every option as ``valid_options`` + whatever the list's length (``validate --full-options``).""" + token = _FULL_ENUM_OPTIONS.set(enabled) + try: + yield + finally: + _FULL_ENUM_OPTIONS.reset(token) + + +def enum_option_fields(options: list, closest: list | None = None) -> dict[str, Any]: + """The option-listing fields of one enum finding. + + ``suggestions`` is the ``closest`` matches (or, with none, the first + options), at most :data:`ENUM_SUGGEST_MAX`; ``option_count`` is the whole + list's length. ``valid_options`` is the full, typed list only when it is + short (:data:`ENUM_INLINE_MAX`) or the caller asked for it + (:func:`full_enum_options`); otherwise ``options_omitted`` says how many + options the finding does not name. + """ + opts = list(options) + suggestions = list(closest or [])[:ENUM_SUGGEST_MAX] or opts[:ENUM_SUGGEST_MAX] + out: dict[str, Any] = {"suggestions": suggestions, "option_count": len(opts)} + if len(opts) <= ENUM_INLINE_MAX or _FULL_ENUM_OPTIONS.get(): + out["valid_options"] = opts + else: + out["options_omitted"] = len(opts) - len(suggestions) + return out + + +def enum_listing_hint(field: str, options: list, suggestions: list, class_type: str | None = None) -> str: + """The hint of an enum finding. A short list is named whole; a long one + is pointed at — the closest options are already in ``suggestions`` — with + the way to filter the rest instead of dumping it.""" + n = len(options) + if n <= ENUM_INLINE_MAX or _FULL_ENUM_OPTIONS.get(): + return f"valid options: {', '.join(str(v) for v in options)}" + target = class_type or "" + query = f'inputs.#(name=="{field}").choices.#(%"**")#' + return ( + f"pick one of `suggestions` (the closest of {n} options); to search all {n}, filter them with " + f"`comfy nodes show {target} --select '{query}'`, or re-run validate with --full-options" + ) + + FRONTEND_MARKER_SLOTS = frozenset({"control_after_generate", "upload", "audioUI"}) # ``Comfy.AudioWidget`` appends an ``audioUI`` player to exactly these classes. @@ -506,17 +573,22 @@ def validate_catalog(self, value: Any) -> list[dict]: candidates.add(str(int(value))) enum_str = {str(e) for e in self.enum_values} if not (candidates & enum_str): + suggestions = self.suggest_combo(value, limit=ENUM_SUGGEST_MAX) + best = self.best_combo_match(value) + if best is not None: + suggestions = [best, *(s for s in suggestions if s != best)][:ENUM_SUGGEST_MAX] + listing = enum_option_fields(self.enum_values, suggestions) warning = { "code": "unknown_enum_value", "field": self.name, "message": f"{value!r} not in {len(self.enum_values)} known options for {self.name}", - "valid_options": list(self.enum_values), + "option_count": listing["option_count"], } - suggestions = self.suggest_combo(value) - best = self.best_combo_match(value) if best is not None: - suggestions = [best, *(s for s in suggestions if s != best)] warning["best_match"] = best + for key in ("valid_options", "options_omitted"): + if key in listing: + warning[key] = listing[key] if suggestions: warning["did_you_mean"] = suggestions warning["message"] += f" — closest: {', '.join(suggestions)}" @@ -2497,6 +2569,12 @@ def _output_reachable_node_ids(workflow: dict[str, Any], graph: Graph) -> set[st return reachable +def _closest_options(value: Any, options: list) -> list: + """Up to :data:`ENUM_SUGGEST_MAX` options closest to ``value`` by name.""" + by_str = {str(o): o for o in options} + return [by_str[m] for m in difflib.get_close_matches(str(value), list(by_str), n=ENUM_SUGGEST_MAX, cutoff=0.5)] + + def _validate_catalog_value( node_id: str, class_type: str, input_name: str, port: Port, value: Any, *, range_is_error: bool = True ) -> tuple[list[dict], list[dict]]: @@ -2514,23 +2592,17 @@ def _validate_catalog_value( warnings: list[dict] = [] for w in port.validate_catalog(value): if w["code"] == "unknown_enum_value": - top = port.enum_values[:8] + # The closest options and the count, not the whole list (see + # enum_option_fields): a short list is still carried whole. + listing = enum_option_fields(port.enum_values, w.get("did_you_mean")) errors.append( { "node_id": node_id, "field": input_name, "code": "unknown_enum_value", "message": w["message"], - "hint": f"valid options include: {', '.join(str(v) for v in top)}" - + ( - f" (and {len(port.enum_values) - 8} more — see valid_options)" - if len(port.enum_values) > 8 - else "" - ), - "suggestions": port.enum_values[:20], - # full, typed list — never truncated, so the agent - # can pick a real value instead of guessing. - "valid_options": list(port.enum_values), + "hint": enum_listing_hint(input_name, port.enum_values, listing["suggestions"], class_type), + **listing, } ) elif w["code"] == "no_options_available": @@ -2833,9 +2905,8 @@ def _check_dynamic_combo_input( # the colon is self-refuting — say plainly that the schema # couldn't be read instead of dangling an empty enumeration. hint = ( - f"set {name!r} to one of its options: " - + ", ".join(str(k) for k in keys[:8]) - + (f" (and {len(keys) - 8} more — see valid_options)" if len(keys) > 8 else "") + f"set {name!r} to one of its options — " + + enum_listing_hint(name, keys, keys[:ENUM_SUGGEST_MAX], class_type) if keys else f"{name!r} is required, but its option schema didn't parse — check object_info for this node" ) @@ -2849,8 +2920,7 @@ def _check_dynamic_combo_input( f"which option's sub-inputs apply, so this node fails at execution" ), "hint": hint, - "suggestions": keys[:20], - "valid_options": keys, + **enum_option_fields(keys), } ) return errors, warnings, set(), {f"{name}."} @@ -2902,10 +2972,10 @@ def _check_dynamic_combo_input( f"{selected!r} not in {len(keys)} known options for {name} — its sub-inputs " f"cannot be resolved, so this node fails at execution" ), - "hint": f"valid options: {', '.join(str(k) for k in keys[:8])}" - + (f" (and {len(keys) - 8} more — see valid_options)" if len(keys) > 8 else ""), - "suggestions": keys[:20], - "valid_options": keys, + "hint": enum_listing_hint( + name, keys, _closest_options(selected, keys) or keys[:ENUM_SUGGEST_MAX], class_type + ), + **enum_option_fields(keys, _closest_options(selected, keys)), } ) return errors, warnings, set(), {f"{name}."} diff --git a/comfy_cli/selector.py b/comfy_cli/selector.py index 3d9891b0c..95cea0bec 100644 --- a/comfy_cli/selector.py +++ b/comfy_cli/selector.py @@ -3,8 +3,8 @@ This module is the single selector implementation for the CLI (V1-011 / C4): the four heaviest read commands (``templates ls``, ``nodes show``, ``workflow slots``, ``generate list``) accept ``--select `` and project -their JSON payload through it. No second dialect will ever be added — keep the -grammar exactly this small. +their JSON payload through it. No second dialect will ever be added — the +grammar is gjson's path syntax, kept to the subset below. Grammar (gjson-style dot paths): @@ -20,9 +20,23 @@ - **multi-select** — ``name,inputs`` splits on commas and returns an object keyed by each sub-expression that matched. It is a miss only when every part misses. - -There is no escaping: keys containing ``.``, ``,`` or ``#`` cannot be -addressed. Malformed expressions (empty, empty segment, empty part) are + - **row query** (gjson's) — ``items.#()#`` keeps the array elements + that satisfy ```` (the rest of the path maps over them, like ``#``); + ``items.#()`` is the FIRST such element (the rest of the path walks + it). ```` is `` ``, `` `` for an array of + scalars, or a bare ```` (the element has it, non-null). ```` is + a dot path inside the element; ```` is one of ``==`` ``!=`` ``<`` + ``<=`` ``>`` ``>=`` ``%`` (glob match, ``*`` and ``?``) ``!%``; ```` + is a ``"double-quoted"`` string (``\\"`` escapes a quote), a number, + ``true``, ``false`` or ``null``. ``==``/``!=`` compare a number and a + string by their text (``instance_id=="5"`` matches ``5``); the order + operators compare two numbers or two strings, never a mix. Zero matching + elements is an answer, not a miss: ``#(…)#`` returns ``[]``; ``#(…)`` with + no match is a miss. Dots and commas inside a row query are its own + (``#(value=="a.b,c")``). + +Outside a row query there is no escaping: keys containing ``.``, ``,`` or +``#`` cannot be addressed. Malformed expressions (empty, empty segment, empty part) are reported as a miss, never an error — the CLI fails open (see ``selected_payload``): the command still succeeds and returns a bounded key inventory of the full payload plus a ``select_no_match`` advisory so the @@ -53,7 +67,10 @@ def select(data: Any, expr: str) -> tuple[Any, bool]: """ if not isinstance(expr, str) or not expr.strip(): return None, False - parts = [p.strip() for p in expr.split(",")] + parts = _split_top(expr, ",") + if parts is None: + return None, False + parts = [p.strip() for p in parts] if len(parts) > 1: out: dict[str, Any] = {} for part in parts: @@ -66,19 +83,184 @@ def select(data: Any, expr: str) -> tuple[Any, bool]: return _select_one(data, parts[0]) +def _split_top(text: str, sep: str) -> list[str] | None: + """Split ``text`` on ``sep`` outside any ``#(...)`` row query and outside + quoted strings in one. ``None`` for an unbalanced query or quote.""" + parts: list[str] = [] + depth = 0 + in_str = False + start = 0 + i = 0 + while i < len(text): + ch = text[i] + if in_str: + if ch == "\\": + i += 1 + elif ch == '"': + in_str = False + elif ch == '"' and depth: + in_str = True + elif ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth < 0: + return None + elif ch == sep and depth == 0: + parts.append(text[start:i]) + start = i + 1 + i += 1 + if depth or in_str: + return None + parts.append(text[start:]) + return parts + + def _select_one(data: Any, path: str) -> tuple[Any, bool]: if not path: return None, False - segments = path.split(".") - if any(seg == "" for seg in segments): + segments = _split_top(path, ".") + if segments is None or any(seg == "" for seg in segments): return None, False return _walk(data, segments) +_QUERY_OPS = ("==", "!=", "<=", ">=", "!%", "<", ">", "%") + + +def _parse_query(seg: str) -> tuple[tuple[list[str], str | None, Any], bool] | None: + """Parse a ``#()`` / ``#()#`` segment to + ``((key_path, op, value), all_matches)``; ``None`` when ``seg`` is not a + well-formed row query.""" + if not seg.startswith("#("): + return None + if seg.endswith(")#"): + body, every = seg[2:-2], True + elif seg.endswith(")"): + body, every = seg[2:-1], False + else: + return None + # The operator is the first one outside a quoted value: values are always + # quoted when they are strings, so a key never contains a quote. + quote = body.find('"') + head = body if quote < 0 else body[:quote] + op_at, op = -1, None + for candidate in _QUERY_OPS: + at = head.find(candidate) + if at >= 0 and (op_at < 0 or at < op_at or (at == op_at and len(candidate) > len(op))): + op_at, op = at, candidate + if op is None: + key = body.strip() + if not key or '"' in key: + return None + path = key.split(".") + return ((path, None, None), every) if all(path) else None + key = body[:op_at].strip() + raw = body[op_at + len(op) :].strip() + path = key.split(".") if key else [] + if not all(path): + return None + ok, value = _parse_value(raw) + if not ok: + return None + if op in ("%", "!%") and not isinstance(value, str): + return None + return (path, op, value), every + + +def _parse_value(raw: str) -> tuple[bool, Any]: + if len(raw) >= 2 and raw[0] == '"' and raw[-1] == '"': + try: + value = json.loads(raw) + except ValueError: + return False, None + return isinstance(value, str), value + if raw in ("true", "false", "null"): + return True, {"true": True, "false": False, "null": None}[raw] + try: + number = json.loads(raw) + except ValueError: + return False, None + if isinstance(number, int | float) and not isinstance(number, bool): + return True, number + return False, None + + +def _is_number(value: Any) -> bool: + return isinstance(value, int | float) and not isinstance(value, bool) + + +def _glob(pattern: str, text: str) -> bool: + """gjson's match: ``*`` any run, ``?`` one character, everything else literal.""" + import re + + regex = "".join(".*" if c == "*" else "." if c == "?" else re.escape(c) for c in pattern) + return re.fullmatch(regex, text, flags=re.DOTALL) is not None + + +def _text(value: Any) -> str: + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, float) and value.is_integer(): + return str(int(value)) + return str(value) + + +def _satisfies(element: Any, cond: tuple[list[str], str | None, Any]) -> bool: + path, op, want = cond + got: Any = element + for key in path: + if not isinstance(got, Mapping) or key not in got: + return False + got = got[key] + if op is None: + return got is not None + if op in ("%", "!%"): + if not isinstance(got, str): + return False + return _glob(want, got) is (op == "%") + if op in ("==", "!="): + if _is_number(got) and _is_number(want): + equal = got == want + elif isinstance(got, Mapping | list) or isinstance(want, Mapping | list): + equal = False + elif got is None or want is None or isinstance(got, bool) or isinstance(want, bool): + equal = type(got) is type(want) and got == want + else: + equal = _text(got) == _text(want) + return equal is (op == "==") + if _is_number(got) and _is_number(want): + a, b = got, want + elif isinstance(got, str) and isinstance(want, str): + a, b = got, want + else: + return False + return {"<": a < b, "<=": a <= b, ">": a > b, ">=": a >= b}[op] + + def _walk(current: Any, segments: list[str]) -> tuple[Any, bool]: if not segments: return current, True seg, rest = segments[0], segments[1:] + if seg.startswith("#("): + parsed = _parse_query(seg) + if parsed is None or not isinstance(current, list): + return None, False + cond, every = parsed + if not every: + for element in current: + if _satisfies(element, cond): + return _walk(element, rest) + return None, False + kept = [element for element in current if _satisfies(element, cond)] + if not rest: + return kept, True + out = [] + for element in kept: + result, matched = _walk(element, rest) + if matched: + out.append(result) + return out, True if seg == WILDCARD: if not isinstance(current, list): return None, False diff --git a/tests/comfy_cli/test_selector_conformance.py b/tests/comfy_cli/test_selector_conformance.py new file mode 100644 index 000000000..26d65b6f8 --- /dev/null +++ b/tests/comfy_cli/test_selector_conformance.py @@ -0,0 +1,26 @@ +"""Replays tests/data/selector_conformance.json through ``comfy_cli.selector``. + +The corpus is language-neutral on purpose: the cloud agent holds a Go twin of +this grammar (its recall tool projects archived payloads it cannot hand back +to the CLI) and replays a verbatim copy of the same file, so a change here that +the twin does not make fails there too. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from comfy_cli.selector import select + +CORPUS = json.loads((Path(__file__).parent.parent / "data" / "selector_conformance.json").read_text(encoding="utf-8")) + + +@pytest.mark.parametrize("case", CORPUS["cases"], ids=[c["expr"] or "" for c in CORPUS["cases"]]) +def test_selector_conformance(case): + result, matched = select(CORPUS["data"], case["expr"]) + assert matched is case["matched"] + if case["matched"]: + assert result == case["result"] diff --git a/tests/data/selector_conformance.json b/tests/data/selector_conformance.json new file mode 100644 index 000000000..a394e3cc1 --- /dev/null +++ b/tests/data/selector_conformance.json @@ -0,0 +1,65 @@ +{ + "_comment": "Language-neutral conformance corpus for the --select grammar (comfy_cli/selector.py). Each case is (expr) over `data` -> (matched, result). The cloud agent's Go twin (services/agent/internal/loop/selector.go) replays a verbatim copy of this file, so the two implementations cannot drift. Run by tests/comfy_cli/test_selector_conformance.py.", + "version": 2, + "data": { + "rows": [ + {"name": "a", "tags": ["x", "y"]}, + {"name": "b", "tags": []}, + {"nameless": 1} + ], + "count": 3, + "meta": {"k": "v"}, + "slots": [ + {"address": "5.seed", "instance_id": 5, "node_type": "KSampler", "current_value": 1}, + {"address": "5.steps", "instance_id": 5, "node_type": "KSampler", "current_value": 20}, + {"address": "70/2011.text", "instance_id": "70/2011", "node_type": "CLIPTextEncode", "current_value": ""}, + {"address": "70/2012.text", "instance_id": "70/2012", "node_type": "CLIPTextEncode", "current_value": "a cat, 4k"}, + {"address": "9.text", "instance_id": 9, "node_type": "CLIPTextEncode", "current_value": "a.b,c"} + ], + "inputs": [ + {"name": "lora_name", "choices": ["MoXinV1.safetensors", "add_detail.safetensors", "moxin_v2.safetensors"]}, + {"name": "strength_model", "choices": []} + ] + }, + "cases": [ + {"expr": "rows.#.name", "matched": true, "result": ["a", "b"]}, + {"expr": "rows.0.name", "matched": true, "result": "a"}, + {"expr": "rows.#.tags.#", "matched": true, "result": [["x", "y"], []]}, + {"expr": "count,rows.#.name", "matched": true, "result": {"count": 3, "rows.#.name": ["a", "b"]}}, + {"expr": "nope", "matched": false}, + {"expr": "rows.#.zzz", "matched": false}, + {"expr": "", "matched": false}, + {"expr": "rows..name", "matched": false}, + {"expr": "meta.k", "matched": true, "result": "v"}, + {"expr": "rows.2.nameless", "matched": true, "result": 1}, + {"expr": "count,nope", "matched": true, "result": {"count": 3}}, + {"expr": "#", "matched": false}, + {"expr": "rows.#.tags", "matched": true, "result": [["x", "y"], []]}, + + {"expr": "slots.#(node_type==\"KSampler\")#.address", "matched": true, "result": ["5.seed", "5.steps"]}, + {"expr": "slots.#(node_type==\"KSampler\").address", "matched": true, "result": "5.seed"}, + {"expr": "slots.#(node_type==\"Nope\")#", "matched": true, "result": []}, + {"expr": "slots.#(node_type==\"Nope\")#.address", "matched": true, "result": []}, + {"expr": "slots.#(node_type==\"Nope\").address", "matched": false}, + {"expr": "slots.#(current_value==\"\")#.address", "matched": true, "result": ["70/2011.text"]}, + {"expr": "slots.#(instance_id==\"70/2011\").current_value", "matched": true, "result": ""}, + {"expr": "slots.#(instance_id==5)#.address", "matched": true, "result": ["5.seed", "5.steps"]}, + {"expr": "slots.#(instance_id==\"5\")#.address", "matched": true, "result": ["5.seed", "5.steps"]}, + {"expr": "slots.#(instance_id!=5)#.address", "matched": true, "result": ["70/2011.text", "70/2012.text", "9.text"]}, + {"expr": "slots.#(current_value>=20)#.address", "matched": true, "result": ["5.steps"]}, + {"expr": "slots.#(current_value<20)#.address", "matched": true, "result": ["5.seed"]}, + {"expr": "slots.#(address%\"70/*\")#.current_value", "matched": true, "result": ["", "a cat, 4k"]}, + {"expr": "slots.#(address!%\"70/*\")#.address", "matched": true, "result": ["5.seed", "5.steps", "9.text"]}, + {"expr": "slots.#(current_value==\"a.b,c\").address", "matched": true, "result": "9.text"}, + {"expr": "slots.#(node_type==\"KSampler\")#.address,count", "matched": true, "result": {"slots.#(node_type==\"KSampler\")#.address": ["5.seed", "5.steps"], "count": 3}}, + {"expr": "inputs.#(name==\"lora_name\").choices.#(%\"*o?in*\")#", "matched": true, "result": ["MoXinV1.safetensors", "moxin_v2.safetensors"]}, + {"expr": "inputs.#(name==\"lora_name\").choices.#(==\"add_detail.safetensors\")", "matched": true, "result": "add_detail.safetensors"}, + {"expr": "inputs.#(name==\"lora_name\").choices.#(%\"MoXin?1*\")#", "matched": true, "result": ["MoXinV1.safetensors"]}, + {"expr": "rows.#(tags)#.name", "matched": true, "result": ["a", "b"]}, + {"expr": "rows.#(nameless==1)", "matched": true, "result": {"nameless": 1}}, + {"expr": "meta.#(k==\"v\")", "matched": false}, + {"expr": "slots.#(node_type==\"KSampler\"", "matched": false}, + {"expr": "slots.#(node_type=~\"K\")#", "matched": false}, + {"expr": "slots.#()#", "matched": false} + ] +} From b0f7362a24849b3e2dceb0ca11398e2df66b56d0 Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:27:58 -0700 Subject: [PATCH 02/10] wip: print renders broken links marked instead of refusing Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/error_codes.py | 6 +- comfy_cli/workflow_print.py | 94 +++++++++++++++---- .../command/test_workflow_print_cmd.py | 11 ++- tests/comfy_cli/test_workflow_print.py | 72 +++++++++++--- 4 files changed, 147 insertions(+), 36 deletions(-) diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index 2e3966db7..e2ff964de 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -638,8 +638,10 @@ class ErrorCode: ErrorCode( "workflow_print_unsupported", "`comfy workflow print` refused: the workflow contains something it cannot render faithfully " - "(legacy group node, duplicate node id, link to a missing node or output slot, non-integer link slot, " - "link cycle, unknown `--format`). `details.reasons` lists every reason.", + "(legacy group node, duplicate node id, link cycle, unknown `--format`). `details.reasons` lists every " + "reason. A broken link (missing endpoint node, missing output or input slot, non-integer slot) is not a " + "refusal: the graph prints with that input as `None`, the line marked `BROKEN`, and a warning naming the " + "`connect` that repairs it.", "fix the listed reasons, or read the graph with `comfy workflow slots` / `comfy workflow ls-nodes`", ), ErrorCode( diff --git a/comfy_cli/workflow_print.py b/comfy_cli/workflow_print.py index b33897286..1a5676866 100644 --- a/comfy_cli/workflow_print.py +++ b/comfy_cli/workflow_print.py @@ -164,6 +164,10 @@ def _validate(nodes: list[dict], links: list[list]) -> list[str]: Collects every problem found (rather than stopping at the first) so the caller's ``PrintUnsupported`` can report them all at once. + A broken LINK is not one of them: it is rendered, marked, and reported + (see ``_broken_links`` and ``_stale_input_slot_links``) — one bad link row + used to hide the whole graph, the shape a caller most needs to read. + A subgraph instance whose definition is missing is deliberately NOT a refusal here (at any depth): it's printed opaquely instead — see ``_render_missing_subgraph_line`` and D11 in the task brief (amended). @@ -190,31 +194,62 @@ def _validate(nodes: list[dict], links: list[list]) -> list[str]: else: seen_ids.add(nid) + return reasons + + +def _broken_links(nodes: list[dict], links: list[Any], qualify: Any = str) -> tuple[list[str], dict[str, str], list]: + """Link rows that cannot carry a value, as ``(warnings, broken, rest)``. + + ``broken`` maps a link id to the short reason the consuming input's line + is marked with; ``rest`` is every other row (well-formed, both endpoints + present), for the stale-input check and the link map. A broken link is + rendered as ``None`` on the input that holds it, marked ``BROKEN`` on that + node's line, and reported with the edit that repairs it — never a refusal + of the whole print. + + Broken means: a malformed row, an endpoint node that does not exist, a + non-integer slot, or an output slot the source node does not have. + """ + warnings: list[str] = [] + broken: dict[str, str] = {} + rest: list = [] nodes_by_id = {str(n.get("id")): n for n in nodes} + + def holder_of(link_id: Any, tgt: dict | None) -> str | None: + for inp in (tgt or {}).get("inputs") or []: + if isinstance(inp, dict) and inp.get("link") is not None and str(inp["link"]) == str(link_id): + return str(inp.get("name") or "") + return None + for link in links: if not isinstance(link, list) or len(link) < 5: - reasons.append(f"link malformed: {link!r}") + warnings.append(f"ignoring malformed link row {link!r}") continue link_id, src_id, src_slot, tgt_id, tgt_slot = link[0], link[1], link[2], link[3], link[4] src_node = nodes_by_id.get(str(src_id)) - if src_node is None: - reasons.append(f"link {link_id} references missing node {src_id}") - continue tgt_node = nodes_by_id.get(str(tgt_id)) if tgt_node is None: - reasons.append(f"link {link_id} references missing node {tgt_id}") + warnings.append(f"link {link_id} targets missing node {qualify(tgt_id)}; it feeds nothing and was ignored") continue - # Slots index into outputs/inputs below and into widgets_values later; - # a None or "0" would raise a TypeError deep in the render instead of - # being reported here with every other structural problem. - if not _is_slot_index(src_slot) or not _is_slot_index(tgt_slot): - reasons.append(f"link {link_id} has a non-integer slot") + why = None + if src_node is None: + why = f"its source node {src_id} does not exist" + elif not _is_slot_index(src_slot) or not _is_slot_index(tgt_slot): + why = "it has a non-integer slot" + else: + outputs = src_node.get("outputs") + if isinstance(outputs, list) and not (0 <= src_slot < len(outputs)): + why = f"node {src_id} has no output slot {src_slot} (it has {len(outputs)})" + if why is None: + rest.append(link) continue - outputs = src_node.get("outputs") - if isinstance(outputs, list) and not (0 <= src_slot < len(outputs)): - reasons.append(f"link {link_id} references out-of-range output slot {src_slot} on node {src_id}") - # An out-of-range INPUT slot is not a refusal: see _stale_input_slot_links. - return reasons + broken[str(link_id)] = why + name = holder_of(link_id, tgt_node) + into = f"input {name!r} of node {qualify(tgt_id)}" if name is not None else f"node {qualify(tgt_id)}" + source = f"{qualify(src_id)}." if src_node is not None else "." + fix = f"`connect {source} {qualify(tgt_id)}.{name}`" if name else "`connect` to the input it was meant for" + warnings.append(f"BROKEN link {link_id} into {into}: {why}; printed as None — re-wire it with {fix}") + return warnings, broken, rest def _stale_input_slot_links(nodes: list[dict], links: list[list], qualify: Any = str) -> tuple[list[str], set[str]]: @@ -265,7 +300,18 @@ def _stale_input_slot_links(nodes: list[dict], links: list[list], qualify: Any = warnings.append(f"{where}; rendered through input {str(holder.get('name') or '')!r}, which holds it") else: ignored.add(str(link_id)) - warnings.append(f"{where}; no input holds it, so it feeds nothing and was ignored") + src = link[1] + source = "the subgraph input" if str(src) == _PROXY_IN else f"node {qualify(src)} output {link[2]}" + fixed = f"{qualify(src)}.{link[2]}" if str(src) != _PROXY_IN else None + warnings.append( + f"{where}; no input holds it, so it feeds nothing and was ignored. It was wired from {source} — " + + ( + f"if that value was meant for node {qualify(tgt_id)}, re-wire it with " + f"`connect {fixed} {qualify(tgt_id)}.` rather than retyping the value" + if fixed + else "re-wire it to the input it was meant for" + ) + ) return warnings, ignored @@ -571,6 +617,9 @@ class _RenderCtx: # level). A nested subgraph instance registers against it so its own # address can later be expanded through every instance of THIS definition. owner_def: str | None = None + # Link id -> why it carries no value (see ``_broken_links``): the input + # holding it prints ``None`` and the line is marked ``BROKEN``. + broken_links: dict[str, str] = field(default_factory=dict) def proxy_in_name(self, slot: Any) -> str: name = self.proxy_in_names.get(slot) @@ -773,6 +822,8 @@ def _build_args( continue link = ctx.link_map.get(str(link_id)) if link is None: + if str(link_id) in ctx.broken_links: + annotations.append(f" {name} BROKEN link {link_id}: {ctx.broken_links[str(link_id)]}") continue src_id, src_slot, _tgt_id, _tgt_slot = link outcome = _resolve_source( @@ -814,6 +865,8 @@ def _build_args( continue link = ctx.link_map.get(str(link_id)) if link is None: + if str(link_id) in ctx.broken_links: + annotations.append(f" {name} BROKEN link {link_id}: {ctx.broken_links[str(link_id)]}") _place_arg(name, "None", args, extra) continue src_id, src_slot, _tgt_id, _tgt_slot = link @@ -1308,6 +1361,11 @@ def _render_definition_block( if reasons: raise PrintUnsupported(reasons) first_instance = state.first_instance_by_def.get(def_id, def_id) + broken_warnings, broken, _rest = _broken_links( + interior_nodes, validate_links, lambda nid: f"{first_instance}/{nid}" + ) + state.warnings.extend(broken_warnings) + all_links = {lid: link for lid, link in all_links.items() if lid not in broken} # Every link into an interior node, including one from the ``-10`` input # proxy (which ``_validate`` does not see): a stale target slot is stale # whatever feeds it. @@ -1350,6 +1408,7 @@ def _render_definition_block( proxy_in_names=proxy_in_names, addr_prefix=first_instance, owner_def=def_id, + broken_links=broken, ) lines = _render_nodes(order, ctx, graph, state.defs_by_id, binding_by_id, state, depth + 1) @@ -1426,6 +1485,8 @@ def render_py(workflow: dict, graph: Graph | None) -> PrintResult: reasons = _validate(nodes, links) if reasons: raise PrintUnsupported(reasons) + broken_warnings, broken, links = _broken_links(nodes, links) + warnings.extend(broken_warnings) stale_warnings, stale_ids = _stale_input_slot_links(nodes, links) warnings.extend(stale_warnings) @@ -1463,6 +1524,7 @@ def render_py(workflow: dict, graph: Graph | None) -> PrintResult: reroute_sources=reroute_sources, set_sources=set_sources, get_vars=get_vars, + broken_links=broken, ) skipped: list[dict] = [] diff --git a/tests/comfy_cli/command/test_workflow_print_cmd.py b/tests/comfy_cli/command/test_workflow_print_cmd.py index fa5a2b593..3aaf30490 100644 --- a/tests/comfy_cli/command/test_workflow_print_cmd.py +++ b/tests/comfy_cli/command/test_workflow_print_cmd.py @@ -173,10 +173,9 @@ def test_print_unsupported_lists_reasons(tmp_path, capsys): } env = _run(["print", str(_write_workflow(tmp_path, wf)), "--input", str(SD15_OI)], capsys, expect_ok=False) assert env["error"]["code"] == "workflow_print_unsupported" - assert env["error"]["details"]["reasons"] == [ - "node 1 is a legacy group node (workflow>Grp)", - "link 7 references missing node 99", - ] + # A broken link is rendered and marked, never a refusal; the legacy group + # node still is. + assert env["error"]["details"]["reasons"] == ["node 1 is a legacy group node (workflow>Grp)"] def test_print_rejects_unknown_format(capsys): @@ -232,5 +231,7 @@ def test_print_renders_a_stale_input_slot_link_and_reports_it(tmp_path, capsys): assert d["node_count"] == 2 assert "samples=empty_latent_image" in d["source"] assert d["warnings"] == [ - "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was ignored" + "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was " + "ignored. It was wired from node 1 output 0 — if that value was meant for node 2, re-wire it with " + "`connect 1.0 2.` rather than retyping the value" ] diff --git a/tests/comfy_cli/test_workflow_print.py b/tests/comfy_cli/test_workflow_print.py index 1f1ef123d..6c58cf5ea 100644 --- a/tests/comfy_cli/test_workflow_print.py +++ b/tests/comfy_cli/test_workflow_print.py @@ -262,7 +262,7 @@ def test_cycle_is_refused(sd15_graph): assert e.value.reasons == ["link cycle among nodes 1, 2"] -def test_dangling_link_is_refused(sd15_graph): +def test_dangling_link_renders_marked_instead_of_refusing(sd15_graph): wf = _mini( [ _node( @@ -273,9 +273,47 @@ def test_dangling_link_is_refused(sd15_graph): ], [[1, 99, 0, 2, 0, "LATENT"]], ) - with pytest.raises(PrintUnsupported) as e: - render_py(wf, sd15_graph) - assert e.value.reasons == ["link 1 references missing node 99"] + res = render_py(wf, sd15_graph) + assert ( + "VAEDecode(samples=None, vae=None) # 2 samples BROKEN link 1: its source node 99 does not exist" in res.source + ) + assert res.warnings == [ + "BROKEN link 1 into input 'samples' of node 2: its source node 99 does not exist; printed as None — " + "re-wire it with `connect . 2.samples`" + ] + + +def test_out_of_range_output_slot_renders_marked_instead_of_refusing(sd15_graph): + """The eff-oob-edit shape: three link rows from an output slot their + source does not have used to refuse the whole print.""" + wf = _stale_slot_workflow([1, 1, 0, 2, 0, "LATENT"]) + wf["links"] = [[1, 1, 3, 2, 0, "LATENT"]] + res = render_py(wf, sd15_graph) + assert res.node_count == 2 + assert ( + "VAEDecode(samples=None, vae=None) # 2 samples BROKEN link 1: node 1 has no output slot 3 (it has 1)" + in res.source + ) + assert res.warnings == [ + "BROKEN link 1 into input 'samples' of node 2: node 1 has no output slot 3 (it has 1); printed as None — " + "re-wire it with `connect 1. 2.samples`" + ] + + +def test_out_of_range_output_slot_inside_a_definition_is_marked_and_qualified(sd15_graph): + wf = json.loads((FIXTURES / "subgraph_template_ui.json").read_text()) + graph = Graph.from_object_info(json.loads((FIXTURES / "subgraph_object_info.json").read_text())) + sg = next(s for s in wf["definitions"]["subgraphs"] if s["id"] == "d33c1791-dfd2-4102-8540-aa63e4434cd2") + link = next( + lk for lk in sg["links"] if str(lk["origin_id"]) not in ("-10", "-20") and str(lk["target_id"]) != "-20" + ) + link["origin_slot"] = 9 + res = render_py(wf, graph) + assert any( + w.startswith(f"BROKEN link {link['id']} into input ") and f"of node 10/{link['target_id']}:" in w + for w in res.warnings + ), res.warnings + assert f"BROKEN link {link['id']}: node {link['origin_id']} has no output slot 9" in res.source def test_legacy_group_node_is_refused(sd15_graph): @@ -730,7 +768,7 @@ def test_non_identifier_proxy_names_use_subscripts(sd15_graph): @pytest.mark.parametrize("slot", [None, "0"]) -def test_non_integer_link_slot_is_refused_not_raised(sd15_graph, slot): +def test_non_integer_link_slot_renders_marked_not_raised(sd15_graph, slot): wf = _mini( [ _node( @@ -744,9 +782,8 @@ def test_non_integer_link_slot_is_refused_not_raised(sd15_graph, slot): ], [[7, 1, slot, 2, 0, "LATENT"]], ) - with pytest.raises(PrintUnsupported) as e: - render_py(wf, sd15_graph) - assert e.value.reasons == ["link 7 has a non-integer slot"] + res = render_py(wf, sd15_graph) + assert "samples BROKEN link 7: it has a non-integer slot" in res.source def test_malformed_input_entry_is_skipped_with_warning(sd15_graph): @@ -1014,7 +1051,9 @@ def test_out_of_range_input_slot_renders_and_warns(sd15_graph): assert "samples=empty_latent_image" in res.source assert "vae=None" in res.source assert res.warnings == [ - "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was ignored" + "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was " + "ignored. It was wired from node 1 output 0 — if that value was meant for node 2, re-wire it with " + "`connect 1.0 2.` rather than retyping the value" ] @@ -1024,7 +1063,9 @@ def test_out_of_range_input_slot_adds_no_ordering_edge(sd15_graph): res = render_py(wf, sd15_graph) assert [ln.rsplit("# ", 1)[1].split()[0] for ln in res.source.splitlines()] == ["1", "2"] assert res.warnings == [ - "link 7 targets input slot 6 on node 1, which has 0 inputs; no input holds it, so it feeds nothing and was ignored" + "link 7 targets input slot 6 on node 1, which has 0 inputs; no input holds it, so it feeds nothing and was " + "ignored. It was wired from node 2 output 0 — if that value was meant for node 1, re-wire it with " + "`connect 2.0 1.` rather than retyping the value" ] @@ -1054,7 +1095,9 @@ def test_out_of_range_input_slot_inside_a_definition_is_qualified(sd15_graph): n_inputs = len(tgt.get("inputs") or []) assert ( f"link 9999 targets input slot 42 on node 10/{tgt['id']}, which has {n_inputs} inputs; " - "no input holds it, so it feeds nothing and was ignored" + f"no input holds it, so it feeds nothing and was ignored. It was wired from node 10/{src['id']} output 0 — " + f"if that value was meant for node 10/{tgt['id']}, re-wire it with `connect 10/{src['id']}.0 " + f"10/{tgt['id']}.` rather than retyping the value" ) in res.warnings @@ -1064,7 +1107,9 @@ def test_out_of_range_input_slot_on_a_node_without_inputs_is_reported(sd15_graph res = render_py(wf, sd15_graph) assert [ln.rsplit("# ", 1)[1].split()[0] for ln in res.source.splitlines()] == ["1", "2"] assert res.warnings == [ - "link 7 targets input slot 0 on node 1, which has 0 inputs; no input holds it, so it feeds nothing and was ignored" + "link 7 targets input slot 0 on node 1, which has 0 inputs; no input holds it, so it feeds nothing and was " + "ignored. It was wired from node 2 output 0 — if that value was meant for node 1, re-wire it with " + "`connect 2.0 1.` rather than retyping the value" ] @@ -1080,5 +1125,6 @@ def test_out_of_range_input_slot_fed_by_the_definition_input_proxy_is_reported(s n_inputs = len(tgt.get("inputs") or []) assert ( f"link 9998 targets input slot 42 on node 10/{tgt['id']}, which has {n_inputs} inputs; " - "no input holds it, so it feeds nothing and was ignored" + "no input holds it, so it feeds nothing and was ignored. It was wired from the subgraph input — " + "re-wire it to the input it was meant for" ) in res.warnings From de0baea3272c393073483142adef564fd4223f63 Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:33:27 -0700 Subject: [PATCH 03/10] wip: bound tests, dyn-combo hint Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/cql/engine.py | 16 +- tests/comfy_cli/cql/test_engine.py | 12 +- tests/comfy_cli/test_agent_output_bounds.py | 157 ++++++++++++++++++++ 3 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 tests/comfy_cli/test_agent_output_bounds.py diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index 655947e61..ccc78c27f 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -125,7 +125,9 @@ def enum_option_fields(options: list, closest: list | None = None) -> dict[str, return out -def enum_listing_hint(field: str, options: list, suggestions: list, class_type: str | None = None) -> str: +def enum_listing_hint( + field: str, options: list, suggestions: list, class_type: str | None = None, *, listing: str = "choices" +) -> str: """The hint of an enum finding. A short list is named whole; a long one is pointed at — the closest options are already in ``suggestions`` — with the way to filter the rest instead of dumping it.""" @@ -133,9 +135,9 @@ def enum_listing_hint(field: str, options: list, suggestions: list, class_type: if n <= ENUM_INLINE_MAX or _FULL_ENUM_OPTIONS.get(): return f"valid options: {', '.join(str(v) for v in options)}" target = class_type or "" - query = f'inputs.#(name=="{field}").choices.#(%"**")#' + query = f'inputs.#(name=="{field}").{listing}.#(%"**")#' return ( - f"pick one of `suggestions` (the closest of {n} options); to search all {n}, filter them with " + f"pick one of `suggestions` ({len(suggestions)} of {n} options); to search all {n}, filter them with " f"`comfy nodes show {target} --select '{query}'`, or re-run validate with --full-options" ) @@ -2906,7 +2908,7 @@ def _check_dynamic_combo_input( # couldn't be read instead of dangling an empty enumeration. hint = ( f"set {name!r} to one of its options — " - + enum_listing_hint(name, keys, keys[:ENUM_SUGGEST_MAX], class_type) + + enum_listing_hint(name, keys, keys[:ENUM_SUGGEST_MAX], class_type, listing="selection_keys") if keys else f"{name!r} is required, but its option schema didn't parse — check object_info for this node" ) @@ -2973,7 +2975,11 @@ def _check_dynamic_combo_input( f"cannot be resolved, so this node fails at execution" ), "hint": enum_listing_hint( - name, keys, _closest_options(selected, keys) or keys[:ENUM_SUGGEST_MAX], class_type + name, + keys, + _closest_options(selected, keys) or keys[:ENUM_SUGGEST_MAX], + class_type, + listing="selection_keys", ), **enum_option_fields(keys, _closest_options(selected, keys)), } diff --git a/tests/comfy_cli/cql/test_engine.py b/tests/comfy_cli/cql/test_engine.py index 3f1f54f7b..aece37446 100644 --- a/tests/comfy_cli/cql/test_engine.py +++ b/tests/comfy_cli/cql/test_engine.py @@ -3404,7 +3404,8 @@ def test_deep_stray_key_attributed_to_nested_level(self, graph_dynamic: Graph): def test_required_missing_hint_truncates_many_selection_keys(self): """A dynamic combo with hundreds of options must not dump them all - into the required_input_missing hint — first 8, then a count.""" + into the required_input_missing error — the first few, a count, and + the query that lists the rest.""" options = [{"key": f"ckpt-{i:03d}", "inputs": {"required": {}, "optional": {}}} for i in range(30)] info = { "Loader": { @@ -3421,9 +3422,12 @@ def test_required_missing_hint_truncates_many_selection_keys(self): result = g.validate_workflow({"1": {"class_type": "Loader", "inputs": {}}}) err = next(e for e in result["errors"] if e["code"] == "required_input_missing") hint = err["hint"] - assert "ckpt-007" in hint - assert "ckpt-008" not in hint - assert "and 22 more" in hint + assert err["suggestions"] == ["ckpt-000", "ckpt-001", "ckpt-002", "ckpt-003", "ckpt-004"] + assert err["option_count"] == 30 + assert "valid_options" not in err + assert "ckpt-005" not in json.dumps(err) + assert "of 30 options" in hint + assert 'inputs.#(name=="model").selection_keys' in hint def test_stale_sub_key_warning_survives_on_unreachable_node(self): """dyn_errors/dyn_warnings used to be bundled into one reachability diff --git a/tests/comfy_cli/test_agent_output_bounds.py b/tests/comfy_cli/test_agent_output_bounds.py new file mode 100644 index 000000000..cb975b966 --- /dev/null +++ b/tests/comfy_cli/test_agent_output_bounds.py @@ -0,0 +1,157 @@ +"""Bounds on the agent-facing payloads that carried whole option lists. + +Production (Langfuse, comfy-cloud-prod): one `validate` of a graph with eleven +unknown model filenames over a 377-file folder returned ~340K tokens, because +every `unknown_enum_value` error carried the folder's full listing twice +(`suggestions` and `valid_options`); `nodes show LoraLoader` was ~31KB of LoRA +filenames on every call. These tests pin the bound and the way back to the +full list. +""" + +from __future__ import annotations + +import json +from typing import Any + +import pytest +from typer.testing import CliRunner + +from comfy_cli.caller import Caller +from comfy_cli.command import nodes as nodes_cmd +from comfy_cli.cql.engine import ENUM_INLINE_MAX, ENUM_SUGGEST_MAX, Graph, full_enum_options +from comfy_cli.output.renderer import OutputMode, Renderer, reset_renderer_for_testing, set_renderer + +FILES = [f"model_{i:03d}_v1.safetensors" for i in range(377)] + + +def _object_info() -> dict[str, Any]: + return { + "CheckpointLoaderSimple": { + "input": {"required": {"ckpt_name": [FILES]}}, + "input_order": {"required": ["ckpt_name"]}, + "output": ["MODEL", "CLIP", "VAE"], + "output_name": ["MODEL", "CLIP", "VAE"], + "category": "loaders", + "display_name": "Load Checkpoint", + "output_node": False, + "python_module": "nodes", + }, + "SaveLatent": { + "input": {"required": {"model": ["MODEL"], "mode": [["a", "b", "c"]]}}, + "input_order": {"required": ["model", "mode"]}, + "output": [], + "output_name": [], + "category": "latent", + "display_name": "Save", + "output_node": True, + "python_module": "nodes", + }, + } + + +def _workflow(n_bad: int = 11) -> dict[str, Any]: + wf: dict[str, Any] = {} + for i in range(n_bad): + wf[str(i * 2 + 1)] = { + "class_type": "CheckpointLoaderSimple", + "inputs": {"ckpt_name": f"model_{i:03d}_v2.safetensors"}, + } + wf[str(i * 2 + 2)] = {"class_type": "SaveLatent", "inputs": {"model": [str(i * 2 + 1), 0], "mode": "a"}} + return wf + + +class TestValidateEnumErrors: + def test_an_unknown_enum_error_names_the_closest_options_not_the_folder(self): + result = Graph.from_object_info(_object_info()).validate_workflow(_workflow()) + errors = [e for e in result["errors"] if e["code"] == "unknown_enum_value"] + assert len(errors) == 11 + err = errors[0] + assert err["option_count"] == len(FILES) + assert "valid_options" not in err + assert len(err["suggestions"]) <= ENUM_SUGGEST_MAX + assert err["suggestions"][0] == "model_000_v1.safetensors" + assert err["options_omitted"] == len(FILES) - len(err["suggestions"]) + assert "--full-options" in err["hint"] and "choices.#(%" in err["hint"] + # Eleven bad filenames: ~0.8KB each (was ~22KB each, the folder twice). + assert len(json.dumps(result["errors"])) < 11 * 1_000 + + def test_full_options_restores_the_whole_typed_list(self): + with full_enum_options(): + result = Graph.from_object_info(_object_info()).validate_workflow(_workflow(1)) + err = next(e for e in result["errors"] if e["code"] == "unknown_enum_value") + assert err["valid_options"] == FILES + + def test_a_short_option_list_is_still_carried_whole(self): + wf = {"1": {"class_type": "SaveLatent", "inputs": {"model": None, "mode": "zz"}}} + result = Graph.from_object_info(_object_info()).validate_workflow(wf) + err = next(e for e in result["errors"] if e["code"] == "unknown_enum_value") + assert err["valid_options"] == ["a", "b", "c"] + assert len(["a", "b", "c"]) <= ENUM_INLINE_MAX + + def test_the_edit_finding_is_bounded_too(self): + port = Graph.from_object_info(_object_info()).node("CheckpointLoaderSimple").inputs[0] + (finding,) = port.validate_catalog("model_007_v2.safetensors") + assert finding["code"] == "unknown_enum_value" + assert "valid_options" not in finding + assert finding["option_count"] == len(FILES) + assert finding["did_you_mean"][0] == "model_007_v1.safetensors" + + +@pytest.fixture(autouse=True) +def _renderer(): + reset_renderer_for_testing() + yield + reset_renderer_for_testing() + + +def _run(args: list[str], capsys, monkeypatch) -> dict[str, Any]: + monkeypatch.setattr(nodes_cmd, "_get_graph", lambda *a, **kw: Graph.from_object_info(_object_info())) + r = Renderer.resolve( + is_stdout_tty=False, env={}, caller=Caller(kind="user", agentic=False, source_env=None), json_flag=True + ) + r.mode = OutputMode.JSON + set_renderer(r) + result = CliRunner().invoke(nodes_cmd.app, args, standalone_mode=False) + out = capsys.readouterr().out or result.stdout or "" + for line in reversed(out.strip().splitlines()): + try: + return json.loads(line) + except json.JSONDecodeError: + continue + raise AssertionError(f"no envelope: {out[:400]} {result.exception}") + + +class TestNodesShowChoices: + def test_show_caps_a_long_choice_list_with_a_count(self, capsys, monkeypatch): + data = _run(["show", "CheckpointLoaderSimple"], capsys, monkeypatch)["data"] + ckpt = data["inputs"][0] + assert len(ckpt["choices"]) == nodes_cmd.CHOICES_INLINE_MAX + assert ckpt["choices_total"] == len(FILES) + assert ckpt["choices_truncated"] is True + assert "--all-choices" in data["choices_note"] + assert len(json.dumps(data)) < 4_000 + + def test_all_choices_lists_every_choice(self, capsys, monkeypatch): + data = _run(["show", "CheckpointLoaderSimple", "--all-choices"], capsys, monkeypatch)["data"] + assert data["inputs"][0]["choices"] == FILES + assert "choices_note" not in data + + def test_a_select_projects_the_full_schema_and_can_filter_it(self, capsys, monkeypatch): + env = _run( + ["show", "CheckpointLoaderSimple", "--select", 'inputs.#(name=="ckpt_name").choices.#(%"model_37?_*")#'], + capsys, + monkeypatch, + ) + assert env["data"] == [f"model_{i}_v1.safetensors" for i in range(370, 377)] + + def test_a_short_choice_list_is_untouched(self, capsys, monkeypatch): + data = _run(["show", "SaveLatent"], capsys, monkeypatch)["data"] + mode = next(i for i in data["inputs"] if i["name"] == "mode") + assert mode["choices"] == ["a", "b", "c"] + assert "choices_total" not in mode and "choices_note" not in data + + def test_search_expand_top_caps_choices(self, capsys, monkeypatch): + data = _run(["search", "Checkpoint", "--expand-top", "1"], capsys, monkeypatch)["data"] + entry = data["expanded"][0] + assert entry["inputs"][0]["choices_total"] == len(FILES) + assert len(entry["inputs"][0]["choices"]) == nodes_cmd.CHOICES_INLINE_MAX From bc4a2d2023c5ab011b8cfdc9a4aed3508502c084 Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:40:32 -0700 Subject: [PATCH 04/10] wip: connect wires two nodes inside one subgraph definition; print calls a re-wired stale row a leftover Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/workflow_ops.py | 207 ++++++++++++++++- comfy_cli/workflow_print.py | 23 +- .../command/test_connect_interior.py | 215 ++++++++++++++++++ .../command/test_workflow_print_cmd.py | 5 +- tests/comfy_cli/test_workflow_print.py | 9 +- 5 files changed, 450 insertions(+), 9 deletions(-) create mode 100644 tests/comfy_cli/command/test_connect_interior.py diff --git a/comfy_cli/workflow_ops.py b/comfy_cli/workflow_ops.py index 4164386dc..4ca91e0f0 100644 --- a/comfy_cli/workflow_ops.py +++ b/comfy_cli/workflow_ops.py @@ -1205,7 +1205,8 @@ def _subgraph_boundary_error(workflow: dict, node_id: Any) -> ValueError | None: ) return ValueError( f"node {canonical} is inside subgraph {segments[0]} ({str(sg.get('name') or '?')!r}) — a link cannot cross " - f"the subgraph boundary, so connect cannot reach it. Interior widgets ARE settable: " + f"the subgraph boundary, so connect cannot reach it from outside. Two nodes inside the same subgraph CAN be " + f"wired (`connect {segments[0]}/. {segments[0]}/.`). Interior widgets ARE settable: " f"`comfy workflow set-widget {canonical}. `. To wire a live link, connect to one of " f"the instance's own slots (see `comfy workflow slots`), or promote the input in the ComfyUI editor first." ) @@ -1359,6 +1360,9 @@ def _connect_impl( actor: str = "cli", base_version: int = 0, ) -> tuple[dict, dict]: + interior = _interior_link_scope(workflow, from_node, to_node) + if interior is not None: + return _connect_interior(workflow, graph, interior, from_slot, to_slot, actor=actor, base_version=base_version) for endpoint in (from_node, to_node): boundary = _subgraph_boundary_error(workflow, endpoint) if boundary is not None: @@ -1400,6 +1404,199 @@ def _connect_impl( return apply_op(workflow, op, graph), op +def _interior_link_scope(workflow: dict, from_node: Any, to_node: Any) -> dict | None: + """The definition both endpoints of a connect live in, when they are two + interior nodes of ONE subgraph instance (``70/2005`` -> ``70/2011``). + + Returns ``{"path", "definition", "src", "dst"}`` — ``path`` is the + instance path (``["70"]``, or ``["70", "5"]`` nested) — or ``None`` when + the endpoints are not both interior to the same instance, so the caller's + top-level and boundary handling applies unchanged. Such a link crosses no + boundary: it lives in the definition's own ``links``. + + Raises when the definition is shared by more than one instance: a link + written into a shared definition rewires every instance at once, which the + doc host refuses (``rejectSharedInteriorDefinition``), so the CLI refuses + it the same way rather than minting an op no replica will apply. + """ + from comfy_cli.cql import engine as _engine + + ends = [] + for endpoint in (from_node, to_node): + text = str(endpoint) + if _find_by_str(workflow, text) is not None: + return None # a literal top-level node id + if _engine._SUBGRAPH_PATH_SEP in text: + segments = _engine.split_node_path(workflow, text) + elif ":" in text: + segments = text.split(":") + else: + return None + if len(segments) < 2: + return None + ends.append(segments) + (src_path, dst_path) = ends + if src_path[:-1] != dst_path[:-1]: + return None + path = src_path[:-1] + try: + host = _navigate_subgraph_path(workflow, path) + except ValueError: + return None + defs_by_id = _engine._subgraph_defs_by_id(workflow) + definition = defs_by_id.get(str(host.get("type", ""))) + if definition is None: + return None + nodes = {str(n.get("id")): n for n in definition.get("nodes") or [] if isinstance(n, dict)} + src, dst = nodes.get(str(src_path[-1])), nodes.get(str(dst_path[-1])) + if src is None or dst is None: + return None # the boundary error names what the definition holds + instances = _definition_instance_count(workflow, str(definition.get("id"))) + if instances > 1: + raise ValueError( + f"subgraph {'/'.join(path)} shares its definition {definition.get('id')} with " + f"{instances} instances, so a link inside it would rewire all {instances} at once — " + "connect refuses that; edit a single-instance copy (unpack or duplicate the subgraph in the editor)" + ) + return {"path": [str(seg) for seg in path], "definition": definition, "src": src, "dst": dst} + + +def _definition_instance_count(workflow: dict, def_id: str) -> int: + """How many nodes, top level or inside any definition, instantiate ``def_id``.""" + from comfy_cli.cql import engine as _engine + + count = sum(1 for n in workflow.get("nodes") or [] if isinstance(n, dict) and str(n.get("type")) == def_id) + for sg in _engine._subgraph_defs_by_id(workflow).values(): + count += sum(1 for n in sg.get("nodes") or [] if isinstance(n, dict) and str(n.get("type")) == def_id) + return count + + +def _connect_interior( + workflow: dict, graph, scope: dict, from_slot: Any, to_slot: Any, *, actor: str, base_version: int +) -> tuple[dict, dict]: + """Wire two interior nodes of one single-instance definition. Concrete + slots only: the doc host's interior connect has no grow form, so a widget + that is not already listed as an input is refused with the reason.""" + src, dst = scope["src"], scope["dst"] + out_idx, link_type = _resolve_output_slot(src, graph, from_slot) + in_idx, grow = _resolve_input_target(dst, graph, to_slot, link_type) + if grow is not None or in_idx is None: + raise ValueError( + f"input {to_slot!r} of interior node {'/'.join(scope['path'])}/{dst.get('id')} is not an input slot " + "the node lists; inside a subgraph connect can only wire an existing input" + ) + dst_type = (dst.get("inputs") or [])[in_idx].get("type") + if not _types_compatible(link_type, dst_type): + raise ValueError( + f"type mismatch: {link_type} output of node {'/'.join(scope['path'])}/{src.get('id')} cannot connect " + f"to {dst_type} input {(dst.get('inputs') or [])[in_idx].get('name')!r} of node " + f"{'/'.join(scope['path'])}/{dst.get('id')}" + ) + op = _new_op( + "connect", + actor, + base_version, + path=list(scope["path"]), + link_id=mint_id(), + from_node=src.get("id"), + from_slot=out_idx, + to_node=dst.get("id"), + to_slot=in_idx, + link_type=link_type, + ) + return apply_op(workflow, op, graph), op + + +def _apply_interior_connect(workflow: dict, op: dict) -> None: + """Apply a ``connect`` carrying an instance ``path``: the link lives in the + instance's definition. Same totality and LWW register as the top-level + concrete branch, scoped to the definition (comfy-multi-player + ``applyInteriorConnect``).""" + from comfy_cli.cql import engine as _engine + + try: + host = _navigate_subgraph_path(workflow, [str(seg) for seg in op["path"]]) + except ValueError: + return # instance concurrently deleted => delete wins + definition = _engine._subgraph_defs_by_id(workflow).get(str(host.get("type", ""))) + if definition is None: + return + nodes = {str(n.get("id")): n for n in definition.get("nodes") or [] if isinstance(n, dict)} + dst = nodes.get(str(op["to_node"])) + if dst is None: + return + to_idx = op["to_slot"] + ins = dst.get("inputs") + if ( + not isinstance(ins, list) + or not isinstance(to_idx, int) + or isinstance(to_idx, bool) + or not 0 <= to_idx < len(ins) + or not isinstance(ins[to_idx], dict) + ): + return + if not _lww_gate(workflow, op): + return + _lww_commit(workflow, op) + if not isinstance(definition.get("links"), list): + definition["links"] = [] + prev = ins[to_idx].get("link") + if prev is not None and prev != op["link_id"]: + _remove_interior_link(definition, prev) + links = definition["links"] # read after the removal, which rebuilds the list + src = nodes.get(str(op["from_node"])) + outs = (src or {}).get("outputs") + from_slot = op["from_slot"] + if ( + src is None + or not isinstance(outs, list) + or not isinstance(from_slot, int) + or isinstance(from_slot, bool) + or not 0 <= from_slot < len(outs) + or not isinstance(outs[from_slot], dict) + ): + return + if not any(_interior_link_id(lk) == op["link_id"] for lk in links): + links.append( + { + "id": op["link_id"], + "origin_id": op["from_node"], + "origin_slot": from_slot, + "target_id": op["to_node"], + "target_slot": to_idx, + "type": op["link_type"], + } + ) + ins[to_idx]["link"] = op["link_id"] + if outs[from_slot].get("links") is None: + outs[from_slot]["links"] = [] + if op["link_id"] not in outs[from_slot]["links"]: + outs[from_slot]["links"].append(op["link_id"]) + + +def _interior_link_id(link: Any) -> Any: + """A definition link's id, whichever shape it is stored in (object or the + top-level array form some exports use).""" + if isinstance(link, dict): + return link.get("id") + if isinstance(link, list) and link: + return link[0] + return None + + +def _remove_interior_link(definition: dict, link_id: Any) -> None: + definition["links"] = [lk for lk in definition.get("links") or [] if _interior_link_id(lk) != link_id] + for n in definition.get("nodes") or []: + if not isinstance(n, dict): + continue + for inp in n.get("inputs") or []: + if isinstance(inp, dict) and inp.get("link") == link_id: + inp["link"] = None + for out in n.get("outputs") or []: + if isinstance(out, dict) and link_id in (out.get("links") or []): + out["links"] = [lid for lid in out["links"] if lid != link_id] + + def clear(workflow: dict, *, actor: str = "cli", base_version: int = 0) -> tuple[dict, dict]: """Remove every node, link, and group in one op. last_node_id/last_link_id are preserved so ids minted after a clear stay monotonic (id reuse would @@ -2384,6 +2581,9 @@ def _apply_connect(workflow: dict, op: dict, graph) -> None: # order. Resolve the destination before mutating anything; if it is gone the # target slot does not exist and never will (ids are never reused), so there # is no register to claim and delete simply wins. + if op.get("path"): + _apply_interior_connect(workflow, op) + return dst = _find_by_str(workflow, op["to_node"]) if dst is None: return @@ -2664,6 +2864,11 @@ def _write_target(op: dict) -> tuple: # under a dynamic combo (``model.reference_images.image_1``) must # not share a target with its sibling (``model.reference_videos``). return ("input", str(op["to_node"]), "grow", _autogrow_base(str(grow["name"]))) + if op.get("path"): + # An interior input is its own register, scoped by the instance path + # (comfy-multi-player ``writeTarget``): ``2011`` inside 70 is not a + # top-level node 2011. + return ("input", tuple(str(seg) for seg in op["path"]), str(op["to_node"]), op["to_slot"]) return ("input", str(op["to_node"]), op["to_slot"]) return (kind,) diff --git a/comfy_cli/workflow_print.py b/comfy_cli/workflow_print.py index 1a5676866..b84402bf0 100644 --- a/comfy_cli/workflow_print.py +++ b/comfy_cli/workflow_print.py @@ -298,8 +298,15 @@ def _stale_input_slot_links(nodes: list[dict], links: list[list], qualify: Any = ) if holder is not None: warnings.append(f"{where}; rendered through input {str(holder.get('name') or '')!r}, which holds it") + continue + ignored.add(str(link_id)) + rewired = _input_fed_from(inputs, links, link[1], link[2], link_id) + if rewired is not None: + warnings.append( + f"{where}; a leftover row — input {rewired!r} already gets that value from node " + f"{qualify(link[1])} output {link[2]} through another link, so nothing needs re-wiring" + ) else: - ignored.add(str(link_id)) src = link[1] source = "the subgraph input" if str(src) == _PROXY_IN else f"node {qualify(src)} output {link[2]}" fixed = f"{qualify(src)}.{link[2]}" if str(src) != _PROXY_IN else None @@ -315,6 +322,20 @@ def _stale_input_slot_links(nodes: list[dict], links: list[list], qualify: Any = return warnings, ignored +def _input_fed_from(inputs: list, links: list[list], src_id: Any, src_slot: Any, except_id: Any) -> str | None: + """The name of an input in ``inputs`` that a link OTHER than ``except_id`` + feeds from ``src_id``/``src_slot`` — a stale row whose value was already + re-wired — else ``None``.""" + by_id = {str(lk[0]): lk for lk in links if isinstance(lk, list) and len(lk) >= 5} + for inp in inputs: + if not isinstance(inp, dict) or inp.get("link") is None or str(inp["link"]) == str(except_id): + continue + row = by_id.get(str(inp["link"])) + if row is not None and str(row[1]) == str(src_id) and row[2] == src_slot: + return str(inp.get("name") or "") + return None + + def _toposort(printable: list[dict], link_map: dict[str, tuple]) -> list[dict]: """Kahn's algorithm over ``printable`` nodes, using only links whose source AND target are both printable. Ties break by ascending numeric id. diff --git a/tests/comfy_cli/command/test_connect_interior.py b/tests/comfy_cli/command/test_connect_interior.py new file mode 100644 index 000000000..2eb9bb548 --- /dev/null +++ b/tests/comfy_cli/command/test_connect_interior.py @@ -0,0 +1,215 @@ +"""`workflow connect` between two nodes INSIDE one subgraph definition. + +Prod trace f8d27ae4 (SAM3, 357 nodes): three interior links of subgraph 70 +pointed at an input slot their SAM3 nodes no longer had, so the RegexExtract +prompts fed nothing and the text prompts sat empty. The repair is a re-wire +inside the definition (`70/2005.0` -> `70/2011.text_prompt`), but connect +refused every interior address ("a link cannot cross a subgraph boundary"), +so the agent hardcoded the prompt text instead. + +A link between two interior nodes of the same definition crosses no +boundary. connect now wires it and emits a `connect` op carrying the +instance `path`, the shape the doc host's applier (comfy-multi-player +applyInteriorConnect) already accepts. Crossing a boundary is still refused. +""" + +from __future__ import annotations + +import copy + +import pytest +from test_workflow_edit import ( # type: ignore[import-not-found] + _object_info, + _run, + _write, + reset_singleton, # noqa: F401 (autouse fixture) +) + +from comfy_cli import workflow_ops +from comfy_cli.command import workflow_edit +from comfy_cli.cql.engine import Graph + +SG = "5a1c362b-0000-4000-8000-000000000070" + + +def _graph() -> Graph: + info = copy.deepcopy(_object_info()) + info["StringSource"] = { + "input": {"required": {"value": ["STRING", {"default": ""}]}}, + "input_order": {"required": ["value"]}, + "output": ["STRING"], + "output_name": ["STRING"], + "category": "utils", + "display_name": "String Source", + "output_node": False, + "python_module": "nodes", + } + return Graph.from_object_info(info) + + +@pytest.fixture(autouse=True) +def patched_graph(monkeypatch): + monkeypatch.setattr(workflow_edit, "_get_graph", lambda *a, **kw: _graph()) + + +def _workflow(instances: int = 1) -> dict: + nodes = [{"id": 70 + i, "type": SG, "pos": [0, 0], "inputs": [], "outputs": []} for i in range(instances)] + return { + "last_node_id": 80, + "last_link_id": 0, + "nodes": nodes, + "links": [], + "definitions": { + "subgraphs": [ + { + "id": SG, + "name": "Region prompts", + "inputs": [], + "outputs": [], + "nodes": [ + { + "id": 2005, + "type": "StringSource", + "inputs": [{"name": "value", "type": "STRING", "widget": {"name": "value"}, "link": None}], + "outputs": [{"name": "STRING", "type": "STRING", "links": [9001]}], + "widgets_values": ["a red car"], + }, + { + "id": 2011, + "type": "CLIPTextEncode", + "inputs": [ + {"name": "clip", "type": "CLIP", "link": None}, + {"name": "text", "type": "STRING", "widget": {"name": "text"}, "link": None}, + ], + "outputs": [{"name": "CONDITIONING", "type": "CONDITIONING", "links": []}], + "widgets_values": [""], + }, + { + "id": 2012, + "type": "CheckpointLoaderSimple", + "inputs": [], + "outputs": [ + {"name": "MODEL", "type": "MODEL", "links": []}, + {"name": "CLIP", "type": "CLIP", "links": []}, + {"name": "VAE", "type": "VAE", "links": []}, + ], + "widgets_values": ["a.safetensors"], + }, + ], + # The stale row: aimed at input slot 6, which 2011 does not have. + "links": [ + { + "id": 9001, + "origin_id": 2005, + "origin_slot": 0, + "target_id": 2011, + "target_slot": 6, + "type": "STRING", + } + ], + } + ] + }, + } + + +def _sg(wf: dict) -> dict: + return wf["definitions"]["subgraphs"][0] + + +def _node(wf: dict, nid: int) -> dict: + return next(n for n in _sg(wf)["nodes"] if n["id"] == nid) + + +def test_connect_two_interior_nodes_wires_inside_the_definition(tmp_path, capsys): + path = _write(tmp_path, _workflow()) + env = _run(["connect", str(path), "70/2005.STRING", "70/2011.text"], capsys) + assert env["ok"] is True, env + op = env["data"]["op"] + assert op["op"] == "connect" + assert op["path"] == ["70"] + assert (str(op["from_node"]), op["from_slot"], str(op["to_node"]), op["to_slot"]) == ("2005", 0, "2011", 1) + assert op["link_type"] == "STRING" + import json + + wf = json.loads(path.read_text()) + link_id = op["link_id"] + assert _node(wf, 2011)["inputs"][1]["link"] == link_id + assert link_id in _node(wf, 2005)["outputs"][0]["links"] + row = next(lk for lk in _sg(wf)["links"] if lk["id"] == link_id) + assert (row["origin_id"], row["origin_slot"], row["target_id"], row["target_slot"]) == (2005, 0, 2011, 1) + # Top level untouched. + assert wf["links"] == [] + + +def test_interior_connect_into_a_link_input_by_slot_name(tmp_path, capsys): + path = _write(tmp_path, _workflow()) + env = _run(["connect", str(path), "70/2012.CLIP", "70/2011.clip"], capsys) + assert env["ok"] is True, env + assert env["data"]["op"]["to_slot"] == 0 + + +def test_interior_connect_replaces_the_incumbent_link(tmp_path, capsys): + import json + + path = _write(tmp_path, _workflow()) + first = _run(["connect", str(path), "70/2005.STRING", "70/2011.text", "--base-version", "1"], capsys) + second = _run(["connect", str(path), "70/2005.STRING", "70/2011.text", "--base-version", "2"], capsys) + first_id, second_id = first["data"]["op"]["link_id"], second["data"]["op"]["link_id"] + wf = json.loads(path.read_text()) + ids = [lk["id"] for lk in _sg(wf)["links"]] + assert second_id in ids and first_id not in ids + assert _node(wf, 2011)["inputs"][1]["link"] == second_id + assert first_id not in _node(wf, 2005)["outputs"][0]["links"] + + +def test_interior_connect_type_mismatch_is_refused(tmp_path, capsys): + path = _write(tmp_path, _workflow()) + env = _run(["connect", str(path), "70/2012.MODEL", "70/2011.clip"], capsys) + assert env["ok"] is False + assert "type mismatch" in env["error"]["message"] + + +def test_crossing_the_boundary_is_still_refused(tmp_path, capsys): + wf = _workflow() + wf["nodes"].append( + {"id": 9, "type": "EmptyLatentImage", "inputs": [], "outputs": [{"name": "LATENT", "type": "LATENT"}]} + ) + path = _write(tmp_path, wf) + env = _run(["connect", str(path), "9.LATENT", "70/2011.text"], capsys) + assert env["ok"] is False + assert "inside subgraph 70" in env["error"]["message"] + + +def test_a_shared_definition_is_refused(tmp_path, capsys): + path = _write(tmp_path, _workflow(instances=2)) + env = _run(["connect", str(path), "70/2005.STRING", "70/2011.text"], capsys) + assert env["ok"] is False + assert "2 instances" in env["error"]["message"] + + +def test_the_op_replays_to_the_same_document(): + wf = _workflow() + graph = _graph() + out, op = workflow_ops.connect(copy.deepcopy(wf), graph, "70/2005", "STRING", "70/2011", "text") + replayed = workflow_ops.apply_op(copy.deepcopy(wf), op, graph) + assert replayed["definitions"] == out["definitions"] + assert workflow_ops._write_target(op) == ("input", ("70",), "2011", 1) + + +def test_print_before_and_after_the_rewire(): + """Before: print names the stale row and the exact re-wire. After: the + interior input renders from its source and the row is called a harmless + leftover, so a caller is not sent round the loop again.""" + from comfy_cli.workflow_print import render_py + + wf = _workflow() + graph = _graph() + before = render_py(copy.deepcopy(wf), graph) + (stale,) = [w for w in before.warnings if "link 9001" in w] + assert "`connect 70/2005.0 70/2011.`" in stale + out, _op = workflow_ops.connect(wf, graph, "70/2005", "STRING", "70/2011", "text") + after = render_py(out, graph) + (leftover,) = [w for w in after.warnings if "link 9001" in w] + assert "nothing needs re-wiring" in leftover and "'text'" in leftover + assert "text=string_source" in after.source diff --git a/tests/comfy_cli/command/test_workflow_print_cmd.py b/tests/comfy_cli/command/test_workflow_print_cmd.py index 3aaf30490..0f5eeda1d 100644 --- a/tests/comfy_cli/command/test_workflow_print_cmd.py +++ b/tests/comfy_cli/command/test_workflow_print_cmd.py @@ -231,7 +231,6 @@ def test_print_renders_a_stale_input_slot_link_and_reports_it(tmp_path, capsys): assert d["node_count"] == 2 assert "samples=empty_latent_image" in d["source"] assert d["warnings"] == [ - "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was " - "ignored. It was wired from node 1 output 0 — if that value was meant for node 2, re-wire it with " - "`connect 1.0 2.` rather than retyping the value" + "link 7 targets input slot 6 on node 2, which has 2 inputs; a leftover row — input 'samples' already gets " + "that value from node 1 output 0 through another link, so nothing needs re-wiring" ] diff --git a/tests/comfy_cli/test_workflow_print.py b/tests/comfy_cli/test_workflow_print.py index 6c58cf5ea..8d32e6aca 100644 --- a/tests/comfy_cli/test_workflow_print.py +++ b/tests/comfy_cli/test_workflow_print.py @@ -1051,9 +1051,10 @@ def test_out_of_range_input_slot_renders_and_warns(sd15_graph): assert "samples=empty_latent_image" in res.source assert "vae=None" in res.source assert res.warnings == [ - "link 7 targets input slot 6 on node 2, which has 2 inputs; no input holds it, so it feeds nothing and was " - "ignored. It was wired from node 1 output 0 — if that value was meant for node 2, re-wire it with " - "`connect 1.0 2.` rather than retyping the value" + # Link 1 already carries node 1's LATENT into `samples`: the stale row is + # a duplicate of a live link, so nothing is sent to be re-wired. + "link 7 targets input slot 6 on node 2, which has 2 inputs; a leftover row — input 'samples' already gets " + "that value from node 1 output 0 through another link, so nothing needs re-wiring" ] @@ -1119,7 +1120,7 @@ def test_out_of_range_input_slot_fed_by_the_definition_input_proxy_is_reported(s sg = next(s for s in wf["definitions"]["subgraphs"] if s["id"] == "d33c1791-dfd2-4102-8540-aa63e4434cd2") tgt = sg["nodes"][0] sg["links"].append( - {"id": 9998, "origin_id": -10, "origin_slot": 0, "target_id": tgt["id"], "target_slot": 42, "type": "*"} + {"id": 9998, "origin_id": -10, "origin_slot": 99, "target_id": tgt["id"], "target_slot": 42, "type": "*"} ) res = render_py(wf, graph) n_inputs = len(tgt.get("inputs") or []) From c93bd87c69233e4e475b017d1057c0410f0619b6 Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:44:21 -0700 Subject: [PATCH 05/10] wip: validate reports broken link rows with the connect that repairs them Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/command/workflow.py | 12 ++ comfy_cli/error_codes.py | 14 ++ comfy_cli/link_integrity.py | 192 ++++++++++++++++++ .../command/test_validate_broken_links.py | 129 ++++++++++++ 4 files changed, 347 insertions(+) create mode 100644 comfy_cli/link_integrity.py create mode 100644 tests/comfy_cli/command/test_validate_broken_links.py diff --git a/comfy_cli/command/workflow.py b/comfy_cli/command/workflow.py index f7d7089ae..afa7eee3d 100644 --- a/comfy_cli/command/workflow.py +++ b/comfy_cli/command/workflow.py @@ -1731,6 +1731,8 @@ def validate_api_workflow( # The converter reuses the object_info the graph was already built from # (`graph.object_info`), so offline `--input` works and no second fetch happens. converted_from_ui = False + link_errors: list[dict] = [] + link_warnings: list[dict] = [] if is_ui_workflow(wf_data): if renderer.is_pretty(): rprint("[yellow]Detected UI-format workflow, converting to API format...[/yellow]") @@ -1762,6 +1764,11 @@ def validate_api_workflow( ) raise typer.Exit(code=1) editable_ids = _editable_node_ids(wf_data) + from comfy_cli.link_integrity import broken_link_findings + + # The lowering reads each input's own `link` and never a row's slots, + # so a broken row disappears in it; judge the canvas before it does. + link_errors, link_warnings = broken_link_findings(wf_data) wf_data = converted converted_from_ui = True @@ -1783,6 +1790,11 @@ def validate_api_workflow( if editable != nid: issue["api_node_id"] = nid issue["node_id"] = editable + if link_errors or link_warnings: + result["errors"].extend(link_errors) + result["warnings"].extend(link_warnings) + if link_errors: + result["valid"] = False # Preview credit spend: partner-API (paid) nodes spend Comfy credits when the # workflow is run. This is the same detection `comfy run` uses (authoritative diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index e2ff964de..b8d32a956 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -629,6 +629,20 @@ class ErrorCode: "a skill dir must contain SKILL.md with `name:`/`description:` frontmatter; run `comfy skills validate `", ), # --- workflow editor ----------------------------------------------------- + ErrorCode( + "link_slot_out_of_range", + "A `workflow validate` finding: a link row of a canvas workflow reads an output slot its source node " + "does not have, or targets an input slot its node does not have while an input that could take the " + "value sits empty. The UI→API lowering drops such a row, so the value it was drawn to carry reaches " + "nothing. `node_id` addresses the target (`70/2011` inside a subgraph).", + "re-wire it with the `connect` the hint names (it works between two nodes inside one subgraph too) — " + "don't retype the value the link was meant to carry", + ), + ErrorCode( + "link_source_missing", + "A `workflow validate` finding: an input is wired from a node that does not exist, so it receives nothing.", + "wire the input from a real node with `comfy workflow connect`", + ), ErrorCode( "workflow_not_frontend_format", "Workflow editing requires the UI export (with `nodes[]` / `links[]`); " diff --git a/comfy_cli/link_integrity.py b/comfy_cli/link_integrity.py new file mode 100644 index 000000000..6391aaaf1 --- /dev/null +++ b/comfy_cli/link_integrity.py @@ -0,0 +1,192 @@ +"""Broken link rows in a canvas (UI-format) workflow, as validate findings. + +The UI→API lowering resolves every input through the node's own +``inputs[].link`` and never reads a link row's slot indexes, so a row that +points at an input slot the node does not have, or at an output slot its +source does not have, vanished silently: validate said "valid" while the value +the row was meant to carry reached nothing. Production trace f8d27ae4: three +interior links of a SAM3 subgraph targeted input slot 6 on nodes with inputs +0-5, validate reported 0 errors, and the agent filled the empty prompts by hand +instead of re-wiring them. + +One finding per broken row, addressed the way the edit surface addresses +nodes (``70/2011`` inside a subgraph), each carrying the ``connect`` that +repairs it. +""" + +from __future__ import annotations + +from typing import Any + +_PROXY_IN = "-10" +_PROXY_OUT = "-20" + + +def _is_slot(value: Any) -> bool: + return isinstance(value, int) and not isinstance(value, bool) + + +def _rows(links: Any) -> list[list]: + """Link rows as ``[id, src, src_slot, dst, dst_slot, type]`` whichever + shape they are stored in (top-level arrays, definition objects).""" + out: list[list] = [] + for link in links or []: + if isinstance(link, list) and len(link) >= 5: + out.append(list(link) + [None] * (6 - len(link))) + elif isinstance(link, dict): + out.append( + [ + link.get("id"), + link.get("origin_id"), + link.get("origin_slot"), + link.get("target_id"), + link.get("target_slot"), + link.get("type"), + ] + ) + return out + + +def _scope_findings(nodes: list, links: Any, prefix: str | None) -> tuple[list[dict], list[dict]]: + from comfy_cli.workflow_ops import _types_compatible + + def addr(nid: Any) -> str: + return f"{prefix}/{nid}" if prefix else str(nid) + + errors: list[dict] = [] + warnings: list[dict] = [] + by_id = {str(n.get("id")): n for n in nodes if isinstance(n, dict)} + rows = _rows(links) + rows_by_id = {str(r[0]): r for r in rows} + + def holder(node: dict, link_id: Any) -> dict | None: + for inp in node.get("inputs") or []: + if isinstance(inp, dict) and inp.get("link") is not None and str(inp["link"]) == str(link_id): + return inp + return None + + def fed_from(node: dict, src: Any, slot: Any, except_id: Any) -> str | None: + for inp in node.get("inputs") or []: + if not isinstance(inp, dict) or inp.get("link") is None or str(inp["link"]) == str(except_id): + continue + row = rows_by_id.get(str(inp["link"])) + if row is not None and str(row[1]) == str(src) and row[2] == slot: + return str(inp.get("name") or "") + return None + + for link_id, src_id, src_slot, tgt_id, tgt_slot, link_type in rows: + if str(tgt_id) == _PROXY_OUT: + continue + tgt = by_id.get(str(tgt_id)) + if tgt is None: + continue # feeds nothing at all; the lowering never sees it + src_is_proxy = str(src_id) == _PROXY_IN + src = None if src_is_proxy else by_id.get(str(src_id)) + held = holder(tgt, link_id) + field = str(held.get("name") or "") if held else None + if not src_is_proxy and src is None: + errors.append( + { + "node_id": addr(tgt_id), + "field": field, + "code": "link_source_missing", + "message": f"link {link_id} into node {addr(tgt_id)} comes from node {addr(src_id)}, which does " + "not exist — the input receives nothing", + "hint": f"wire the input from a real node: `connect . {addr(tgt_id)}.{field or ''}`", + } + ) + continue + outputs = (src or {}).get("outputs") + if not src_is_proxy and isinstance(outputs, list) and _is_slot(src_slot) and not 0 <= src_slot < len(outputs): + names = ", ".join( + f"{i}:{o.get('name')}" for i, o in enumerate(outputs) if isinstance(o, dict) and o.get("name") + ) + errors.append( + { + "node_id": addr(tgt_id), + "field": field, + "code": "link_slot_out_of_range", + "message": f"link {link_id} into node {addr(tgt_id)} reads output slot {src_slot} of node " + f"{addr(src_id)}, which has {len(outputs)} output(s) — the input receives nothing", + "hint": f"re-wire it from an output node {addr(src_id)} has ({names or 'none'}): " + f"`connect {addr(src_id)}. {addr(tgt_id)}.{field or ''}`", + } + ) + continue + inputs = [i for i in tgt.get("inputs") or [] if isinstance(i, dict)] + if not _is_slot(tgt_slot) or 0 <= tgt_slot < len(tgt.get("inputs") or []) or held is not None: + continue + # The row aims past the target's inputs and no input holds it: the + # value it was drawn to carry reaches nothing. + if src_is_proxy: + source_ref = None + source_type = link_type + else: + source_ref = f"{addr(src_id)}.{src_slot}" + out = outputs[src_slot] if isinstance(outputs, list) and _is_slot(src_slot) else {} + source_type = (out or {}).get("type") or link_type + already = fed_from(tgt, src_id, src_slot, link_id) + if already is not None: + continue # a leftover row: that value already reaches input `already` + candidates = [ + str(i.get("name")) + for i in inputs + if i.get("link") is None and i.get("name") and _types_compatible(source_type, i.get("type")) + ] + where = ( + f"link {link_id} from {'the subgraph input' if src_is_proxy else f'node {addr(src_id)} output {src_slot}'} " + f"targets input slot {tgt_slot} of node {addr(tgt_id)}, which has {len(tgt.get('inputs') or [])} inputs — " + "the value reaches nothing" + ) + if source_ref and len(candidates) == 1: + fix = f"`connect {source_ref} {addr(tgt_id)}.{candidates[0]}`" + elif source_ref and candidates: + fix = f"`connect {source_ref} {addr(tgt_id)}.`" + else: + fix = None + finding = { + "node_id": addr(tgt_id), + "field": candidates[0] if len(candidates) == 1 else None, + "code": "link_slot_out_of_range", + "message": where, + "hint": ( + f"re-wire it with {fix} — don't retype the value it was meant to carry" + if fix + else "re-wire it to the input it was meant for" + ), + } + # Only an error when an input that could take the value sits empty: + # that is wiring the graph is missing. Otherwise the row is inert. + (errors if candidates else warnings).append(finding) + return errors, warnings + + +def broken_link_findings(workflow: dict) -> tuple[list[dict], list[dict]]: + """``(errors, warnings)`` for every broken link row in a canvas workflow, + top level and inside each subgraph definition (addressed through its + first instance, as ``slots``/``set-widget`` address interior nodes).""" + if not isinstance(workflow, dict): + return [], [] + nodes = [n for n in workflow.get("nodes") or [] if isinstance(n, dict)] + errors, warnings = _scope_findings(nodes, workflow.get("links"), None) + + defs = (workflow.get("definitions") or {}) if isinstance(workflow.get("definitions"), dict) else {} + subgraphs = [sg for sg in defs.get("subgraphs") or [] if isinstance(sg, dict) and sg.get("id")] + by_def = {str(sg["id"]): sg for sg in subgraphs} + prefix: dict[str, str] = {} + queue: list[tuple[list, str | None]] = [(nodes, None)] + while queue: + scope_nodes, scope_prefix = queue.pop(0) + for n in scope_nodes: + def_id = str(n.get("type")) + if def_id in by_def and def_id not in prefix: + prefix[def_id] = f"{scope_prefix}/{n.get('id')}" if scope_prefix else str(n.get("id")) + inner = [x for x in by_def[def_id].get("nodes") or [] if isinstance(x, dict)] + queue.append((inner, prefix[def_id])) + for def_id, pfx in prefix.items(): + sg = by_def[def_id] + inner = [x for x in sg.get("nodes") or [] if isinstance(x, dict)] + e, w = _scope_findings(inner, sg.get("links"), pfx) + errors.extend(e) + warnings.extend(w) + return errors, warnings diff --git a/tests/comfy_cli/command/test_validate_broken_links.py b/tests/comfy_cli/command/test_validate_broken_links.py new file mode 100644 index 000000000..631212a69 --- /dev/null +++ b/tests/comfy_cli/command/test_validate_broken_links.py @@ -0,0 +1,129 @@ +"""validate reports broken link rows instead of lowering them away. + +Prod trace f8d27ae4: three links of a SAM3 subgraph targeted input slot 6 on +nodes with inputs 0-5. The UI→API lowering reads each input's own ``link`` +and never a row's slots, so the rows vanished, validate said 0 errors, and +the agent typed the prompts in by hand instead of re-wiring them. +""" + +from __future__ import annotations + +import copy +import json + +from test_connect_interior import _workflow as _interior_workflow # type: ignore[import-not-found] +from test_workflow_validate import _run, _write, reset_singleton # type: ignore[import-not-found] # noqa: F401 + +from comfy_cli.command import workflow as workflow_cmd +from comfy_cli.link_integrity import broken_link_findings + +OBJECT_INFO = { + "StringSource": { + "input": {"required": {"value": ["STRING", {"default": ""}]}}, + "input_order": {"required": ["value"]}, + "output": ["STRING"], + "output_name": ["STRING"], + "category": "utils", + "display_name": "String Source", + "output_node": False, + "python_module": "nodes", + }, + "TextSink": { + "input": {"required": {"text": ["STRING", {"default": ""}]}}, + "input_order": {"required": ["text"]}, + "output": [], + "output_name": [], + "category": "utils", + "display_name": "Text Sink", + "output_node": True, + "python_module": "nodes", + }, +} + + +def _canvas(link_row: list) -> dict: + return { + "last_node_id": 2, + "last_link_id": 7, + "nodes": [ + { + "id": 1, + "type": "StringSource", + "inputs": [{"name": "value", "type": "STRING", "widget": {"name": "value"}, "link": None}], + "outputs": [{"name": "STRING", "type": "STRING", "links": [7]}], + "widgets_values": ["a red car"], + "mode": 0, + }, + { + "id": 2, + "type": "TextSink", + "inputs": [{"name": "text", "type": "STRING", "widget": {"name": "text"}, "link": None}], + "outputs": [], + "widgets_values": [""], + "mode": 0, + }, + ], + "links": [link_row], + "version": 0.4, + } + + +def _validate(tmp_path, capsys, wf: dict) -> dict: + oi = _write(tmp_path, "oi.json", OBJECT_INFO) + path = _write(tmp_path, "wf.json", wf) + _code, env, _ = _run(workflow_cmd.app, ["validate", "--workflow", path, "--input", oi], capsys) + return env + + +def test_a_link_to_a_missing_input_slot_is_an_error_with_the_rewire(tmp_path, capsys): + env = _validate(tmp_path, capsys, _canvas([7, 1, 0, 2, 6, "STRING"])) + data = env["data"] + assert data["valid"] is False + (err,) = [e for e in data["errors"] if e["code"] == "link_slot_out_of_range"] + assert err["node_id"] == "2" + assert err["field"] == "text" + assert "`connect 1.0 2.text`" in err["hint"] + assert "don't retype" in err["hint"] + + +def test_a_link_from_a_missing_output_slot_is_an_error(tmp_path, capsys): + wf = _canvas([7, 1, 3, 2, 0, "STRING"]) + wf["nodes"][1]["inputs"][0]["link"] = 7 + env = _validate(tmp_path, capsys, wf) + (err,) = [e for e in env["data"]["errors"] if e["code"] == "link_slot_out_of_range"] + assert "output slot 3 of node 1, which has 1 output(s)" in err["message"] + assert "`connect 1. 2.text`" in err["hint"] + + +def test_a_healthy_link_is_clean(tmp_path, capsys): + wf = _canvas([7, 1, 0, 2, 0, "STRING"]) + wf["nodes"][1]["inputs"][0]["link"] = 7 + env = _validate(tmp_path, capsys, wf) + assert env["data"]["valid"] is True, env["data"]["errors"] + + +def test_interior_rows_are_addressed_through_the_instance(): + errors, _warnings = broken_link_findings(_interior_workflow()) + (err,) = errors + assert err["node_id"] == "70/2011" + assert "`connect 70/2005.0 70/2011.text`" in err["hint"] + + +def test_a_rewired_leftover_row_is_not_an_error(): + from test_connect_interior import _graph # type: ignore[import-not-found] + + from comfy_cli import workflow_ops + + wf, _op = workflow_ops.connect( + copy.deepcopy(_interior_workflow()), _graph(), "70/2005", "STRING", "70/2011", "text" + ) + errors, warnings = broken_link_findings(wf) + assert errors == [] and warnings == [] + + +def test_a_row_with_no_empty_input_to_take_it_is_only_a_warning(): + wf = _interior_workflow() + node = next(n for n in wf["definitions"]["subgraphs"][0]["nodes"] if n["id"] == 2011) + node["inputs"] = [i for i in node["inputs"] if i["name"] != "text"] + errors, warnings = broken_link_findings(json.loads(json.dumps(wf))) + assert errors == [] and len(warnings) == 1 From 44273f59cd147a1cd38a4abbda2fa18641bd2240 Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 20:56:02 -0700 Subject: [PATCH 06/10] docs(changelog): agent tool output bounds, select row queries, interior connect, broken links Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72f2934a4..8e15c2231 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,42 @@ history. ## [Unreleased] +### Added + +- `--select` takes gjson row queries: `items.#()#` keeps the elements + that match and `items.#()` is the first one, with `==` `!=` `<` `<=` + `>` `>=` `%` (glob) `!%` against a quoted string, number, `true`, `false` or + `null` (e.g. `slots.#(node_type=="CLIPTextEncode")#.address`, + `inputs.#(name=="lora_name").choices.#(%"*detail*")#`). The corpus in + `tests/data/selector_conformance.json` pins the grammar for other + implementations. +- `comfy workflow connect` wires two nodes inside the same subgraph + (`connect 70/2005.0 70/2011.text`) and emits a `connect` op carrying the + instance `path`. A link that crosses the subgraph boundary is still refused, + and so is a link inside a definition shared by several instances. +- `comfy workflow validate` reports broken link rows of a canvas workflow as + `link_slot_out_of_range` / `link_source_missing` errors, each with the + `connect` that repairs it. The UI→API lowering used to drop such a row and + validate said the graph was valid. +- `comfy workflow validate --full-options` and `comfy nodes show --all-choices` + (also on `nodes search --expand-top`) list a long option list in full. + +### Changed + +- An `unknown_enum_value` finding (validate, set-widget, edit batches) names the + closest options in `suggestions` (at most 5) with `option_count`, and carries + `valid_options` only when the list has 12 options or fewer. It used to carry + the whole folder listing twice; one validate in production came to ~340K + tokens. +- `comfy nodes show` and `nodes search --expand-top` cut a combo input's + `choices` longer than 20 to the first 20, with `choices_total`, + `choices_truncated` and a `choices_note` naming the `--select` that filters + the full list. `--select` still projects the full schema. +- `comfy workflow print` renders a graph with broken links instead of refusing + it: the input prints `None`, the line is marked `BROKEN`, and a warning names + the `connect` that repairs it. A stale row whose value already reaches the + node through another link is reported as a leftover. + ### Fixed - A cloud request the account's plan does not allow (free generations used up, From df06170c385b273b1b0415af24a2cd7c06895f6d Mon Sep 17 00:00:00 2001 From: kishore Date: Wed, 30 Sep 2026 21:32:19 -0700 Subject: [PATCH 07/10] =?UTF-8?q?fix:=20review=20findings=20=E2=80=94=20ro?= =?UTF-8?q?w-query=20parens=20only=20after=20#,=20non-integer=20link=20slo?= =?UTF-8?q?ts=20in=20validate,=20boundary=20linkIds=20on=20interior=20rewi?= =?UTF-8?q?re,=20nested=20enum=20hints?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/command/nodes.py | 22 ++++++++++------- comfy_cli/cql/engine.py | 7 ++++++ comfy_cli/link_integrity.py | 14 ++++++++++- comfy_cli/selector.py | 11 +++++---- comfy_cli/workflow_ops.py | 4 ++++ .../command/test_connect_interior.py | 13 ++++++++++ .../command/test_validate_broken_links.py | 8 +++++++ tests/comfy_cli/test_agent_output_bounds.py | 24 +++++++++++++++++++ tests/data/selector_conformance.json | 8 ++++++- 9 files changed, 96 insertions(+), 15 deletions(-) diff --git a/comfy_cli/command/nodes.py b/comfy_cli/command/nodes.py index 49666dd31..c4498fd36 100644 --- a/comfy_cli/command/nodes.py +++ b/comfy_cli/command/nodes.py @@ -169,8 +169,9 @@ def _cap_choices(payload: dict[str, Any]) -> dict[str, Any]: with ``choices_total`` and ``choices_truncated``. Adds a top-level ``choices_note`` naming how to read or filter the full list, once.""" capped: list[str] = [] + top_level: list[str] = [] - def walk(inputs: Any) -> None: + def walk(inputs: Any, nested: bool = False) -> None: if not isinstance(inputs, list): return for entry in inputs: @@ -182,19 +183,24 @@ def walk(inputs: Any) -> None: entry["choices"] = choices[:CHOICES_INLINE_MAX] entry["choices_truncated"] = True capped.append(str(entry.get("name"))) + if not nested: + top_level.append(str(entry.get("name"))) for option in entry.get("dynamic_options") or []: if isinstance(option, dict): - walk(option.get("inputs")) + walk(option.get("inputs"), nested=True) walk(payload.get("inputs")) if capped: name = payload.get("name") or "" - first = capped[0] - payload["choices_note"] = ( - f"{', '.join(capped)}: only the first {CHOICES_INLINE_MAX} choices are listed (see choices_total). " - f"Check or find one with `comfy nodes show {name} --select " - f'\'inputs.#(name=="{first}").choices.#(%"**")#\'`; --all-choices lists every choice.' - ) + note = f"{', '.join(capped)}: only the first {CHOICES_INLINE_MAX} choices are listed (see choices_total). " + if top_level: + # A nested (dynamic-combo) input is not under top-level `inputs`, + # so only a top-level one gets the filter query. + note += ( + f"Check or find one with `comfy nodes show {name} --select " + f'\'inputs.#(name=="{top_level[0]}").choices.#(%"**")#\'`; ' + ) + payload["choices_note"] = note + "--all-choices lists every choice." return payload diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index ccc78c27f..5375d0284 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -134,6 +134,13 @@ def enum_listing_hint( n = len(options) if n <= ENUM_INLINE_MAX or _FULL_ENUM_OPTIONS.get(): return f"valid options: {', '.join(str(v) for v in options)}" + if "." in field: + # A dotted name is a dynamic-combo sub-input: it sits under + # `dynamic_options[].inputs`, not top-level `inputs`. + return ( + f"pick one of `suggestions` ({len(suggestions)} of {n} options); re-run validate with " + f"--full-options to list all {n}" + ) target = class_type or "" query = f'inputs.#(name=="{field}").{listing}.#(%"**")#' return ( diff --git a/comfy_cli/link_integrity.py b/comfy_cli/link_integrity.py index 6391aaaf1..d845627ee 100644 --- a/comfy_cli/link_integrity.py +++ b/comfy_cli/link_integrity.py @@ -96,6 +96,18 @@ def fed_from(node: dict, src: Any, slot: Any, except_id: Any) -> str | None: } ) continue + if not _is_slot(tgt_slot) or not (src_is_proxy or _is_slot(src_slot)): + errors.append( + { + "node_id": addr(tgt_id), + "field": field, + "code": "link_slot_out_of_range", + "message": f"link {link_id} into node {addr(tgt_id)} has a non-integer slot — the input " + "receives nothing", + "hint": f"re-wire it: `connect {addr(src_id)}. {addr(tgt_id)}.{field or ''}`", + } + ) + continue outputs = (src or {}).get("outputs") if not src_is_proxy and isinstance(outputs, list) and _is_slot(src_slot) and not 0 <= src_slot < len(outputs): names = ", ".join( @@ -114,7 +126,7 @@ def fed_from(node: dict, src: Any, slot: Any, except_id: Any) -> str | None: ) continue inputs = [i for i in tgt.get("inputs") or [] if isinstance(i, dict)] - if not _is_slot(tgt_slot) or 0 <= tgt_slot < len(tgt.get("inputs") or []) or held is not None: + if 0 <= tgt_slot < len(tgt.get("inputs") or []) or held is not None: continue # The row aims past the target's inputs and no input holds it: the # value it was drawn to carry reaches nothing. diff --git a/comfy_cli/selector.py b/comfy_cli/selector.py index 95cea0bec..f63472c7f 100644 --- a/comfy_cli/selector.py +++ b/comfy_cli/selector.py @@ -85,7 +85,8 @@ def select(data: Any, expr: str) -> tuple[Any, bool]: def _split_top(text: str, sep: str) -> list[str] | None: """Split ``text`` on ``sep`` outside any ``#(...)`` row query and outside - quoted strings in one. ``None`` for an unbalanced query or quote.""" + quoted strings in one. ``None`` for an unclosed query or quote. A paren + that does not open (or sit inside) a row query is part of a key.""" parts: list[str] = [] depth = 0 in_str = False @@ -100,12 +101,12 @@ def _split_top(text: str, sep: str) -> list[str] | None: in_str = False elif ch == '"' and depth: in_str = True - elif ch == "(": + elif ch == "(" and (depth or (i > 0 and text[i - 1] == WILDCARD)): + # Only `#(` opens a row query; outside one a paren is an ordinary + # key character, as it was before row queries existed. depth += 1 - elif ch == ")": + elif ch == ")" and depth: depth -= 1 - if depth < 0: - return None elif ch == sep and depth == 0: parts.append(text[start:i]) start = i + 1 diff --git a/comfy_cli/workflow_ops.py b/comfy_cli/workflow_ops.py index 4ca91e0f0..9a007ef8f 100644 --- a/comfy_cli/workflow_ops.py +++ b/comfy_cli/workflow_ops.py @@ -1586,6 +1586,10 @@ def _interior_link_id(link: Any) -> Any: def _remove_interior_link(definition: dict, link_id: Any) -> None: definition["links"] = [lk for lk in definition.get("links") or [] if _interior_link_id(lk) != link_id] + # A replaced link from the input proxy is also listed on its boundary input. + for boundary in definition.get("inputs") or []: + if isinstance(boundary, dict) and isinstance(boundary.get("linkIds"), list): + boundary["linkIds"] = [lid for lid in boundary["linkIds"] if lid != link_id] for n in definition.get("nodes") or []: if not isinstance(n, dict): continue diff --git a/tests/comfy_cli/command/test_connect_interior.py b/tests/comfy_cli/command/test_connect_interior.py index 2eb9bb548..a9ce73b58 100644 --- a/tests/comfy_cli/command/test_connect_interior.py +++ b/tests/comfy_cli/command/test_connect_interior.py @@ -213,3 +213,16 @@ def test_print_before_and_after_the_rewire(): (leftover,) = [w for w in after.warnings if "link 9001" in w] assert "nothing needs re-wiring" in leftover and "'text'" in leftover assert "text=string_source" in after.source + + +def test_replacing_a_boundary_link_drops_it_from_the_boundary_input(): + wf = _workflow() + sg = _sg(wf) + sg["inputs"] = [{"id": "in-1", "name": "prompt", "type": "STRING", "linkIds": [8001, 8002]}] + sg["links"].append( + {"id": 8001, "origin_id": -10, "origin_slot": 0, "target_id": 2011, "target_slot": 1, "type": "STRING"} + ) + _node(wf, 2011)["inputs"][1]["link"] = 8001 + out, op = workflow_ops.connect(wf, _graph(), "70/2005", "STRING", "70/2011", "text") + assert _sg(out)["inputs"][0]["linkIds"] == [8002] + assert 8001 not in [lk["id"] for lk in _sg(out)["links"]] diff --git a/tests/comfy_cli/command/test_validate_broken_links.py b/tests/comfy_cli/command/test_validate_broken_links.py index 631212a69..263d09d80 100644 --- a/tests/comfy_cli/command/test_validate_broken_links.py +++ b/tests/comfy_cli/command/test_validate_broken_links.py @@ -127,3 +127,11 @@ def test_a_row_with_no_empty_input_to_take_it_is_only_a_warning(): node["inputs"] = [i for i in node["inputs"] if i["name"] != "text"] errors, warnings = broken_link_findings(json.loads(json.dumps(wf))) assert errors == [] and len(warnings) == 1 + + +def test_a_non_integer_slot_is_an_error_like_print_marks_it(tmp_path, capsys): + wf = _canvas([7, 1, None, 2, 0, "STRING"]) + wf["nodes"][1]["inputs"][0]["link"] = 7 + env = _validate(tmp_path, capsys, wf) + (err,) = [e for e in env["data"]["errors"] if e["code"] == "link_slot_out_of_range"] + assert "non-integer slot" in err["message"] diff --git a/tests/comfy_cli/test_agent_output_bounds.py b/tests/comfy_cli/test_agent_output_bounds.py index cb975b966..8228c5c1f 100644 --- a/tests/comfy_cli/test_agent_output_bounds.py +++ b/tests/comfy_cli/test_agent_output_bounds.py @@ -155,3 +155,27 @@ def test_search_expand_top_caps_choices(self, capsys, monkeypatch): entry = data["expanded"][0] assert entry["inputs"][0]["choices_total"] == len(FILES) assert len(entry["inputs"][0]["choices"]) == nodes_cmd.CHOICES_INLINE_MAX + + +def test_a_nested_enum_hint_does_not_point_at_top_level_inputs(): + from comfy_cli.cql.engine import enum_listing_hint + + hint = enum_listing_hint("model.style", FILES, FILES[:5], "Foo") + assert "--full-options" in hint and "inputs.#(" not in hint + + +def test_a_nested_capped_choice_list_only_advertises_all_choices(): + payload = { + "name": "Foo", + "inputs": [ + { + "name": "model", + "choices": [], + "dynamic_options": [{"keys": ["a"], "inputs": [{"name": "model.ckpt", "choices": FILES}]}], + } + ], + } + nodes_cmd._cap_choices(payload) + sub = payload["inputs"][0]["dynamic_options"][0]["inputs"][0] + assert sub["choices_total"] == len(FILES) + assert "--select" not in payload["choices_note"] and "--all-choices" in payload["choices_note"] diff --git a/tests/data/selector_conformance.json b/tests/data/selector_conformance.json index a394e3cc1..98b4c02ca 100644 --- a/tests/data/selector_conformance.json +++ b/tests/data/selector_conformance.json @@ -9,6 +9,8 @@ ], "count": 3, "meta": {"k": "v"}, + "a(b": 1, + "c)d": {"e": 2}, "slots": [ {"address": "5.seed", "instance_id": 5, "node_type": "KSampler", "current_value": 1}, {"address": "5.steps", "instance_id": 5, "node_type": "KSampler", "current_value": 20}, @@ -60,6 +62,10 @@ {"expr": "meta.#(k==\"v\")", "matched": false}, {"expr": "slots.#(node_type==\"KSampler\"", "matched": false}, {"expr": "slots.#(node_type=~\"K\")#", "matched": false}, - {"expr": "slots.#()#", "matched": false} + {"expr": "slots.#()#", "matched": false}, + {"expr": "a(b", "matched": true, "result": 1}, + {"expr": "c)d.e", "matched": true, "result": 2}, + {"expr": "a(b,count", "matched": true, "result": {"a(b": 1, "count": 3}}, + {"expr": "meta.k(x", "matched": false} ] } From 3a09b413128087bfd779093e41f7334615f9624e Mon Sep 17 00:00:00 2001 From: kishore Date: Fri, 2 Oct 2026 12:06:02 -0700 Subject: [PATCH 08/10] fix: quote catalog names in copyable --select hints; a row query whose projection misses every kept row is a miss CodeRabbit on #960: a node or input name with a quote broke the selector in the enum and choices hints, and shell syntax in a name could run when the command is copied. The field is now a JSON string literal and both arguments are shell-quoted. `rows.#(cond)#.missing` returned [] as a match and skipped the select_no_match fallback; it now misses like `#` does. Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/command/nodes.py | 10 +++---- comfy_cli/cql/engine.py | 10 +++++-- comfy_cli/selector.py | 6 +++- tests/comfy_cli/test_agent_output_bounds.py | 31 +++++++++++++++++++++ tests/comfy_cli/test_selector.py | 9 ++++++ 5 files changed, 57 insertions(+), 9 deletions(-) diff --git a/comfy_cli/command/nodes.py b/comfy_cli/command/nodes.py index c4498fd36..c59718923 100644 --- a/comfy_cli/command/nodes.py +++ b/comfy_cli/command/nodes.py @@ -20,6 +20,8 @@ from __future__ import annotations import difflib +import json +import shlex from typing import Annotated, Any import typer @@ -191,15 +193,13 @@ def walk(inputs: Any, nested: bool = False) -> None: walk(payload.get("inputs")) if capped: - name = payload.get("name") or "" + name = shlex.quote(str(payload["name"])) if payload.get("name") else "" note = f"{', '.join(capped)}: only the first {CHOICES_INLINE_MAX} choices are listed (see choices_total). " if top_level: # A nested (dynamic-combo) input is not under top-level `inputs`, # so only a top-level one gets the filter query. - note += ( - f"Check or find one with `comfy nodes show {name} --select " - f'\'inputs.#(name=="{top_level[0]}").choices.#(%"**")#\'`; ' - ) + query = shlex.quote(f'inputs.#(name=={json.dumps(top_level[0])}).choices.#(%"**")#') + note += f"Check or find one with `comfy nodes show {name} --select {query}`; " payload["choices_note"] = note + "--all-choices lists every choice." return payload diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index 5375d0284..fefe1e089 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -15,6 +15,7 @@ import json import logging import math +import shlex import urllib.error import urllib.parse import urllib.request @@ -141,11 +142,14 @@ def enum_listing_hint( f"pick one of `suggestions` ({len(suggestions)} of {n} options); re-run validate with " f"--full-options to list all {n}" ) - target = class_type or "" - query = f'inputs.#(name=="{field}").{listing}.#(%"**")#' + target = shlex.quote(class_type) if class_type else "" + # Catalog names are untrusted text: encode the field as a selector string + # literal and shell-quote both arguments, so the copyable command neither + # breaks the selector nor runs anything a node pack put in a name. + query = shlex.quote(f'inputs.#(name=={json.dumps(field)}).{listing}.#(%"**")#') return ( f"pick one of `suggestions` ({len(suggestions)} of {n} options); to search all {n}, filter them with " - f"`comfy nodes show {target} --select '{query}'`, or re-run validate with --full-options" + f"`comfy nodes show {target} --select {query}`, or re-run validate with --full-options" ) diff --git a/comfy_cli/selector.py b/comfy_cli/selector.py index f63472c7f..08e4162e9 100644 --- a/comfy_cli/selector.py +++ b/comfy_cli/selector.py @@ -261,7 +261,11 @@ def _walk(current: Any, segments: list[str]) -> tuple[Any, bool]: result, matched = _walk(element, rest) if matched: out.append(result) - return out, True + # Like `#`: zero kept elements is an answer, but a remainder that + # matches none of the kept ones is a miss. + if out or not kept: + return out, True + return None, False if seg == WILDCARD: if not isinstance(current, list): return None, False diff --git a/tests/comfy_cli/test_agent_output_bounds.py b/tests/comfy_cli/test_agent_output_bounds.py index 8228c5c1f..e3a2f98ed 100644 --- a/tests/comfy_cli/test_agent_output_bounds.py +++ b/tests/comfy_cli/test_agent_output_bounds.py @@ -179,3 +179,34 @@ def test_a_nested_capped_choice_list_only_advertises_all_choices(): sub = payload["inputs"][0]["dynamic_options"][0]["inputs"][0] assert sub["choices_total"] == len(FILES) assert "--select" not in payload["choices_note"] and "--all-choices" in payload["choices_note"] + + +def test_the_enum_filter_hint_quotes_names_from_the_catalog(): + import shlex + + from comfy_cli.cql.engine import enum_listing_hint + from comfy_cli.selector import select + + field = 'we"ird' + hint = enum_listing_hint(field, FILES, FILES[:5], "Bad'; touch x #") + cmd = next(span for span in hint.split("`") if span.startswith("comfy ")) + argv = shlex.split(cmd) + assert argv[:3] == ["comfy", "nodes", "show"] and argv[3] == "Bad'; touch x #" + query = argv[argv.index("--select") + 1].replace("", "") + _, matched = select({"inputs": [{"name": field, "choices": ["a"]}]}, query) + assert matched + + +def test_the_choices_note_quotes_names_from_the_catalog(): + import shlex + + from comfy_cli.selector import select + + payload = {"name": "Bad'; touch x #", "inputs": [{"name": 'we"ird', "choices": FILES}]} + nodes_cmd._cap_choices(payload) + cmd = payload["choices_note"].split("`")[1] + argv = shlex.split(cmd) + assert argv[3] == "Bad'; touch x #" + query = argv[argv.index("--select") + 1].replace("", "") + _, matched = select({"inputs": [{"name": 'we"ird', "choices": ["a"]}]}, query) + assert matched diff --git a/tests/comfy_cli/test_selector.py b/tests/comfy_cli/test_selector.py index b15e45cd8..5fbb5e92b 100644 --- a/tests/comfy_cli/test_selector.py +++ b/tests/comfy_cli/test_selector.py @@ -212,3 +212,12 @@ def test_select_is_pure_and_does_not_mutate(): select(PAYLOAD, "nope") selected_payload(PAYLOAD, "a..b") assert json.dumps(PAYLOAD, sort_keys=True) == snapshot + + +def test_a_row_query_whose_projection_misses_every_kept_element_is_a_miss(): + from comfy_cli.selector import select + + data = {"rows": [{"name": "a"}, {"name": "b"}]} + assert select(data, 'rows.#(name=="a")#.nope') == (None, False) + assert select(data, 'rows.#(name=="zzz")#.nope') == ([], True) + assert select(data, 'rows.#(name%"*")#.name') == (["a", "b"], True) From 3468ccd1a7438fa0dc5d6a9e28d444c75780584e Mon Sep 17 00:00:00 2001 From: kishore Date: Fri, 2 Oct 2026 19:32:33 -0700 Subject: [PATCH 09/10] fix(cql): carry suggestions on catalog enum warnings; scope the print guarantee to links with a target Port.validate_catalog copied options_omitted but not the suggestions it is counted against, so a value close to nothing reported an omitted count with no options named. The workflow_print_unsupported description now says "missing source node": a link whose target is missing is ignored with a warning, not rendered as BROKEN. Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/cql/engine.py | 2 +- comfy_cli/error_codes.py | 5 +++-- tests/comfy_cli/test_agent_output_bounds.py | 10 ++++++++++ 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/comfy_cli/cql/engine.py b/comfy_cli/cql/engine.py index fefe1e089..e4fce8faa 100644 --- a/comfy_cli/cql/engine.py +++ b/comfy_cli/cql/engine.py @@ -599,7 +599,7 @@ def validate_catalog(self, value: Any) -> list[dict]: } if best is not None: warning["best_match"] = best - for key in ("valid_options", "options_omitted"): + for key in ("suggestions", "valid_options", "options_omitted"): if key in listing: warning[key] = listing[key] if suggestions: diff --git a/comfy_cli/error_codes.py b/comfy_cli/error_codes.py index b8d32a956..f53459ac3 100644 --- a/comfy_cli/error_codes.py +++ b/comfy_cli/error_codes.py @@ -653,9 +653,10 @@ class ErrorCode: "workflow_print_unsupported", "`comfy workflow print` refused: the workflow contains something it cannot render faithfully " "(legacy group node, duplicate node id, link cycle, unknown `--format`). `details.reasons` lists every " - "reason. A broken link (missing endpoint node, missing output or input slot, non-integer slot) is not a " + "reason. A broken link (missing source node, missing output or input slot, non-integer slot) is not a " "refusal: the graph prints with that input as `None`, the line marked `BROKEN`, and a warning naming the " - "`connect` that repairs it.", + "`connect` that repairs it. A link whose target node is missing feeds nothing: it is ignored with a " + "warning.", "fix the listed reasons, or read the graph with `comfy workflow slots` / `comfy workflow ls-nodes`", ), ErrorCode( diff --git a/tests/comfy_cli/test_agent_output_bounds.py b/tests/comfy_cli/test_agent_output_bounds.py index e3a2f98ed..6368edebd 100644 --- a/tests/comfy_cli/test_agent_output_bounds.py +++ b/tests/comfy_cli/test_agent_output_bounds.py @@ -96,6 +96,16 @@ def test_the_edit_finding_is_bounded_too(self): assert finding["option_count"] == len(FILES) assert finding["did_you_mean"][0] == "model_007_v1.safetensors" + def test_the_edit_finding_names_the_options_its_omitted_count_excludes(self): + # A value close to nothing still gets the first options as + # `suggestions`; the finding must carry them, or `options_omitted` + # counts options it never named. + port = Graph.from_object_info(_object_info()).node("CheckpointLoaderSimple").inputs[0] + (finding,) = port.validate_catalog("zzzz-nothing-like-it.bin") + assert "valid_options" not in finding + assert finding["suggestions"], "the omitted count must sit beside the options it excludes" + assert len(finding["suggestions"]) + finding["options_omitted"] == finding["option_count"] + @pytest.fixture(autouse=True) def _renderer(): From 8486173cfd0b766881c22b868102ad1bb8b440f0 Mon Sep 17 00:00:00 2001 From: kishore Date: Sat, 3 Oct 2026 00:12:47 -0700 Subject: [PATCH 10/10] fix(workflow): count nested subgraph instances through their ancestors; dedup proxy-sourced leftover rows _definition_instance_count scanned each definition body once and visited a definition twice (the index keys it by id and by name), so a definition nested in a shared outer one counted wrong: two top-level O holding one I is two live I. It now multiplies body occurrences by the enclosing definition's count, once per definition, and treats a self-instantiating definition as shared. connect's instances > 1 refusal therefore fires for a nested shared definition and no longer refuses a nested single instance. link_integrity's leftover-row dedup (fed_from) now runs for rows sourced from the subgraph input proxy too, and workflow print qualifies a missing interior source id. Co-Authored-By: Claude Opus 5.5 (1M context) --- comfy_cli/link_integrity.py | 9 ++- comfy_cli/workflow_ops.py | 53 ++++++++++++-- comfy_cli/workflow_print.py | 2 +- .../command/test_connect_interior.py | 56 ++++++++++++++ .../test_link_integrity_proxy_dedup.py | 73 +++++++++++++++++++ 5 files changed, 183 insertions(+), 10 deletions(-) create mode 100644 tests/comfy_cli/test_link_integrity_proxy_dedup.py diff --git a/comfy_cli/link_integrity.py b/comfy_cli/link_integrity.py index d845627ee..b408912f8 100644 --- a/comfy_cli/link_integrity.py +++ b/comfy_cli/link_integrity.py @@ -129,7 +129,11 @@ def fed_from(node: dict, src: Any, slot: Any, except_id: Any) -> str | None: if 0 <= tgt_slot < len(tgt.get("inputs") or []) or held is not None: continue # The row aims past the target's inputs and no input holds it: the - # value it was drawn to carry reaches nothing. + # value it was drawn to carry reaches nothing, unless a sibling link + # from the same source+slot (a real node or the subgraph input proxy) + # already carries it, which makes this a leftover row. + if fed_from(tgt, src_id, src_slot, link_id) is not None: + continue if src_is_proxy: source_ref = None source_type = link_type @@ -137,9 +141,6 @@ def fed_from(node: dict, src: Any, slot: Any, except_id: Any) -> str | None: source_ref = f"{addr(src_id)}.{src_slot}" out = outputs[src_slot] if isinstance(outputs, list) and _is_slot(src_slot) else {} source_type = (out or {}).get("type") or link_type - already = fed_from(tgt, src_id, src_slot, link_id) - if already is not None: - continue # a leftover row: that value already reaches input `already` candidates = [ str(i.get("name")) for i in inputs diff --git a/comfy_cli/workflow_ops.py b/comfy_cli/workflow_ops.py index 9a007ef8f..4bc69c34d 100644 --- a/comfy_cli/workflow_ops.py +++ b/comfy_cli/workflow_ops.py @@ -1462,13 +1462,56 @@ def _interior_link_scope(workflow: dict, from_node: Any, to_node: Any) -> dict | def _definition_instance_count(workflow: dict, def_id: str) -> int: - """How many nodes, top level or inside any definition, instantiate ``def_id``.""" + """How many live instances of definition ``def_id`` the workflow holds. + + An instance inside another definition's body is live once per live + instance of that enclosing definition, so a body's occurrences are + multiplied by the enclosing definition's own count (two top-level ``O`` + whose body holds one ``I`` is two live ``I``). Each definition is counted + once even though the index also keys it by name. A definition that + instantiates itself, directly or through others, cannot be expanded; it + counts as shared (2) so callers refuse to rewrite it. + """ from comfy_cli.cql import engine as _engine - count = sum(1 for n in workflow.get("nodes") or [] if isinstance(n, dict) and str(n.get("type")) == def_id) - for sg in _engine._subgraph_defs_by_id(workflow).values(): - count += sum(1 for n in sg.get("nodes") or [] if isinstance(n, dict) and str(n.get("type")) == def_id) - return count + by_key = _engine._subgraph_defs_by_id(workflow) + defs = list({id(sg): sg for sg in by_key.values()}.values()) + + def occurrences(nodes: Any, target: dict) -> int: + return sum(1 for n in nodes or [] if isinstance(n, dict) and by_key.get(str(n.get("type", ""))) is target) + + memo: dict[int, int] = {} + in_progress: set[int] = set() + + def count(target: dict) -> int: + key = id(target) + if key in memo: + return memo[key] + if key in in_progress: + raise _CyclicDefinition + in_progress.add(key) + try: + total = occurrences(workflow.get("nodes"), target) + for sg in defs: + n = occurrences(sg.get("nodes"), target) + if n: + total += n * count(sg) + finally: + in_progress.discard(key) + memo[key] = total + return total + + target = by_key.get(str(def_id)) + if target is None: + return 0 + try: + return count(target) + except _CyclicDefinition: + return 2 + + +class _CyclicDefinition(Exception): + """A subgraph definition instantiates itself, so its instances cannot be counted.""" def _connect_interior( diff --git a/comfy_cli/workflow_print.py b/comfy_cli/workflow_print.py index b84402bf0..fd538823e 100644 --- a/comfy_cli/workflow_print.py +++ b/comfy_cli/workflow_print.py @@ -233,7 +233,7 @@ def holder_of(link_id: Any, tgt: dict | None) -> str | None: continue why = None if src_node is None: - why = f"its source node {src_id} does not exist" + why = f"its source node {qualify(src_id)} does not exist" elif not _is_slot_index(src_slot) or not _is_slot_index(tgt_slot): why = "it has a non-integer slot" else: diff --git a/tests/comfy_cli/command/test_connect_interior.py b/tests/comfy_cli/command/test_connect_interior.py index a9ce73b58..2c3091736 100644 --- a/tests/comfy_cli/command/test_connect_interior.py +++ b/tests/comfy_cli/command/test_connect_interior.py @@ -226,3 +226,59 @@ def test_replacing_a_boundary_link_drops_it_from_the_boundary_input(): out, op = workflow_ops.connect(wf, _graph(), "70/2005", "STRING", "70/2011", "text") assert _sg(out)["inputs"][0]["linkIds"] == [8002] assert 8001 not in [lk["id"] for lk in _sg(out)["links"]] + + +OUTER = "5a1c362b-0000-4000-8000-0000000000aa" + + +def _nested_workflow(outer_instances: int) -> dict: + """``outer_instances`` top-level instances of OUTER, whose definition holds + ONE instance (node 300) of the inner definition SG. Each OUTER instance + therefore carries its own live SG instance, all sharing SG's one body.""" + wf = _workflow(instances=0) + wf["nodes"] = [ + {"id": 80 + i, "type": OUTER, "pos": [0, 0], "inputs": [], "outputs": []} for i in range(outer_instances) + ] + wf["definitions"]["subgraphs"].append( + { + "id": OUTER, + "name": "Outer", + "inputs": [], + "outputs": [], + "nodes": [{"id": 300, "type": SG, "pos": [0, 0], "inputs": [], "outputs": []}], + "links": [], + } + ) + return wf + + +def test_a_definition_shared_through_its_ancestor_is_refused(tmp_path, capsys): + # Two OUTER instances each hold one SG instance: SG has 2 live instances, + # though its body appears once in OUTER's definition. Wiring inside it + # would rewire both, so connect must refuse as for a flat shared def. + path = _write(tmp_path, _nested_workflow(outer_instances=2)) + before = path.read_text() + env = _run(["connect", str(path), "80/300/2005.STRING", "80/300/2011.text"], capsys) + assert env["ok"] is False, env + assert "2 instances" in env["error"]["message"] + assert path.read_text() == before + + +def test_a_nested_single_instance_definition_is_wired(tmp_path, capsys): + path = _write(tmp_path, _nested_workflow(outer_instances=1)) + env = _run(["connect", str(path), "80/300/2005.STRING", "80/300/2011.text"], capsys) + assert env["ok"] is True, env + assert env["data"]["op"]["path"] == ["80", "300"] + + +def test_instance_count_multiplies_through_ancestors(): + assert workflow_ops._definition_instance_count(_nested_workflow(3), SG) == 3 + assert workflow_ops._definition_instance_count(_nested_workflow(3), OUTER) == 3 + + +def test_instance_count_terminates_on_a_self_instantiating_definition(): + wf = _workflow(instances=1) + _sg(wf)["nodes"].append({"id": 2099, "type": SG, "pos": [0, 0], "inputs": [], "outputs": []}) + # A definition that names itself cannot be expanded; it must not hang and + # must report it as shared (more than one live instance). + assert workflow_ops._definition_instance_count(wf, SG) > 1 diff --git a/tests/comfy_cli/test_link_integrity_proxy_dedup.py b/tests/comfy_cli/test_link_integrity_proxy_dedup.py new file mode 100644 index 000000000..ecf06a184 --- /dev/null +++ b/tests/comfy_cli/test_link_integrity_proxy_dedup.py @@ -0,0 +1,73 @@ +"""A leftover link row is inert when its value already reaches the target. + +`_scope_findings` recognizes a stale or duplicate row whose source+slot +already feeds the target through a sibling link, and stays silent. That +dedup must hold when the source is the subgraph's input proxy (-10) too: +the same topology must produce the same (empty) findings whichever kind of +source the rows share. +""" + +from __future__ import annotations + +import pytest + +from comfy_cli.link_integrity import _scope_findings + + +def _scope(src_id: int) -> tuple[list, list]: + nodes = [ + { + "id": 5, + "type": "StringSource", + "inputs": [], + "outputs": [{"name": "STRING", "type": "STRING", "links": [1, 2]}], + }, + { + "id": 7, + "type": "CLIPTextEncode", + "inputs": [ + {"name": "clip", "type": "CLIP", "link": None}, + {"name": "text", "type": "STRING", "link": 1}, + ], + "outputs": [], + }, + ] + links = [ + # The live link: source slot 0 into input 1 (`text`). + [1, src_id, 0, 7, 1, "STRING"], + # A leftover row from the same source+slot, aimed past 7's inputs. + [2, src_id, 0, 7, 6, "STRING"], + ] + return nodes, links + + +@pytest.mark.parametrize("src_id", [5, -10], ids=["real-node source", "subgraph-input proxy source"]) +def test_a_leftover_row_whose_value_already_reaches_the_target_is_silent(src_id): + nodes, links = _scope(src_id) + errors, warnings = _scope_findings(nodes, links, "70") + assert errors == [] + assert warnings == [], "the value already reaches input `text` through link 1" + + +def test_a_proxy_row_that_reaches_nothing_is_still_reported(): + nodes, links = _scope(-10) + nodes[1]["inputs"][1]["link"] = None # nothing else carries the proxy value + links = [links[1]] + errors, warnings = _scope_findings(nodes, links, "70") + assert [f["code"] for f in errors + warnings] == ["link_slot_out_of_range"] + + +def test_print_names_a_missing_interior_source_by_its_qualified_address(): + from comfy_cli.workflow_print import _broken_links + + nodes = [ + { + "id": 7, + "type": "CLIPTextEncode", + "inputs": [{"name": "text", "type": "STRING", "link": 3}], + "outputs": [], + } + ] + warnings, _, _ = _broken_links(nodes, [[3, 99, 0, 7, 0, "STRING"]], qualify=lambda nid: f"70/{nid}") + text = " ".join(warnings) + assert "source node 70/99 does not exist" in text, text