From d2cfaf7f729f9c201cf72188e843f027844a4e98 Mon Sep 17 00:00:00 2001 From: Charles Yao Date: Sun, 30 Aug 2026 06:37:18 -0700 Subject: [PATCH] fix(dify_builder): narrow input-failure signals, fix build delete/create id collision Final-review fix wave for the testdata-gate + start-selector slice: - I1 (Important): is_input_failure's signal list included the bare "required" (and generic "variable not found"), which also fire on config/runtime faults unrelated to test inputs -- misrouting those failures to the *_AWAIT_TESTDATA gate, where the diagnosed auto-repair is never offered and re-running just loops at the gate. Narrowed _INPUT_FAILURE_SIGNALS to ("file variable", "not provided", "missing input"), favoring precision per the spec's ambiguous-failure -> config-repair safe default. The real file-input E2E error ("File variable not found for selector: [...]") still matches via "file variable". - M2 (Minor, latent): handle_plan_approval's from-scratch delete-then- create-filter path could drop a generator create_node/connect that reuses a just-deleted placeholder start's id, since _already_present compared against the PRE-delete existing_node_ids/existing_edges -- delete with no re-create, silently vanishing the node. Deleted ids are now excluded from that comparison set; the loop-back path (no deletes) is unaffected. - Cleanups: state.py's Build/Edit state-count comments (14->15, 9->10, verified against the actual enum members), FormCard's docstring now lists the "testdata" variant, and handle_plan_approval binds current_graph's nodes to one local instead of calling .get() 3x and documents the delete-placeholder branch. Tests: extended test_is_input_failure_matches_input_signals's sibling with a config "required" case (red before the signal-list fix, green after), and added a plan_approval test where the generator's create_node reuses the deleted placeholder start's id (red before the id-exclusion fix -- a dangling connect ValueError -- green after). Co-Authored-By: Claude Opus 4.8 --- api/core/dify_builder/contract.py | 2 +- api/core/dify_builder/handlers_build.py | 47 ++++++++++++++----- api/core/dify_builder/handlers_fix.py | 15 +++++- api/core/dify_builder/state.py | 8 ++-- .../core/dify_builder/test_handlers_build.py | 44 +++++++++++++++++ .../core/dify_builder/test_handlers_fix.py | 15 ++++++ 6 files changed, 112 insertions(+), 19 deletions(-) diff --git a/api/core/dify_builder/contract.py b/api/core/dify_builder/contract.py index 152d32ee9fa..125223628f4 100644 --- a/api/core/dify_builder/contract.py +++ b/api/core/dify_builder/contract.py @@ -391,7 +391,7 @@ class PlanCard(_Card): @dataclass class FormCard(_Card): """Editable, re-submittable input card. ``variant`` in - build_requirements|edit_rules. + build_requirements|edit_rules|testdata. """ kind: ClassVar[CardKind] = CardKind.FORM diff --git a/api/core/dify_builder/handlers_build.py b/api/core/dify_builder/handlers_build.py index 6bb59ac5099..ffbf5f9163a 100644 --- a/api/core/dify_builder/handlers_build.py +++ b/api/core/dify_builder/handlers_build.py @@ -292,7 +292,16 @@ def handle_plan_approval(env: Env, turn: Turn, s: Session, fc: DifyBuilderContex exists in the current draft graph. Everything survives the first build (nothing exists yet); a re-approve after a loop-back filters everything out (it all already exists), so apply_repair([]) is a no-op rather than - raising on a colliding node id.""" + raising on a colliding node id. + + From-scratch delete-placeholder branch: when the draft has no non-start + nodes yet (nothing has been built), any start node(s) already on the + draft (e.g. the canvas's default placeholder start) are deleted before + the generator's intents are applied, so the generator's own start node + is the only one left. (Final-review fix, Minor #2): ids about to be + deleted are excluded from the already-present comparison, so a generator + create_node/connect that happens to reuse a just-deleted placeholder id + still gets applied instead of being silently dropped.""" kind = action_kind(turn) if kind != "approve_repair": return StepResult(next=PcState.BUILD_PLAN_APPROVAL, context=fc) @@ -301,27 +310,39 @@ def handle_plan_approval(env: Env, turn: Turn, s: Session, fc: DifyBuilderContex intents = env.agent.build_nodes(list(fc.plan_items)) current_graph, _current_hash = env.dify.read_graph(s.app_id, turn.actor) - existing_node_ids = {n.get("id") for n in current_graph.get("nodes", [])} + current_nodes = current_graph.get("nodes", []) + existing_node_ids = {n.get("id") for n in current_nodes} existing_edges = {(e.get("source"), e.get("target")) for e in current_graph.get("edges", [])} - def _already_present(intent) -> bool: - if intent.op == "create_node": - return intent.args.get("node_id") in existing_node_ids - if intent.op == "connect": - return (intent.args.get("from_node"), intent.args.get("to_node")) in existing_edges - return False - - existing_non_start = [ - n for n in current_graph.get("nodes", []) if (n.get("data") or {}).get("type") != "start" - ] + existing_non_start = [n for n in current_nodes if (n.get("data") or {}).get("type") != "start"] delete_intents: list[MutationIntent] = [] if not existing_non_start: # from-scratch build: drop the draft's placeholder start(s) delete_intents = [ MutationIntent(op="delete_node", args={"node_id": n["id"]}) - for n in current_graph.get("nodes", []) + for n in current_nodes if (n.get("data") or {}).get("type") == "start" and n.get("id") ] + # M2 fix (final review, Minor/latent): ids about to be deleted must NOT + # count as "already present" for the create/connect filter below -- else + # a generator create_node/connect that reuses a just-deleted placeholder + # id (e.g. both named "start") is dropped, and the node vanishes (delete + # with no re-create). Only affects the from-scratch branch: on loop-back + # delete_intents is empty, so these sets are identical to the originals + # and behavior there is unchanged. + deleted_node_ids = {intent.args["node_id"] for intent in delete_intents} + creatable_existing_node_ids = existing_node_ids - deleted_node_ids + creatable_existing_edges = { + (src, dst) for (src, dst) in existing_edges if src not in deleted_node_ids and dst not in deleted_node_ids + } + + def _already_present(intent) -> bool: + if intent.op == "create_node": + return intent.args.get("node_id") in creatable_existing_node_ids + if intent.op == "connect": + return (intent.args.get("from_node"), intent.args.get("to_node")) in creatable_existing_edges + return False + to_apply = delete_intents + [intent for intent in intents if not _already_present(intent)] result = env.dify.apply_repair(s.app_id, turn.actor, to_apply, on_canvas=env.emit_canvas) diff --git a/api/core/dify_builder/handlers_fix.py b/api/core/dify_builder/handlers_fix.py index 748f4caa30e..55e212ea6fd 100644 --- a/api/core/dify_builder/handlers_fix.py +++ b/api/core/dify_builder/handlers_fix.py @@ -234,13 +234,24 @@ def start_schema(graph: Graph) -> StartSchema: return {"variables": []} -_INPUT_FAILURE_SIGNALS = ("variable not found", "file variable", "required", "not provided", "missing input") +_INPUT_FAILURE_SIGNALS = ("file variable", "not provided", "missing input") def is_input_failure(run: Run) -> bool: """Heuristic: True if a failed node's error looks like a missing/invalid test INPUT (vs a config bug) -- so the flow can route back to the testdata gate - instead of the config-repair gate. Signal-substring match on node errors.""" + instead of the config-repair gate. Signal-substring match on node errors. + + Final-review fix (Important #1): the bare ``"required"`` and generic + ``"variable not found"`` signals were dropped -- both also appear in + config/runtime faults unrelated to test inputs (e.g. "field 'timeout' is + required"), which misrouted those failures to the testdata gate, where + the diagnosed auto-repair is never offered. The spec safe default for an + ambiguous failure is the config-repair path, so this tuple favors + PRECISION: only signals specific enough to input/file problems remain -- + ``"file variable"`` alone still matches the real file-input E2E error, + "File variable not found for selector: [...]". + """ for n in run.per_node: if n.status == "failed" and n.error: low = n.error.lower() diff --git a/api/core/dify_builder/state.py b/api/core/dify_builder/state.py index da6558d2585..36bb33e5afb 100644 --- a/api/core/dify_builder/state.py +++ b/api/core/dify_builder/state.py @@ -31,7 +31,8 @@ class PcState(StrEnum): SUCCESS = "success" FAILED = "failed" - # Build (spec §7.1) — 14 states. + # Build (spec §7.1) — 15 states (BUILD_AWAIT_TESTDATA added post-spec for + # the testdata-gate slice). BUILD_CAPABILITY_CHECK = "build.capability_check" BUILD_GOAL_ANALYSIS = "build.goal_analysis" BUILD_INITIAL_PLAN = "build.initial_plan" @@ -48,8 +49,9 @@ class PcState(StrEnum): BUILD_COMPLETE = "build.complete" BUILD_REVERTED = "build.reverted" - # Edit (spec §7.2) — 9 states (EDIT_AWAIT_REPAIR added post-spec for the - # live-test de-canning slice; mirrors BUILD_AWAIT_REPAIR). + # Edit (spec §7.2) — 10 states (EDIT_AWAIT_REPAIR added post-spec for the + # live-test de-canning slice, mirrors BUILD_AWAIT_REPAIR; EDIT_AWAIT_TESTDATA + # added post-spec for the testdata-gate slice). EDIT_CAPABILITY_CHECK = "edit.capability_check" EDIT_IMPACT_ANALYSIS = "edit.impact_analysis" EDIT_PLAN_APPROVAL = "edit.plan_approval" diff --git a/api/tests/unit_tests/core/dify_builder/test_handlers_build.py b/api/tests/unit_tests/core/dify_builder/test_handlers_build.py index 4814b0109e3..dc682066da8 100644 --- a/api/tests/unit_tests/core/dify_builder/test_handlers_build.py +++ b/api/tests/unit_tests/core/dify_builder/test_handlers_build.py @@ -292,6 +292,50 @@ def test_plan_approval_deletes_pre_existing_start_on_from_scratch_build(): assert types == {"start", "end"} # exactly one start +def test_plan_approval_survives_generator_reusing_the_deleted_placeholder_start_id(): + """M2 fix (final review, Minor/latent): on a from-scratch build, delete_intents + drops the draft's placeholder start(s) by id, and _already_present filters + generator creates against the PRE-delete existing_node_ids. If the generator's + create_node happens to reuse a just-deleted placeholder id, the old code path + would see that id in existing_node_ids and drop the create -- delete with no + re-create, so the node silently vanishes from the final graph. The deleted + ids must be excluded from the _already_present comparison set.""" + from core.dify_builder.handlers_build import handle_plan_approval + from core.dify_builder.models import MutationIntent + from tests.unit_tests.core.dify_builder.fakes import FakeBuildDifyPort + + env, _ = _new_env() + env.dify = FakeBuildDifyPort() + # seed a draft whose placeholder start id is "start" + env.dify.graph = { + "nodes": [{"id": "start", "data": {"type": "start", "title": "Old", "variables": []}}], + "edges": [], + } + # generator reuses the SAME id ("start") for its own start node + env.agent.build_nodes = lambda _plan: [ + MutationIntent( + op="create_node", + args={ + "node_type": "start", + "node_id": "start", + "config": {"title": "Start", "variables": [{"variable": "document", "type": "file"}]}, + }, + ), + MutationIntent(op="create_node", args={"node_type": "end", "node_id": "node2", "config": {}}), + MutationIntent(op="connect", args={"from_node": "start", "to_node": "node2"}), + ] + s = _session(entry_mode=EntryMode.BUILD, current_state=PcState.BUILD_PLAN_APPROVAL) + fc = DifyBuilderContext(plan_items=["x"]) + handle_plan_approval(env, Turn(actor=_actor(), action=Action(kind="approve_repair")), s, fc) + + ids = {n["id"] for n in env.dify.graph["nodes"]} + types = {(n.get("data") or {}).get("type") for n in env.dify.graph["nodes"]} + assert "start" in ids, "the create_node reusing the deleted placeholder's id must survive" + assert "node2" in ids + assert types == {"start", "end"} # exactly one start, no vanished node + assert len(env.dify.graph["edges"]) == 1 + + def test_execution_run_test_advances_to_test_and_repair(): from core.dify_builder.handlers_build import handle_execution diff --git a/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py b/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py index 0501aa49e03..e8a6c4b4f3d 100644 --- a/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py +++ b/api/tests/unit_tests/core/dify_builder/test_handlers_fix.py @@ -553,6 +553,21 @@ def test_is_input_failure_matches_input_signals(): assert is_input_failure(Run(per_node=[])) is False +def test_is_input_failure_does_not_misclassify_a_config_required_field_error(): + # I1 (final review, Important): "required" alone is too broad -- it also + # fires on config/runtime faults that have nothing to do with test inputs, + # misrouting them to the testdata gate where the diagnosed auto-repair is + # never offered. Only genuinely input/file-shaped signals should match. + from core.dify_builder.handlers_fix import is_input_failure + from core.dify_builder.models import NodeOutput, Run + cfg = Run( + per_node=[ + NodeOutput(node_id="n1", status="failed", error="Node config invalid: field 'timeout' is required") + ] + ) + assert is_input_failure(cfg) is False + + def test_testdata_form_fields_preserves_file_type(): from core.dify_builder.handlers_fix import testdata_form_fields schema = {"variables": [