diff --git a/CHANGELOG.md b/CHANGELOG.md index 8269bb653..d3510c731 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,13 @@ history. (`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 delete-node` and `delete-nodes` accept the interior paths + `workflow print` shows (`delete-node 70/2012`). They delete the node from its + subgraph definition with its interior links, and emit a `delete_node` op + carrying the instance `path`. A definition shared by several instances, or a + node that backs a promoted widget, is refused without writing. Replaying the + op needs a doc host on comfy-multi-player 0.3.12 or later: older appliers + ignore `path` and delete the top-level node with the same id. - `comfy workflow disconnect .` removes an input link and emits a replayable `disconnect` op. Concrete, promoted, widget-converted, autogrow, and eligible interior-subgraph inputs retain stable last-write-wins diff --git a/comfy_cli/command/workflow_edit.py b/comfy_cli/command/workflow_edit.py index 5b938108b..0f4acb7cb 100644 --- a/comfy_cli/command/workflow_edit.py +++ b/comfy_cli/command/workflow_edit.py @@ -480,7 +480,7 @@ def disconnect_cmd( @tracking.track_command("workflow") def delete_cmd( file: Annotated[str, typer.Argument(help="Frontend-format workflow JSON.")], - node: Annotated[str, typer.Argument(help="Node id to delete.")], + node: Annotated[str, typer.Argument(help="Node id or interior path to delete (for example 70/22).")], actor: ActorOpt = "cli", base_version: BaseVersionOpt = 0, stdout: StdoutOpt = False, @@ -517,7 +517,7 @@ def delete_cmd( @tracking.track_command("workflow") def delete_nodes_cmd( file: Annotated[str, typer.Argument(help="Frontend-format workflow JSON.")], - nodes: Annotated[list[str], typer.Argument(help="Node ids to delete (one or more).")], + nodes: Annotated[list[str], typer.Argument(help="Node ids or interior paths to delete (one or more).")], actor: ActorOpt = "cli", base_version: BaseVersionOpt = 0, stdout: StdoutOpt = False, diff --git a/comfy_cli/skills/comfy/SKILL.md b/comfy_cli/skills/comfy/SKILL.md index cb40d28cd..ccb049580 100644 --- a/comfy_cli/skills/comfy/SKILL.md +++ b/comfy_cli/skills/comfy/SKILL.md @@ -483,10 +483,14 @@ comfy --json download --out-dir ./out < run.json # pull the in place. - **`delete-node` ≠ `delete`:** `delete-node` removes a *node from the graph file*; `comfy workflow delete` deletes a *saved workflow from Comfy Cloud*. Do not confuse them. -- `add-node`/`connect`/`delete-node` operate on **top-level** nodes only. - `set-widget` additionally resolves values **inside a subgraph** directly — use - the flat promoted address `slots` advertises (e.g. `57.text`) or the nested - form (`57/27.text`); no decompose needed. +- `add-node` operates on **top-level** nodes only. `connect` (between two + nodes of the same subgraph) and `delete-node`/`delete-nodes` also accept the + interior addresses `workflow print` shows (e.g. `70/22`), but only inside a + subgraph definition used by a single instance; a shared definition is + refused rather than edited in every instance. `set-widget` additionally + resolves values **inside a subgraph** directly — use the flat promoted + address `slots` advertises (e.g. `57.text`) or the nested form + (`57/27.text`); no decompose needed. - **`set-node-field` writes one durable, per-node scalar field** — `title`, `mode`, `flags.collapsed` or `flags.pinned` — (or `--clear`s it back to absent). It needs no catalog — none of those four is a widget — and is the diff --git a/comfy_cli/workflow_ops.py b/comfy_cli/workflow_ops.py index 08da921fd..df792a16c 100644 --- a/comfy_cli/workflow_ops.py +++ b/comfy_cli/workflow_ops.py @@ -231,11 +231,11 @@ def __init__(self, node_id: Any, class_type: str, widget: str, *, interior: bool #: delete_node accepts (not the spelling the caller typed). self.node_id = node_id if interior: - # delete_node only removes top-level nodes, and insert_workflow only - # adds at the root, so there is no command route to replace it. + # insert_workflow only adds at the root, so even where delete_node + # can remove an interior note there is no command route to replace it. self.hint = ( - f"{node_id} is a note inside a subgraph definition: delete-node only removes top-level nodes, " - "so its text can only be changed in the ComfyUI editor" + f"{node_id} is a note inside a subgraph definition: no command can insert a replacement inside " + "a subgraph, so its text can only be changed in the ComfyUI editor" ) else: self.hint = ( @@ -1343,6 +1343,27 @@ def _navigate_subgraph_path(workflow: dict, segments: list[str]) -> dict: return node +def _interior_path_deleted(workflow: dict, segments: list[str]) -> bool: + """True when ``segments`` stops at an interior node that is absent from an + existing definition — the shape an interior ``delete_node`` leaves behind, + as opposed to a path that was never valid.""" + from comfy_cli.cql import engine as _engine + + defs_by_id = _engine._subgraph_defs_by_id(workflow) + node = _find_by_str(workflow, segments[0]) + for seg in segments[1:]: + sg = defs_by_id.get(str((node or {}).get("type", ""))) if isinstance(node, dict) else None + if sg is None: + return False + node = next( + (n for n in sg.get("nodes") or [] if isinstance(n, dict) and str(n.get("id", "")) == str(seg)), + None, + ) + if node is None: + return True + return False + + def _subgraph_boundary_error(workflow: dict, node_id: Any) -> ValueError | None: """Explain a connect endpoint that addresses a subgraph interior. @@ -1771,6 +1792,76 @@ def _interior_disconnect_scope(workflow: dict, to_node: Any) -> dict | None: return {"path": [str(seg) for seg in path], "definition": definition, "dst": dst} +def _interior_delete_scope(workflow: dict, node_id: Any) -> dict | None: + """Resolve an interior node delete to its owning single-instance definition.""" + from comfy_cli.cql import engine as _engine + + text = str(node_id) + if _find_by_str(workflow, text) is not None: + return None + 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 + path = segments[:-1] + try: + host = _navigate_subgraph_path(workflow, path) + except ValueError: + return None + definition = _engine._subgraph_defs_by_id(workflow).get(str(host.get("type", ""))) + if definition is None: + return None + target = next( + ( + node + for node in definition.get("nodes") or [] + if isinstance(node, dict) and str(node.get("id")) == str(segments[-1]) + ), + None, + ) + if target is None: + return None + instances = _definition_instance_count(workflow, str(host.get("type", ""))) + if instances > 1: + raise ValueError( + f"subgraph {'/'.join(path)} shares its definition {definition.get('id')} with " + f"{instances} instances, so deleting a node inside it would mutate all {instances} at once — " + "delete-node refuses that; edit a single-instance copy (unpack or duplicate the subgraph in the editor)" + ) + # A promoted widget's value lives positionally in the host's + # widgets_values (or, for a legacy promotion, in proxyWidgets order). + # Deleting its source would shift every later value onto the wrong input, + # and neither this CLI nor the shared applier realigns the host, so refuse. + from comfy_cli.cql import promoted as _promoted + + target_id = str(target.get("id")) + backed = [ + p.name + for p in _promoted.promoted_inputs(definition, _engine._subgraph_defs_by_id(workflow)) + if p.is_widget and str(p.source_node) == target_id + ] + backed += [ + str(entry[1]) + for entry in (host.get("properties") or {}).get("proxyWidgets") or [] + if isinstance(entry, list) and len(entry) >= 2 and str(entry[0]) == target_id + ] + if backed: + raise ValueError( + f"node {'/'.join([*path, target_id])} backs the promoted widget(s) {sorted(set(backed))} of subgraph " + f"{'/'.join(path)}; deleting it would shift the subgraph's remaining widget values onto the wrong " + "inputs — delete-node refuses that; remove the promotion in the editor first" + ) + return { + "path": [str(seg) for seg in path], + "definition": definition, + "target": target, + } + + def _definition_instance_count(workflow: dict, def_id: str) -> int: """How many live instances of definition ``def_id`` the workflow holds. @@ -2300,6 +2391,24 @@ def _delete_node_impl( actor: str = "cli", base_version: int = 0, ) -> tuple[dict, dict]: + interior = _interior_delete_scope(workflow, node_id) + if interior is not None: + target_id = interior["target"]["id"] + removed = [ + _interior_link_id(link) + for link in interior["definition"].get("links") or [] + if str(_interior_link_endpoints(link)[0]) == str(target_id) + or str(_interior_link_endpoints(link)[1]) == str(target_id) + ] + op = _new_op( + "delete_node", + actor, + base_version, + path=interior["path"], + node_id=target_id, + removed_links=removed, + ) + return apply_op(workflow, op, graph), op _require(workflow, node_id) removed = [ln[0] for ln in workflow.get("links") or [] if ln[1] == node_id or ln[3] == node_id] op = _new_op("delete_node", actor, base_version, node_id=node_id, removed_links=removed) @@ -3056,7 +3165,12 @@ def _apply_set_widget(workflow: dict, op: dict, graph) -> bool | None: # Validate against the current definition before the mutating resolver # forks shared ancestors. Imported/replayed malformed ops must be # atomic: a rejected widget cannot leave an unreachable fork behind. - preflight = _navigate_subgraph_path(workflow, [str(s) for s in path]) + try: + preflight = _navigate_subgraph_path(workflow, [str(s) for s in path]) + except ValueError: + if _interior_path_deleted(workflow, [str(s) for s in path]): + return # interior node concurrently deleted => no-op (delete wins) + raise preflight_warnings = _engine._write_widget( copy.deepcopy(preflight), op["inner_widget"], op["value"], graph, extend=False ) @@ -3557,6 +3671,9 @@ def _apply_interior_disconnect(workflow: dict, op: dict) -> None: def _apply_delete_node(workflow: dict, op: dict) -> None: + if op.get("path"): + _apply_interior_delete_node(workflow, op) + return node_id = str(op["node_id"]) # node identity is compared as a string (amendment v1.2) workflow["nodes"] = [n for n in workflow.get("nodes") or [] if str(n.get("id")) != node_id] removed = set(op.get("removed_links") or []) @@ -3583,6 +3700,54 @@ def _apply_delete_node(workflow: dict, op: dict) -> None: workflow.pop("_pending_dynamic_connects", None) +def _apply_interior_delete_node(workflow: dict, op: dict) -> None: + """Apply a path-scoped delete inside one single-instance definition.""" + from comfy_cli.cql import engine as _engine + + try: + host = _navigate_subgraph_path(workflow, [str(seg) for seg in op["path"]]) + except ValueError: + return + definition = _engine._subgraph_defs_by_id(workflow).get(str(host.get("type", ""))) + if definition is None or _definition_instance_count(workflow, str(host.get("type", ""))) > 1: + return + node_id = str(op["node_id"]) + definition["nodes"] = [ + node + for node in definition.get("nodes") or [] + if not (isinstance(node, dict) and str(node.get("id")) == node_id) + ] + removed = {str(link_id) for link_id in op.get("removed_links") or []} + kept = [] + for link in definition.get("links") or []: + origin_id, target_id, _ = _interior_link_endpoints(link) + if str(_interior_link_id(link)) in removed or str(origin_id) == node_id or str(target_id) == node_id: + removed.add(str(_interior_link_id(link))) + continue + kept.append(link) + definition["links"] = kept + kept_ids = {str(_interior_link_id(link)) for link in kept} + # Links from the input proxy / into the output proxy are also listed on the + # definition's boundary slots; a stale id there leaves the slot "connected" + # to a link that no longer exists. + for boundary in [*(definition.get("inputs") or []), *(definition.get("outputs") or [])]: + if isinstance(boundary, dict) and isinstance(boundary.get("linkIds"), list): + boundary["linkIds"] = [link_id for link_id in boundary["linkIds"] if str(link_id) not in removed] + for node in definition.get("nodes") or []: + if not isinstance(node, dict): + continue + for inp in node.get("inputs") or []: + if isinstance(inp, dict) and inp.get("link") is not None and str(inp["link"]) not in kept_ids: + inp["link"] = None + for out in node.get("outputs") or []: + if isinstance(out, dict) and isinstance(out.get("links"), list): + out["links"] = [link_id for link_id in out["links"] if str(link_id) in kept_ids] + if isinstance(definition.get("node_order"), list): + definition["node_order"] = [value for value in definition["node_order"] if str(value) != node_id] + if isinstance(definition.get("link_order"), list): + definition["link_order"] = [value for value in definition["link_order"] if str(value) in kept_ids] + + def _apply_clear(workflow: dict, op: dict) -> None: workflow["nodes"] = [] workflow["links"] = [] @@ -3649,6 +3814,8 @@ def _write_target(op: dict) -> tuple: # neither collides with a same-named widget's register. return ("node_field", str(op["node_id"]), op["field"]) if kind in ("add_node", "delete_node"): + if op.get("path"): + return ("node", tuple(str(s) for s in op["path"]), str(op["node_id"])) return ("node", str(op["node_id"])) if kind in ("connect", "disconnect"): grow = op.get("grow") diff --git a/docs/op-vocabulary-v1.md b/docs/op-vocabulary-v1.md index 776be39ec..cad1cbaaf 100644 --- a/docs/op-vocabulary-v1.md +++ b/docs/op-vocabulary-v1.md @@ -181,9 +181,19 @@ Spec form: ``` Minted op fields: `node_id`, `removed_links` (ids of every link incident to the -node at mint time). Replay removes the node, drops incident links (both the -recorded ones and any link whose endpoint is the node), and scrubs dangling -input/output references. +node at mint time). An interior address such as `70/22` additionally carries +the owner-instance `path` (`["70"]`); it is refused when the definition is +shared by multiple live instances, or when the node is the source of a +promoted widget (its host value would shift onto the wrong input). Replay +removes the node, drops incident links (both the recorded ones and any link +whose endpoint is the node), and scrubs dangling input/output references in +that same graph scope — for an interior delete, including the definition's +boundary `inputs[].linkIds` and `outputs[].linkIds`. + +An interior delete reuses the `delete_node` kind, so a consumer that predates +`path` cannot reject it. comfy-multi-player 0.3.11 and earlier ignore `path`, +remove the top-level node with the same id, and report the op `applied`. Send +interior deletes only to a doc host on comfy-multi-player 0.3.12 or later. * Idempotency: `op_id` no-op; replaying a delete of an already-absent node is a no-op. @@ -556,8 +566,14 @@ Current contract, pinned: a promoted-widget target is still rejected with "promoted widget (a value), not a link input". Both operations refuse a shared definition rather than silently rewiring every instance that uses it. -* **`add_node` and `delete_node` cannot address interior nodes** at all; an - interior id fails as node-not-found against the top-level inventory. +* **`delete_node` permits an interior node in one supported, single-instance + definition.** Its op carries the owner-instance `path`; its node-presence + identity is scoped by that path, and replay removes incident definition + links plus their endpoint references. It refuses a shared definition rather + than deleting from every instance, and refuses a node that backs a promoted + widget. A later interior `set_widget` on the deleted node is a no-op (delete + wins). `add_node` still cannot address an interior location from the CLI + authoring surface. * An interior **widget** write to a shared definition forks the definition at apply time (`engine._isolate_shared_subgraph`): the definition is deep-copied under `"sg-" + sha256(def_id + "\x00" + instance_id)[:32]` — deterministic, diff --git a/tests/comfy_cli/command/test_connect_interior.py b/tests/comfy_cli/command/test_connect_interior.py index 5395029d6..6b07a6618 100644 --- a/tests/comfy_cli/command/test_connect_interior.py +++ b/tests/comfy_cli/command/test_connect_interior.py @@ -193,6 +193,122 @@ def test_interior_disconnect_op_replays_to_the_same_document(): assert workflow_ops._write_target(op) == ("input", ("70",), "2011", 1) +def test_delete_interior_node_removes_it_and_its_definition_links(tmp_path, capsys): + import json + + base = _workflow() + base["definitions"]["subgraphs"][0]["links"].append( + {"id": 9002, "origin_id": 2012, "origin_slot": 1, "target_id": 2011, "target_slot": 0, "type": "CLIP"} + ) + _node(base, 2012)["outputs"][1]["links"] = [9002] + _node(base, 2011)["inputs"][0]["link"] = 9002 + path = _write(tmp_path, base) + + env = _run(["delete-node", str(path), "70/2012"], capsys) + + assert env["ok"] is True, env + op = env["data"]["op"] + assert op["path"] == ["70"] + assert op["node_id"] == 2012 + assert op["removed_links"] == [9002] + result = json.loads(path.read_text()) + assert [node["id"] for node in _sg(result)["nodes"]] == [2005, 2011] + assert [link["id"] for link in _sg(result)["links"]] == [9001] + assert _node(result, 2011)["inputs"][0]["link"] is None + assert result["nodes"][0]["id"] == 70 + + +def test_delete_interior_node_scrubs_its_links_from_the_boundary_slots(tmp_path, capsys): + import json + + base = _workflow() + sg = _sg(base) + sg["inputs"] = [{"id": "in-clip", "name": "clip", "type": "CLIP", "linkIds": [9101, 9103]}] + sg["outputs"] = [{"id": "out-cond", "name": "CONDITIONING", "type": "CONDITIONING", "linkIds": [9102]}] + sg["links"] += [ + {"id": 9101, "origin_id": -10, "origin_slot": 0, "target_id": 2011, "target_slot": 0, "type": "CLIP"}, + {"id": 9102, "origin_id": 2011, "origin_slot": 0, "target_id": -20, "target_slot": 0, "type": "CONDITIONING"}, + # The same boundary input also feeds a surviving node; that id must stay. + {"id": 9103, "origin_id": -10, "origin_slot": 0, "target_id": 2005, "target_slot": 0, "type": "CLIP"}, + ] + _node(base, 2011)["inputs"][0]["link"] = 9101 + _node(base, 2011)["outputs"][0]["links"] = [9102] + _node(base, 2005)["inputs"][0]["link"] = 9103 + path = _write(tmp_path, base) + + env = _run(["delete-node", str(path), "70/2011"], capsys) + + assert env["ok"] is True, env + result = _sg(json.loads(path.read_text())) + assert result["inputs"][0]["linkIds"] == [9103] + assert result["outputs"][0]["linkIds"] == [] + assert [link["id"] for link in result["links"]] == [9103] + replayed = workflow_ops.apply_op(copy.deepcopy(base), env["data"]["op"], _graph()) + assert _sg(replayed)["inputs"] == result["inputs"] + assert _sg(replayed)["outputs"] == result["outputs"] + + +def test_delete_interior_node_refuses_a_promoted_widget_source_without_writing(tmp_path, capsys): + base = _workflow() + sg = _sg(base) + sg["inputs"] = [{"id": "in-text", "name": "prompt", "type": "STRING", "linkIds": [9101]}] + sg["links"].append( + {"id": 9101, "origin_id": -10, "origin_slot": 0, "target_id": 2011, "target_slot": 1, "type": "STRING"} + ) + _node(base, 2011)["inputs"][1]["link"] = 9101 + legacy = copy.deepcopy(_workflow()) + legacy["nodes"][0]["properties"] = {"proxyWidgets": [["2012", "ckpt_name"]]} + + for workflow, target in ((base, "70/2011"), (legacy, "70/2012")): + path = _write(tmp_path, workflow) + before = path.read_bytes() + + env = _run(["delete-node", str(path), target], capsys) + + assert env["ok"] is False, env + assert "backs the promoted widget" in env["error"]["message"] + assert path.read_bytes() == before + + +def test_interior_set_widget_after_interior_delete_is_a_no_op_in_either_order(): + base = _workflow() + graph = _graph() + _, delete = workflow_ops.delete_node(copy.deepcopy(base), graph, "70/2012", actor="a", base_version=1) + _, write = workflow_ops.set_widget(copy.deepcopy(base), graph, "70/2012", "ckpt_name", "b.safetensors", actor="b") + + results = [] + for ops in ((delete, write), (write, delete)): + doc = copy.deepcopy(base) + for op in ops: + workflow_ops.apply_op(doc, copy.deepcopy(op), graph) + results.append(doc["definitions"]) + + assert results[0] == results[1] + assert [node["id"] for node in _sg({"definitions": results[0]})["nodes"]] == [2005, 2011] + + +def test_interior_delete_op_replays_to_the_same_document(): + base = _workflow() + graph = _graph() + out, op = workflow_ops.delete_node(copy.deepcopy(base), graph, "70/2012", actor="agent", base_version=4) + + replayed = workflow_ops.apply_op(copy.deepcopy(base), copy.deepcopy(op), graph) + + assert replayed["definitions"] == out["definitions"] + assert workflow_ops._write_target(op) == ("node", ("70",), "2012") + + +def test_delete_interior_node_refuses_a_shared_definition_without_writing(tmp_path, capsys): + path = _write(tmp_path, _workflow(instances=2)) + before = path.read_bytes() + + env = _run(["delete-node", str(path), "70/2012"], capsys) + + assert env["ok"] is False + assert "shares its definition" in env["error"]["message"] + assert path.read_bytes() == before + + def test_interior_grown_reconnect_and_disconnect_share_path_register(): base = _workflow() _node(base, 2011)["inputs"][1]["grow_id"] = 7001