From b7b1e32dced92bf7d3235a7a2fb1f091ff1f6fb8 Mon Sep 17 00:00:00 2001
From: Aysha Afrah Ziya <aysha26@digiscrypt.com>
Date: Tue, 1 Sep 2026 14:07:43 +0530
Subject: [PATCH] validate substituted node type in instantiateTreeNode

---
 src/bt_factory.cpp           | 22 ++++++++++++++++++
 tests/gtest_substitution.cpp | 43 ++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp
index 9040e80f1..81272a4eb 100644
--- a/src/bt_factory.cpp
+++ b/src/bt_factory.cpp
@@ -365,6 +365,28 @@ std::unique_ptr<TreeNode> BehaviorTreeFactory::instantiateTreeNode(
     }
   }
 
+  // A substitution rule must not turn a node into a structurally incompatible
+  // type. The XML was validated (children count, mandatory ID) against the
+  // original node type, so replacing a leaf with a SubTree, Decorator or
+  // Control leaves the builder with a node whose child or "ID" attribute the
+  // XML never supplied. That later dereferences a null child pointer or builds
+  // a std::string from a null Attribute("ID"). Allow same-type swaps and swaps
+  // to a leaf (the common "replace with a mock action" case, including the
+  // SubTree -> TestNode substitution used for mocking).
+  if(substituted)
+  {
+    const NodeType original_type = it_manifest->second.type;
+    const NodeType new_type = node->type();
+    if(new_type != original_type && new_type != NodeType::ACTION &&
+       new_type != NodeType::CONDITION)
+    {
+      throw RuntimeError("Substitution of node [", ID, "] of type [",
+                         toStr(original_type), "] with a node of type [", toStr(new_type),
+                         "] is not allowed: a substitution may only keep the same "
+                         "type or replace the node with a leaf (Action/Condition)");
+    }
+  }
+
   // No substitution rule applied: default behavior
   if(!substituted)
   {
diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp
index c64f6b1b6..bce34cd03 100644
--- a/tests/gtest_substitution.cpp
+++ b/tests/gtest_substitution.cpp
@@ -546,3 +546,46 @@ TEST(Substitution, StringSubstitutionRegistrationID_Issue930)
   // The substituted node should still work correctly
   ASSERT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS);
 }
+
+// Regression test: a substitution rule must not turn a node into a
+// structurally incompatible type. The XML for a compact leaf (<AlwaysSuccess/>)
+// carries no "ID" attribute and no children, so replacing it with a SubTree
+// (which reads element->Attribute("ID")) or a Decorator (which needs a child)
+// used to segfault at tree construction / first tick. It must now throw.
+TEST(Substitution, IncompatibleTypeSubstitutionThrows)
+{
+  static const char* xml_text = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="MainTree">
+      <AlwaysSuccess name="action_A"/>
+    </BehaviorTree>
+  </root>
+  )";
+
+  // leaf -> SubTree: element has no ID attribute
+  {
+    BehaviorTreeFactory factory;
+    factory.registerBehaviorTreeFromText(xml_text);
+    factory.addSubstitutionRule("action_A", "SubTree");
+    EXPECT_THROW(factory.createTree("MainTree"), RuntimeError);
+  }
+
+  // leaf -> Decorator: element has no child
+  {
+    BehaviorTreeFactory factory;
+    factory.registerBehaviorTreeFromText(xml_text);
+    factory.addSubstitutionRule("action_A", "Inverter");
+    EXPECT_THROW(factory.createTree("MainTree"), RuntimeError);
+  }
+
+  // leaf -> leaf stays valid (the common mock case)
+  {
+    BehaviorTreeFactory factory;
+    factory.registerBehaviorTreeFromText(xml_text);
+    factory.registerSimpleAction("MyMock", [](TreeNode&) { return NodeStatus::SUCCESS; });
+    factory.addSubstitutionRule("action_A", "MyMock");
+    Tree tree;
+    ASSERT_NO_THROW(tree = factory.createTree("MainTree"));
+    EXPECT_EQ(tree.tickWhileRunning(), NodeStatus::SUCCESS);
+  }
+}