mirror of
https://github.com/langgenius/dify.git
synced 2026-10-04 13:47:55 +08:00
fix(dify-builder): walk dotted paths in apply_set_node_config
apply_set_node_config wrote node["data"][path] as a flat key, so every dotted path the repair agent emits (cases.0.conditions.1.value) created a junk key the workflow engine ignores. The dry run passed, the change set printed a plausible line, and the graph never changed -- which is why ESQ1-285 survived nine approvals and ESQ1-290 six. Paths now resolve through lists and mappings, and an unwalkable path raises. Raising is half the fix: filter_applicable dry-runs intents through the real apply_* and feeds rejection reasons back to the repair agent, so a bad path now produces corrective feedback instead of a silent no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
bf8333bc77
commit
7e82ab20d9
2 files changed
+142
-2
No files matched your search
@@ -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}")
|
||||
|
||||
@@ -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]
|
||||
Reference in new issue
Block a user