Detect recursive subtree cycles at parse time (fixes #979) (#1085)

* Detect recursive subtree cycles at parse time (fixes #979)

Use an ancestor set passed through recursivelyCreateSubtree to detect
cycles, avoiding the substring-matching false positives of a prefix
path check.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* Pass ancestors set by reference instead of by value

Avoids copying the set at each recursive call. Uses insert-on-entry
and erase-on-exit (DFS backtracking) so sibling subtrees sharing the
same ID are handled correctly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

src/xml_parsing.cpp

@@ -21,6 +21,7 @@
 #include <string>
 #include <tuple>
 #include <typeindex>
+#include <unordered_set>
 
 #if defined(_MSVC_LANG) && !defined(__clang__)
 #define __bt_cplusplus (_MSC_VER == 1900 ? 201103L : _MSVC_LANG)
@@ -196,7 +197,8 @@ struct XMLParser::PImpl
   void recursivelyCreateSubtree(const std::string& tree_ID, const std::string& tree_path,
                                 const std::string& prefix_path, Tree& output_tree,
                                 Blackboard::Ptr blackboard,
-                                const TreeNode::Ptr& root_node);
+                                const TreeNode::Ptr& root_node,
+                                std::unordered_set<std::string>& ancestors);
 
   void getPortsRecursively(const XMLElement* element,
                            std::vector<std::string>& output_ports);
@@ -703,8 +705,9 @@ Tree XMLParser::instantiateTree(const Blackboard::Ptr& root_blackboard,
                        "root_blackboard");
   }
 
+  std::unordered_set<std::string> ancestors;
   _p->recursivelyCreateSubtree(main_tree_ID, {}, {}, output_tree, root_blackboard,
-                               TreeNode::Ptr());
+                               TreeNode::Ptr(), ancestors);
   output_tree.initialize();
   return output_tree;
 }
@@ -1005,13 +1008,16 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element,
   return new_node;
 }
 
-void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID,
-                                                    const std::string& tree_path,
-                                                    const std::string& prefix_path,
-                                                    Tree& output_tree,
-                                                    Blackboard::Ptr blackboard,
-                                                    const TreeNode::Ptr& root_node)
+void BT::XMLParser::PImpl::recursivelyCreateSubtree(
+    const std::string& tree_ID, const std::string& tree_path,
+    const std::string& prefix_path, Tree& output_tree, Blackboard::Ptr blackboard,
+    const TreeNode::Ptr& root_node, std::unordered_set<std::string>& ancestors)
 {
+  if(!ancestors.insert(tree_ID).second)
+  {
+    throw RuntimeError("Recursive behavior tree cycle detected: tree '", tree_ID,
+                       "' references itself (directly or indirectly)");
+  }
   std::function<void(const TreeNode::Ptr&, Tree::Subtree::Ptr, std::string,
                      const XMLElement*)>
       recursiveStep;
@@ -1140,7 +1146,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID,
       recursivelyCreateSubtree(subtree_ID,
                                subtree_path,        // name
                                subtree_path + "/",  //prefix
-                               output_tree, new_bb, node);
+                               output_tree, new_bb, node, ancestors);
     }
   };
 
@@ -1162,6 +1168,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID,
   output_tree.subtrees.push_back(new_tree);
 
   recursiveStep(root_node, new_tree, prefix_path, root_element);
+  ancestors.erase(tree_ID);
 }
 
 void XMLParser::PImpl::getPortsRecursively(const XMLElement* element,

tests/gtest_subtree.cpp

@@ -732,6 +732,86 @@ TEST(SubTree, SubtreeNameNotRegistered)
   ASSERT_ANY_THROW(factory.registerBehaviorTreeFromText(xml_text));
 }
 
+TEST(SubTree, RecursiveSubtree)
+{
+  // clang-format off
+
+  static const char* xml_text = R"(
+  <root BTCPP_format="4" >
+      <BehaviorTree ID="MainTree">
+         <Sequence name="root">
+             <AlwaysSuccess/>
+             <SubTree ID="MainTree" />
+         </Sequence>
+      </BehaviorTree>
+  </root>
+ )";
+
+  // clang-format on
+  BehaviorTreeFactory factory;
+
+  ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text));
+}
+
+TEST(SubTree, RecursiveCycle)
+{
+  // clang-format off
+
+  static const char* xml_text = R"(
+  <root BTCPP_format="4" main_tree_to_execute="MainTree">
+      <BehaviorTree ID="MainTree">
+         <Sequence name="root">
+             <AlwaysSuccess/>
+             <SubTree ID="TreeA" />
+         </Sequence>
+      </BehaviorTree>
+
+      <BehaviorTree ID="TreeA">
+         <Sequence name="root">
+             <AlwaysSuccess/>
+             <SubTree ID="TreeB" />
+         </Sequence>
+      </BehaviorTree>
+
+      <BehaviorTree ID="TreeB">
+         <Sequence name="root">
+             <AlwaysSuccess/>
+             <SubTree ID="MainTree" />
+         </Sequence>
+      </BehaviorTree>
+  </root>
+ )";
+
+  // clang-format on
+  BehaviorTreeFactory factory;
+
+  ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text));
+}
+
+TEST(SubTree, SubstringTreeIDsAreNotRecursive)
+{
+  // Verify that tree IDs which are substrings of each other do NOT
+  // incorrectly trigger the recursive cycle detection.
+  // clang-format off
+
+  static const char* xml_text = R"(
+  <root BTCPP_format="4" main_tree_to_execute="Tree">
+      <BehaviorTree ID="Tree">
+         <SubTree ID="TreeABC" />
+      </BehaviorTree>
+
+      <BehaviorTree ID="TreeABC">
+         <AlwaysSuccess/>
+      </BehaviorTree>
+  </root>
+ )";
+
+  // clang-format on
+  BehaviorTreeFactory factory;
+
+  ASSERT_NO_THROW(auto tree = factory.createTreeFromText(xml_text));
+}
+
 // Test for Groot2 issue #56: duplicate _fullpath when multiple subtrees have the same name
 // https://github.com/BehaviorTree/Groot2/issues/56
 //