mirror of
https://github.com/langgenius/dify.git
synced 2026-10-04 13:47:55 +08:00
fix(dify-builder): ground model on all model-bearing nodes, carry branch handles, clean test lint
Final consolidated fix wave from whole-branch review of the Real Build +
Edit Cognition slice:
- _ground now overwrites the model dict on ANY create_node whose config
carries a model dict (llm, question-classifier, parameter-extractor),
not just "llm", so a drifted/hallucinated provider/name on those other
node types no longer flows through ungrounded.
- to_intents now carries a generator edge's sourceHandle/targetHandle
into the connect intent's args (source_handle/target_handle) so
branching nodes (if-else, question-classifier) wire onto the correct
port instead of collapsing onto the default handle; edges without
handles still produce bare {from_node, to_node} args.
- Cleared the 17 ruff errors (PT018/E302/E306/E501) across the slice's
test files by splitting composite asserts, adding missing blank
lines, and wrapping long lines -- no assertions weakened, no noqa.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
a07dc0e766
commit
cd5fb6bbf0
7 files changed
+130
-18
No files matched your search
@@ -68,7 +68,8 @@ def test_build_form_fields_whitelists_and_coerces_type():
|
||||
]
|
||||
)
|
||||
assert [f.key for f in fields] == ["a", "b"]
|
||||
assert fields[0].type == "select" and fields[0].options == ["x"]
|
||||
assert fields[0].type == "select"
|
||||
assert fields[0].options == ["x"]
|
||||
assert fields[1].type == "text" # unknown type coerced to text
|
||||
|
||||
|
||||
@@ -416,7 +417,8 @@ def test_governance_automatic_learns_and_reaches_complete():
|
||||
res = handle_governance_feedback(env, Turn(actor=_actor()), *repo.get_session(s.id))
|
||||
assert res.next == PcState.BUILD_COMPLETE
|
||||
kinds = [i.kind for i in res.items]
|
||||
assert "build_learning" in kinds and "summary" in kinds
|
||||
assert "build_learning" in kinds
|
||||
assert "summary" in kinds
|
||||
assert "notice" in kinds
|
||||
assert env.agent.learn_calls == 1 # seam called for automatic
|
||||
|
||||
@@ -454,7 +456,8 @@ def test_governance_ask_rests_at_await_learning_with_pending_card():
|
||||
res = handle_governance_feedback(env, Turn(actor=_actor()), *repo.get_session(s.id))
|
||||
assert res.next == PcState.BUILD_AWAIT_LEARNING
|
||||
bl = [i for i in res.items if i.kind == "build_learning"][0]
|
||||
assert bl.payload["policy"] == "ask" and bl.payload["state"] == "pending"
|
||||
assert bl.payload["policy"] == "ask"
|
||||
assert bl.payload["state"] == "pending"
|
||||
assert getattr(env.agent, "learn_calls", 0) == 0 # not learned until accepted
|
||||
|
||||
|
||||
@@ -474,7 +477,8 @@ def test_await_learning_accept_learns_and_completes():
|
||||
)
|
||||
assert res.next == PcState.BUILD_COMPLETE
|
||||
kinds = [i.kind for i in res.items]
|
||||
assert "decision" in kinds and "summary" in kinds
|
||||
assert "decision" in kinds
|
||||
assert "summary" in kinds
|
||||
assert "notice" in kinds
|
||||
assert env.agent.learn_calls == 1
|
||||
|
||||
|
||||
@@ -195,4 +195,5 @@ def test_placeholder_learn_from_build_returns_descriptor():
|
||||
from core.dify_builder.placeholder_agent import PlaceholderAgent
|
||||
|
||||
out = PlaceholderAgent().learn_from_build("summarize a PDF", {}, ["step"], ["llm"])
|
||||
assert isinstance(out, str) and out != ""
|
||||
assert isinstance(out, str)
|
||||
assert out != ""
|
||||
@@ -73,6 +73,51 @@ def test_build_nodes_degrades_to_empty_on_generator_error():
|
||||
assert build.build_nodes("t1", {}, ["x"]) == []
|
||||
|
||||
|
||||
def test_build_nodes_grounds_model_on_question_classifier_node():
|
||||
gen_graph_qc = {
|
||||
"graph": {
|
||||
"nodes": [
|
||||
{"id": "s", "type": "custom", "data": {"type": "start", "title": "Start", "variables": []}},
|
||||
{
|
||||
"id": "qc1",
|
||||
"type": "custom",
|
||||
"data": {
|
||||
"type": "question-classifier",
|
||||
"title": "Classify",
|
||||
"model": {"provider": "wrong", "name": "wrong"},
|
||||
"classes": [],
|
||||
},
|
||||
},
|
||||
{"id": "e", "type": "custom", "data": {"type": "end", "title": "End", "outputs": []}},
|
||||
],
|
||||
"edges": [
|
||||
{"id": "e1", "source": "s", "target": "qc1"},
|
||||
{"id": "e2", "source": "qc1", "target": "e"},
|
||||
],
|
||||
},
|
||||
"error": "",
|
||||
"errors": [],
|
||||
}
|
||||
with (
|
||||
patch.object(build, "_generator_model_config", return_value=_fake_mc("anthropic", "claude-opus-4-8")),
|
||||
patch(
|
||||
"services.dify_builder.agent.build.WorkflowGeneratorService.generate_workflow_graph",
|
||||
return_value=gen_graph_qc,
|
||||
),
|
||||
):
|
||||
intents = build.build_nodes("t1", {}, ["Classify the request"])
|
||||
|
||||
by_type = {}
|
||||
for intent in intents:
|
||||
if intent.op == "create_node":
|
||||
by_type[intent.args["node_type"]] = intent.args["config"]
|
||||
assert by_type["question-classifier"]["model"]["provider"] == "anthropic" # ungrounded model overwritten
|
||||
assert by_type["question-classifier"]["model"]["name"] == "claude-opus-4-8"
|
||||
# no fabricated params: the fixture's node carried no completion_params, and none
|
||||
# should be synthesized (e.g. a guessed temperature) during grounding.
|
||||
assert "completion_params" not in by_type["question-classifier"]["model"]
|
||||
|
||||
|
||||
def test_build_nodes_dataset_ids_are_independent_lists_per_node():
|
||||
gen_graph_two_kb = {
|
||||
"graph": {
|
||||
|
||||
@@ -15,14 +15,17 @@ def test_to_intents_translates_nodes_then_edges_and_normalizes_start():
|
||||
assert ops == [("create_node", "start"), ("create_node", "n_llm"), ("connect", ("start", "n_llm"))]
|
||||
# config carries data minus the type key; create_node args expose node_type
|
||||
start = intents[0]
|
||||
assert start.args["node_type"] == "start" and "type" not in start.args["config"]
|
||||
assert start.args["node_type"] == "start"
|
||||
assert "type" not in start.args["config"]
|
||||
assert start.args["config"]["title"] == "Start"
|
||||
|
||||
|
||||
def test_to_intents_skips_malformed_nodes():
|
||||
from services.dify_builder.agent.graph_translate import to_intents
|
||||
graph = {"nodes": [{"id": "x", "data": {}}, {"id": "", "data": {"type": "llm"}}], "edges": []}
|
||||
assert to_intents(graph) == [] # no type / no id -> dropped
|
||||
|
||||
|
||||
def test_to_intents_multiple_start_nodes_only_first_normalized():
|
||||
from services.dify_builder.agent.graph_translate import to_intents
|
||||
# Graph with two start-type nodes: only first should be renamed to "start", second keeps original id
|
||||
@@ -59,3 +62,36 @@ def test_to_intents_multiple_start_nodes_only_first_normalized():
|
||||
assert connects[0].args["from_node"] == "start", "Edge from s1 should be remapped to 'start'"
|
||||
# Second edge should connect from "s2" (not remapped) to "n_llm"
|
||||
assert connects[1].args["from_node"] == "s2", "Edge from s2 should keep 's2'"
|
||||
|
||||
|
||||
def test_to_intents_carries_branch_handles_through_connect():
|
||||
from services.dify_builder.agent.graph_translate import to_intents
|
||||
|
||||
graph = {
|
||||
"nodes": [
|
||||
{"id": "if1", "type": "custom", "data": {"type": "if-else", "title": "If"}},
|
||||
{"id": "n_true", "type": "custom", "data": {"type": "llm", "title": "True branch"}},
|
||||
],
|
||||
"edges": [
|
||||
{"id": "e1", "source": "if1", "target": "n_true", "sourceHandle": "true", "targetHandle": "target"},
|
||||
],
|
||||
}
|
||||
intents = to_intents(graph)
|
||||
connect = next(i for i in intents if i.op == "connect")
|
||||
assert connect.args["source_handle"] == "true"
|
||||
assert connect.args["target_handle"] == "target"
|
||||
|
||||
|
||||
def test_to_intents_connect_without_handles_has_no_handle_keys():
|
||||
from services.dify_builder.agent.graph_translate import to_intents
|
||||
|
||||
graph = {
|
||||
"nodes": [
|
||||
{"id": "n1", "type": "custom", "data": {"type": "start", "title": "Start"}},
|
||||
{"id": "n2", "type": "custom", "data": {"type": "llm", "title": "LLM"}},
|
||||
],
|
||||
"edges": [{"id": "e1", "source": "n1", "target": "n2"}],
|
||||
}
|
||||
intents = to_intents(graph)
|
||||
connect = next(i for i in intents if i.op == "connect")
|
||||
assert connect.args == {"from_node": "start", "to_node": "n2"}
|
||||
@@ -6,19 +6,27 @@ def test_list_tenant_resources_normalizes_all_three_kinds():
|
||||
|
||||
class _Model:
|
||||
def __init__(self, m): self.model = m
|
||||
|
||||
class _ProvResp:
|
||||
def __init__(self, p, ms): self.provider, self.models = p, [_Model(m) for m in ms]
|
||||
|
||||
class _DS:
|
||||
def __init__(self, i, n): self.id, self.name = i, n
|
||||
|
||||
tool_record = {
|
||||
"provider_name": "google", "tool_name": "search",
|
||||
"tool_label": "Google Search", "description": "",
|
||||
}
|
||||
with patch.object(resources, "_list_models", return_value=[_ProvResp("anthropic", ["claude-opus-4-8"])]), \
|
||||
patch.object(resources, "_list_datasets", return_value=[_DS("kb-1", "Company KB")]), \
|
||||
patch.object(resources, "_list_tools", return_value=[{"provider_name": "google", "tool_name": "search", "tool_label": "Google Search", "description": ""}]):
|
||||
patch.object(resources, "_list_tools", return_value=[tool_record]):
|
||||
out = resources.list_tenant_resources("t1")
|
||||
|
||||
assert out.models[0].id == "anthropic/claude-opus-4-8"
|
||||
assert out.datasets[0].id == "kb-1" and out.datasets[0].label == "Company KB"
|
||||
assert out.tools[0].id == "google/search" and out.tools[0].label == "Google Search"
|
||||
assert out.datasets[0].id == "kb-1"
|
||||
assert out.datasets[0].label == "Company KB"
|
||||
assert out.tools[0].id == "google/search"
|
||||
assert out.tools[0].label == "Google Search"
|
||||
|
||||
|
||||
def test_each_source_degrades_to_empty():
|
||||
@@ -36,14 +44,20 @@ def test_mixed_failure_partial_success():
|
||||
class _DS:
|
||||
def __init__(self, i, n): self.id, self.name = i, n
|
||||
|
||||
tool_record = {
|
||||
"provider_name": "google", "tool_name": "search",
|
||||
"tool_label": "Google Search", "description": "",
|
||||
}
|
||||
with patch.object(resources, "_list_models", side_effect=RuntimeError("boom")), \
|
||||
patch.object(resources, "_list_datasets", return_value=[_DS("kb-1", "Company KB")]), \
|
||||
patch.object(resources, "_list_tools", return_value=[{"provider_name": "google", "tool_name": "search", "tool_label": "Google Search", "description": ""}]):
|
||||
patch.object(resources, "_list_tools", return_value=[tool_record]):
|
||||
out = resources.list_tenant_resources("t1")
|
||||
|
||||
assert out.models == []
|
||||
assert len(out.datasets) == 1 and out.datasets[0].id == "kb-1"
|
||||
assert len(out.tools) == 1 and out.tools[0].id == "google/search"
|
||||
assert len(out.datasets) == 1
|
||||
assert out.datasets[0].id == "kb-1"
|
||||
assert len(out.tools) == 1
|
||||
assert out.tools[0].id == "google/search"
|
||||
|
||||
|
||||
def test_malformed_tool_record_degrades():
|
||||
@@ -55,4 +69,5 @@ def test_malformed_tool_record_degrades():
|
||||
out = resources.list_tenant_resources("t1")
|
||||
|
||||
assert out.tools == []
|
||||
assert out.models == [] and out.datasets == []
|
||||
assert out.models == []
|
||||
assert out.datasets == []
|
||||
Reference in new issue
Block a user