diff --git a/api/services/dify_builder/graph_ops.py b/api/services/dify_builder/graph_ops.py index 445067983b4..670aa54e26f 100644 --- a/api/services/dify_builder/graph_ops.py +++ b/api/services/dify_builder/graph_ops.py @@ -37,8 +37,58 @@ def validate_intent_args(intent: MutationIntent) -> None: raise ValueError(f"missing required arg(s) {missing} for op {intent.op!r}") +def _resolve_path(container: dict[str, Any], path: str) -> tuple[Any, str | int]: + """Walk ``path``'s parent segments and return ``(parent, last_key)``. + + Segments are dot-separated; an all-digit segment indexes a list, any + other segment is a mapping key. Only the FINAL segment may be absent -- + that is the key being written. Every intermediate segment must already + resolve, and a segment that cannot be walked raises ``ValueError``. + + Raising is the point. ``filter_applicable`` dry-runs each intent through + this function and turns the exception into a rejection reason the repair + agent sees on its next attempt. Before this existed, a dotted path wrote + one flat junk key that the workflow engine ignored, so every nested + repair silently did nothing (ESQ1-285, ESQ1-290). + """ + segments = path.split(".") + if not path or any(segment == "" for segment in segments): + raise ValueError(f"empty segment in path: {path!r}") + + cursor: Any = container + for depth, segment in enumerate(segments[:-1]): + walked = ".".join(segments[: depth + 1]) + if segment.isdigit(): + if not isinstance(cursor, list): + raise ValueError(f"path {path!r}: {walked} indexes a {type(cursor).__name__}, not a list") + index = int(segment) + if index >= len(cursor): + raise ValueError(f"path {path!r}: index {index} out of range at {walked} (len {len(cursor)})") + cursor = cursor[index] + continue + if not isinstance(cursor, dict): + raise ValueError(f"path {path!r}: {walked} is not a mapping") + if segment not in cursor: + raise ValueError(f"path {path!r}: no key {segment!r} at {walked}") + cursor = cursor[segment] + + last = segments[-1] + if last.isdigit(): + if not isinstance(cursor, list): + raise ValueError(f"path {path!r}: final segment indexes a {type(cursor).__name__}, not a list") + index = int(last) + if index >= len(cursor): + raise ValueError(f"path {path!r}: index {index} out of range (len {len(cursor)})") + return cursor, index + if not isinstance(cursor, dict): + raise ValueError(f"path {path!r}: final container is a {type(cursor).__name__}, not a mapping") + return cursor, last + + def apply_set_node_config(graph: Graph, node_id: str, path: str, value: Any) -> tuple[Graph, list[str]]: - """Set ``node["data"][path] = value`` for the node whose ``id == node_id``. + """Set the value at ``path`` inside ``node["data"]`` for the node whose + ``id == node_id``. ``path`` is dot-separated (``"code"``, + ``"cases.0.conditions.1.value"``); see ``_resolve_path``. The placeholder agent emits intents shaped ``{node_id, path, value}`` (e.g. ``path="code"`` to rewrite a Code node's ``data["code"]``); this is @@ -52,7 +102,8 @@ def apply_set_node_config(graph: Graph, node_id: str, path: str, value: Any) -> for node in new_graph.get("nodes", []): if node.get("id") == node_id: - node.setdefault("data", {})[path] = value + parent, key = _resolve_path(node.setdefault("data", {}), path) + parent[key] = value return new_graph, [node_id] raise ValueError(f"node not found: {node_id}") diff --git a/api/tests/unit_tests/services/dify_builder/test_graph_ops.py b/api/tests/unit_tests/services/dify_builder/test_graph_ops.py index daae3c6de98..7113086dae5 100644 --- a/api/tests/unit_tests/services/dify_builder/test_graph_ops.py +++ b/api/tests/unit_tests/services/dify_builder/test_graph_ops.py @@ -556,3 +556,92 @@ def test_filter_rejects_insert_between_with_non_dict_edge(): ok, bad = graph_ops.filter_applicable(_G, intents, allowed_node_types={"llm", "end"}) assert ok == [] assert len(bad) == 1 # rejected with a reason, not raised + + +def _ifelse_graph(): + return { + "nodes": [ + { + "id": "node5", + "data": { + "type": "if-else", + "title": "Review Decision", + "cases": [ + { + "case_id": "true", + "conditions": [ + {"variable_selector": ["node4", "score"], "comparison_operator": ">", "value": "0"}, + {"variable_selector": ["node4", "text"], "comparison_operator": ">", "value": "5"}, + ], + } + ], + }, + } + ], + "edges": [], + } + + +def test_set_node_config_writes_through_a_dotted_path(): + graph = _ifelse_graph() + out, changed = apply_set_node_config(graph, "node5", "cases.0.conditions.1.comparison_operator", "contains") + assert changed == ["node5"] + condition = out["nodes"][0]["data"]["cases"][0]["conditions"][1] + assert condition["comparison_operator"] == "contains" + # the sibling condition is untouched + assert out["nodes"][0]["data"]["cases"][0]["conditions"][0]["comparison_operator"] == ">" + # and no junk flat key was created + assert "cases.0.conditions.1.comparison_operator" not in out["nodes"][0]["data"] + + +def test_set_node_config_still_writes_a_top_level_key(): + graph = {"nodes": [{"id": "n1", "data": {"type": "code"}}], "edges": []} + out, changed = apply_set_node_config(graph, "n1", "code", "print(1)") + assert changed == ["n1"] + assert out["nodes"][0]["data"]["code"] == "print(1)" + + +def test_set_node_config_creates_a_missing_top_level_key(): + """Unchanged behaviour: the LAST segment may be new -- that is the write.""" + graph = {"nodes": [{"id": "n1", "data": {"type": "code"}}], "edges": []} + out, _changed = apply_set_node_config(graph, "n1", "outputs", {"result": {"type": "string"}}) + assert out["nodes"][0]["data"]["outputs"] == {"result": {"type": "string"}} + + +def test_set_node_config_rejects_a_missing_intermediate_key(): + graph = {"nodes": [{"id": "n1", "data": {"type": "code"}}], "edges": []} + with pytest.raises(ValueError, match="no key 'cases'"): + apply_set_node_config(graph, "n1", "cases.0.value", "x") + + +def test_set_node_config_rejects_an_out_of_range_index(): + graph = _ifelse_graph() + with pytest.raises(ValueError, match="out of range"): + apply_set_node_config(graph, "node5", "cases.0.conditions.7.value", "x") + + +def test_set_node_config_rejects_an_index_into_a_mapping(): + graph = _ifelse_graph() + with pytest.raises(ValueError, match="not a list"): + apply_set_node_config(graph, "node5", "cases.0.0", "x") + + +def test_set_node_config_rejects_an_empty_segment(): + graph = _ifelse_graph() + with pytest.raises(ValueError, match="empty segment"): + apply_set_node_config(graph, "node5", "cases..0", "x") + + +def test_filter_applicable_rejects_a_bad_path_with_a_reason(): + """The whole point of raising: the repair agent gets corrective feedback.""" + from services.dify_builder.graph_ops import filter_applicable + + graph = _ifelse_graph() + intent = MutationIntent( + op="set_node_config", + args={"node_id": "node5", "path": "cases.0.conditions.9.value", "value": "x"}, + ) + applicable, rejected = filter_applicable(graph, [intent]) + assert applicable == [] + assert len(rejected) == 1 + assert "out of range" in rejected[0][1]