mirror of
https://github.com/langgenius/dify.git
synced 2026-10-04 13:47:55 +08:00
fix(dify-builder): an endpoint the user didn't supply becomes an input
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
a73c443ecd
commit
ea46a69040
9 files changed
+161
-59
No files matched your search
@@ -473,7 +473,14 @@ def handle_plan_approval(env: Env, turn: Turn, s: Session, fc: DifyBuilderContex
|
||||
# built nodes (not the Builder's session model). resource_ids were already
|
||||
# string-filtered when stored (handle_resource_recommendation).
|
||||
selected_resource_ids = list(fc.resource_selection.get("resource_ids") or [])
|
||||
build_result = env.agent.build_nodes(list(fc.plan_items), selected_resource_ids)
|
||||
# The text the user actually typed (goal + the requirements form as they
|
||||
# submitted it) -- core/dify_builder cannot import services, so this is
|
||||
# built inline, matching services.dify_builder.agent.user_supplied's
|
||||
# trusted_text_for byte-for-byte (see test_plan_approval_passes_trusted_
|
||||
# text_matching_user_supplied_trusted_text_for). Lets build_nodes tell an
|
||||
# endpoint the user actually gave from one the model invented (ESQ1-302/S5b).
|
||||
trusted_text = f"{fc.goal_text}\n{json.dumps(dict(fc.requirements), ensure_ascii=False)}"
|
||||
build_result = env.agent.build_nodes(list(fc.plan_items), selected_resource_ids, trusted_text=trusted_text)
|
||||
intents = build_result.intents
|
||||
|
||||
if not any(intent.op == "create_node" for intent in intents):
|
||||
|
||||
@@ -139,47 +139,51 @@ class PlaceholderAgent:
|
||||
"Emit the final report",
|
||||
]
|
||||
|
||||
def build_nodes(self, plan_items: list[str], resource_ids: list[str] | None = None) -> BuildNodesResult:
|
||||
def build_nodes(
|
||||
self, plan_items: list[str], resource_ids: list[str] | None = None, *, trusted_text: str = ""
|
||||
) -> BuildNodesResult:
|
||||
# Start -> Knowledge-Retrieval -> LLM -> End. Creates and connects are
|
||||
# interleaved so each connect's endpoints already exist when
|
||||
# apply_connect validates them.
|
||||
return BuildNodesResult(intents=[
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.START,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.START),
|
||||
"node_id": BUILD_START_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.KNOWLEDGE_RETRIEVAL,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.KNOWLEDGE_RETRIEVAL),
|
||||
"node_id": BUILD_KNOWLEDGE_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_START_ID, "to_node": BUILD_KNOWLEDGE_ID}),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.LLM,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.LLM),
|
||||
"node_id": BUILD_LLM_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_KNOWLEDGE_ID, "to_node": BUILD_LLM_ID}),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.END,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.END),
|
||||
"node_id": BUILD_END_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_LLM_ID, "to_node": BUILD_END_ID}),
|
||||
])
|
||||
return BuildNodesResult(
|
||||
intents=[
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.START,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.START),
|
||||
"node_id": BUILD_START_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.KNOWLEDGE_RETRIEVAL,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.KNOWLEDGE_RETRIEVAL),
|
||||
"node_id": BUILD_KNOWLEDGE_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_START_ID, "to_node": BUILD_KNOWLEDGE_ID}),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.LLM,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.LLM),
|
||||
"node_id": BUILD_LLM_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_KNOWLEDGE_ID, "to_node": BUILD_LLM_ID}),
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
args={
|
||||
"node_type": BuiltinNodeTypes.END,
|
||||
"config": node_defaults.default_config(BuiltinNodeTypes.END),
|
||||
"node_id": BUILD_END_ID,
|
||||
},
|
||||
),
|
||||
MutationIntent(op="connect", args={"from_node": BUILD_LLM_ID, "to_node": BUILD_END_ID}),
|
||||
]
|
||||
)
|
||||
|
||||
def learn_from_build(
|
||||
self,
|
||||
|
||||
@@ -91,7 +91,9 @@ class DifyBuilderAgent(Protocol):
|
||||
|
||||
def bind_resources(self, plan_items: list[str], resource_ids: list[str]) -> list[str]: ...
|
||||
|
||||
def build_nodes(self, plan_items: list[str], resource_ids: list[str] | None = None) -> BuildNodesResult: ...
|
||||
def build_nodes(
|
||||
self, plan_items: list[str], resource_ids: list[str] | None = None, *, trusted_text: str = ""
|
||||
) -> BuildNodesResult: ...
|
||||
|
||||
def learn_from_build(
|
||||
self,
|
||||
|
||||
@@ -618,6 +618,8 @@ def build_nodes(
|
||||
model_config: dict[str, Any],
|
||||
plan_items: list[str],
|
||||
resource_ids: Sequence[str] = (),
|
||||
*,
|
||||
trusted_text: str = "",
|
||||
) -> BuildNodesResult:
|
||||
try:
|
||||
mc = _generator_model_config(tenant_id, model_config)
|
||||
@@ -660,7 +662,7 @@ def build_nodes(
|
||||
# runtime model follows the user's choice.
|
||||
grounding_mc = _selected_workflow_model(tenant_id, resource_ids) or mc
|
||||
_ground(intents, grounding_mc, tenant_id, plan_items)
|
||||
for node_id in _ground_placeholder_endpoints(intents):
|
||||
for node_id in _ground_placeholder_endpoints(intents, trusted_text=trusted_text):
|
||||
logger.info("Dify Builder: http-request %s had a placeholder URL; now read from a start variable", node_id)
|
||||
applicable, rejected = graph_ops.filter_applicable({"nodes": [], "edges": []}, intents, _ALLOWED_NODE_TYPES)
|
||||
if not applicable:
|
||||
@@ -759,9 +761,9 @@ def _is_placeholder_endpoint(url: str) -> bool:
|
||||
return _PLACEHOLDER_URL_RE.search(text) is not None or _is_localhost_host(text)
|
||||
|
||||
|
||||
def _ground_placeholder_endpoints(intents: list[MutationIntent]) -> list[str]:
|
||||
"""Replace every http-request placeholder URL with a REQUIRED start-node
|
||||
variable and a template reference to it.
|
||||
def _ground_placeholder_endpoints(intents: list[MutationIntent], *, trusted_text: str = "") -> list[str]:
|
||||
"""Replace every http-request URL the user didn't supply with a REQUIRED
|
||||
start-node variable and a template reference to it.
|
||||
|
||||
ESQ1-302 called ``https://api.example.com/ppt/generate``: the model had no
|
||||
endpoint, so it invented one, and every test run failed against a host that
|
||||
@@ -769,7 +771,16 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent]) -> list[str]:
|
||||
the user for the real endpoint -- or, when they have none, tells them what
|
||||
the step needs. Builder-only; the shared generator's own http-request
|
||||
example is ``https://example.com``, so cmd+K keeps its placeholders.
|
||||
Returns the ids of the http-request nodes it re-pointed.
|
||||
|
||||
A URL is grounded when it LOOKS invented (``_is_placeholder_endpoint``) OR,
|
||||
with ``trusted_text`` non-empty, when its host never actually appears in
|
||||
what the user typed (``user_supplied.is_user_supplied_url``) -- a
|
||||
plausible, real-looking host the model invented (S5b:
|
||||
``api.pptrender.io``) is just as much a fabrication as
|
||||
``api.example.com``, only harder to spot by shape alone.
|
||||
``trusted_text == ""`` (no caller-supplied text) keeps exactly today's
|
||||
placeholder-only behaviour. Returns the ids of the http-request nodes it
|
||||
re-pointed.
|
||||
"""
|
||||
start = next((i for i in intents if i.op == "create_node" and i.args.get("node_type") == "start"), None)
|
||||
if start is None:
|
||||
@@ -784,7 +795,9 @@ def _ground_placeholder_endpoints(intents: list[MutationIntent]) -> list[str]:
|
||||
if intent.op != "create_node" or intent.args.get("node_type") != "http-request":
|
||||
continue
|
||||
node_config = dict(intent.args.get("config") or {})
|
||||
if not _is_placeholder_endpoint(str(node_config.get("url") or "")):
|
||||
url = str(node_config.get("url") or "")
|
||||
not_user_supplied = bool(trusted_text) and not user_supplied.is_user_supplied_url(url, trusted_text)
|
||||
if not (_is_placeholder_endpoint(url) or not_user_supplied):
|
||||
continue
|
||||
node_id = str(intent.args.get("node_id") or "")
|
||||
var_name = f"{node_id}_url"
|
||||
|
||||
@@ -133,8 +133,10 @@ class LlmBuilderAgent:
|
||||
def bind_resources(self, plan_items, resource_ids):
|
||||
return build.bind_resources(self.model_or_none(), self._tenant_id, plan_items, resource_ids)
|
||||
|
||||
def build_nodes(self, plan_items, resource_ids=None):
|
||||
return build.build_nodes(self._tenant_id, self._model_config, plan_items, resource_ids or ())
|
||||
def build_nodes(self, plan_items, resource_ids=None, *, trusted_text=""):
|
||||
return build.build_nodes(
|
||||
self._tenant_id, self._model_config, plan_items, resource_ids or (), trusted_text=trusted_text
|
||||
)
|
||||
|
||||
def learn_from_build(self, goal_text, requirements, plan_items, built_node_ids):
|
||||
return build.learn_from_build(
|
||||
|
||||
@@ -438,7 +438,7 @@ class StubAgent:
|
||||
def bind_resources(self, _plan_items, _resource_ids):
|
||||
return []
|
||||
|
||||
def build_nodes(self, _plan_items, _resource_ids=None):
|
||||
def build_nodes(self, _plan_items, _resource_ids=None, *, trusted_text=""): # noqa: ARG002
|
||||
return BuildNodesResult(intents=[])
|
||||
|
||||
def learn_from_build(self, _goal_text, _requirements, _plan_items, _built_node_ids):
|
||||
|
||||
@@ -365,6 +365,46 @@ def test_plan_approval_approve_builds_graph_and_reveals_nodes():
|
||||
assert len(assistant.payload["execution"]["activities"]) == 3
|
||||
|
||||
|
||||
def test_plan_approval_passes_trusted_text_matching_user_supplied_trusted_text_for():
|
||||
"""handle_plan_approval threads the user's own text (goal + submitted
|
||||
requirements) through to build_nodes as trusted_text, so
|
||||
_ground_placeholder_endpoints can tell an endpoint the user actually gave
|
||||
from one the model invented (ESQ1-302/S5b). core/dify_builder cannot
|
||||
import services, so the handler builds this string itself -- pinned here
|
||||
byte-identical to services.dify_builder.agent.user_supplied.trusted_text_for
|
||||
on the same input."""
|
||||
from core.dify_builder.handlers_build import handle_plan_approval
|
||||
from services.dify_builder.agent.user_supplied import trusted_text_for
|
||||
from tests.unit_tests.core.dify_builder.fakes import FakeBuildDifyPort
|
||||
|
||||
class _RecordingAgent(PlaceholderAgent):
|
||||
def __init__(self):
|
||||
self.build_nodes_calls: list[str] = []
|
||||
|
||||
def build_nodes(self, plan_items, resource_ids=None, *, trusted_text=""):
|
||||
self.build_nodes_calls.append(trusted_text)
|
||||
return super().build_nodes(plan_items, resource_ids)
|
||||
|
||||
agent = _RecordingAgent()
|
||||
env, repo = _new_env(dify=FakeBuildDifyPort(), agent=agent)
|
||||
s = _seed_build_session(
|
||||
repo,
|
||||
PcState.BUILD_PLAN_APPROVAL,
|
||||
plan_items=["Retrieve", "Summarize"],
|
||||
plan_version_tag="v1",
|
||||
requirements={"endpoint": "", "currency": "USD"},
|
||||
)
|
||||
session, fc = repo.get_session(s.id)
|
||||
turn = Turn(action=Action(kind="approve_repair", base_version=1), actor=_actor())
|
||||
handle_plan_approval(env, turn, session, fc)
|
||||
|
||||
assert len(agent.build_nodes_calls) == 1
|
||||
trusted_text = agent.build_nodes_calls[0]
|
||||
assert fc.goal_text in trusted_text
|
||||
assert "USD" in trusted_text
|
||||
assert trusted_text == trusted_text_for(fc.goal_text, fc.requirements)
|
||||
|
||||
|
||||
def test_plan_approval_ignores_non_approve_action():
|
||||
from core.dify_builder.handlers_build import handle_plan_approval
|
||||
from tests.unit_tests.core.dify_builder.fakes import FakeBuildDifyPort
|
||||
@@ -392,7 +432,7 @@ def test_plan_approval_empty_build_surfaces_error_and_keeps_canvas():
|
||||
}
|
||||
# generation produced nothing, WITH a specific reason (the real generator returns
|
||||
# e.g. "UNRESOLVED_REFERENCE: ..." / a provider error). The error card must carry it.
|
||||
env.agent.build_nodes = lambda _plan, _rids=None: BuildNodesResult(
|
||||
env.agent.build_nodes = lambda _plan, _rids=None, *, trusted_text="": BuildNodesResult( # noqa: ARG005
|
||||
intents=[], error="UNRESOLVED_REFERENCE: Reference {#node4.x#} not declared"
|
||||
)
|
||||
s = _session(entry_mode=EntryMode.BUILD, current_state=PcState.BUILD_PLAN_APPROVAL)
|
||||
@@ -431,7 +471,7 @@ def test_plan_approval_error_card_carries_diagnostics_into_the_item_payload():
|
||||
"errors": [{"code": "UNRESOLVED_REFERENCE", "detail": "...", "node_id": "node2"}],
|
||||
}
|
||||
]
|
||||
env.agent.build_nodes = lambda _plan, _rids=None: BuildNodesResult(
|
||||
env.agent.build_nodes = lambda _plan, _rids=None, *, trusted_text="": BuildNodesResult( # noqa: ARG005
|
||||
intents=[], error="Reference {#node2.response#} not declared on node 'node2'", diagnostics=diag
|
||||
)
|
||||
s = _session(entry_mode=EntryMode.BUILD, current_state=PcState.BUILD_PLAN_APPROVAL)
|
||||
@@ -459,7 +499,7 @@ def test_plan_approval_deletes_pre_existing_start_on_from_scratch_build():
|
||||
"edges": [],
|
||||
}
|
||||
# generator returns a graph whose start id is "node1" (a document variable)
|
||||
env.agent.build_nodes = lambda _plan, _rids=None: BuildNodesResult(
|
||||
env.agent.build_nodes = lambda _plan, _rids=None, *, trusted_text="": BuildNodesResult( # noqa: ARG005
|
||||
intents=[
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
@@ -503,7 +543,7 @@ def test_plan_approval_survives_generator_reusing_the_deleted_placeholder_start_
|
||||
"edges": [],
|
||||
}
|
||||
# generator reuses the SAME id ("start") for its own start node
|
||||
env.agent.build_nodes = lambda _plan, _rids=None: BuildNodesResult(
|
||||
env.agent.build_nodes = lambda _plan, _rids=None, *, trusted_text="": BuildNodesResult( # noqa: ARG005
|
||||
intents=[
|
||||
MutationIntent(
|
||||
op="create_node",
|
||||
@@ -2499,7 +2539,7 @@ def _stub_agent_building(intents):
|
||||
from tests.unit_tests.core.dify_builder.fakes import StubAgent
|
||||
|
||||
class _Agent(StubAgent):
|
||||
def build_nodes(self, _plan_items, _resource_ids=None):
|
||||
def build_nodes(self, _plan_items, _resource_ids=None, *, trusted_text=""): # noqa: ARG002
|
||||
return BuildNodesResult(intents=list(intents))
|
||||
|
||||
return _Agent()
|
||||
|
||||
@@ -551,7 +551,7 @@ def _gen_graph_with_http(url: str) -> dict:
|
||||
}
|
||||
|
||||
|
||||
def _build_with(gen_graph: dict):
|
||||
def _build_with(gen_graph: dict, *, trusted_text: str = ""):
|
||||
with (
|
||||
patch.object(build, "_generator_model_config", return_value=_fake_mc("anthropic", "x")),
|
||||
patch(
|
||||
@@ -564,7 +564,7 @@ def _build_with(gen_graph: dict):
|
||||
return_value=resources.TenantResources(models=[], datasets=[], tools=[]),
|
||||
),
|
||||
):
|
||||
return build.build_nodes("t1", {}, ["Call the PPT API"]).intents
|
||||
return build.build_nodes("t1", {}, ["Call the PPT API"], trusted_text=trusted_text).intents
|
||||
|
||||
|
||||
def test_build_nodes_grounds_a_placeholder_endpoint_into_a_required_start_variable():
|
||||
@@ -644,6 +644,34 @@ def test_build_nodes_leaves_a_real_endpoint_and_a_templated_url_alone():
|
||||
assert start.args["config"]["variables"] == []
|
||||
|
||||
|
||||
def test_build_nodes_grounds_an_endpoint_the_user_never_supplied_even_when_not_placeholder_shaped():
|
||||
"""A plausible, real-looking host the model invented (S5b/ESQ1-302's shape
|
||||
one step earlier: ``api.pptrender.io``) must still be grounded once
|
||||
trusted_text is available -- ``_is_placeholder_endpoint`` alone would miss
|
||||
it, since nothing about the URL LOOKS invented the way ``example.com`` or
|
||||
``your-api.com`` do."""
|
||||
goal = "Build a workflow that renders a slide deck from bullet points and returns a download link."
|
||||
intents = _build_with(_gen_graph_with_http("https://api.pptrender.io/v1/render"), trusted_text=goal)
|
||||
|
||||
http = next(i for i in intents if i.args.get("node_type") == "http-request")
|
||||
start = next(i for i in intents if i.args.get("node_type") == "start")
|
||||
assert http.args["config"]["url"] == "{{#s.h_url#}}"
|
||||
var = next(v for v in start.args["config"]["variables"] if v["variable"] == "h_url")
|
||||
assert var["required"] is True
|
||||
|
||||
|
||||
def test_build_nodes_leaves_an_endpoint_alone_when_the_user_actually_supplied_its_host():
|
||||
"""The mirror case: trusted_text literally names the same host the
|
||||
generated URL uses, so it is not a fabrication and must be left alone."""
|
||||
goal = "Call https://api.pptrender.io to render a slide deck from bullet points."
|
||||
intents = _build_with(_gen_graph_with_http("https://api.pptrender.io/v1/render"), trusted_text=goal)
|
||||
|
||||
http = next(i for i in intents if i.args.get("node_type") == "http-request")
|
||||
start = next(i for i in intents if i.args.get("node_type") == "start")
|
||||
assert http.args["config"]["url"] == "https://api.pptrender.io/v1/render"
|
||||
assert start.args["config"]["variables"] == []
|
||||
|
||||
|
||||
def test_is_placeholder_endpoint_recognises_the_usual_inventions():
|
||||
assert build._is_placeholder_endpoint("https://api.example.com/ppt/generate")
|
||||
assert build._is_placeholder_endpoint("http://example.org/x")
|
||||
|
||||
@@ -70,8 +70,8 @@ def test_remaining_build_methods_thread_model_and_tenant_args(monkeypatch):
|
||||
seen["bind_resources"] = (model, tenant_id, plan_items, resource_ids)
|
||||
return []
|
||||
|
||||
def mock_build_nodes(tenant_id, model_config, plan_items, resource_ids=()):
|
||||
seen["build_nodes"] = (tenant_id, model_config, plan_items, resource_ids)
|
||||
def mock_build_nodes(tenant_id, model_config, plan_items, resource_ids=(), *, trusted_text=""):
|
||||
seen["build_nodes"] = (tenant_id, model_config, plan_items, resource_ids, trusted_text)
|
||||
return []
|
||||
|
||||
def mock_learn_from_build(model, goal_text, requirements, plan_items, built_node_ids, _reasoning=None):
|
||||
@@ -105,7 +105,13 @@ def test_remaining_build_methods_thread_model_and_tenant_args(monkeypatch):
|
||||
assert seen["bind_resources"] == ("MODEL", "t1", ["step"], ["rid"])
|
||||
|
||||
agent.build_nodes(["step"])
|
||||
assert seen["build_nodes"] == ("t1", model_config, ["step"], ())
|
||||
assert seen["build_nodes"] == ("t1", model_config, ["step"], (), "")
|
||||
|
||||
# trusted_text forwards through the wrapper unchanged (ESQ1-302/S5b: the
|
||||
# handler threads the user's own goal+requirements text down to
|
||||
# build.build_nodes so it can tell a supplied endpoint from an invented one).
|
||||
agent.build_nodes(["step"], trusted_text="goal text and requirements")
|
||||
assert seen["build_nodes"] == ("t1", model_config, ["step"], (), "goal text and requirements")
|
||||
|
||||
agent.learn_from_build("goal", {"x": 1}, ["step"], ["n1"])
|
||||
assert seen["learn_from_build"] == ("MODEL", "goal", {"x": 1}, ["step"], ["n1"])
|
||||
|
||||
Reference in new issue
Block a user