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 <noreply@anthropic.com>

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

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)

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()

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"

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
 

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": [