From fd8ced664e9218dbcf6f398f25dbb2542ee0656b Mon Sep 17 00:00:00 2001 From: yurekami Date: Wed, 2 Sep 2026 01:32:15 +0800 Subject: [PATCH] Keep ReactiveSequence async validation ABI-safe ReactiveSequence XML validation needs registration-time async metadata, but TreeNodeManifest is a public installed struct and changing its layout would create avoidable ABI churn. Reuse the existing manifest metadata channel to track async actions and controls, hide the internal marker from exported TreeNodesModel XML, and mark built-in async controls explicitly while exposing helpers for manual-builder and plugin registrations.\n\nConstraint: TreeNodeManifest is public API in installed headers and the project guide explicitly calls out ABI/back-compat review\nConstraint: VerifyXML runs before node instantiation and can only rely on registration metadata\nRejected: Add a public bool field to TreeNodeManifest | changes public struct layout for an internal validation detail\nRejected: Infer async controls from XML tag names | misses AsyncFallback and custom async registrations\nConfidence: high\nScope-risk: narrow\nDirective: Keep async validation metadata-based and reserve __bt_async for internal manifest state only\nTested: Conan Debug configure/build with VS2022 toolchain; gtest filter Reactive.*:BehaviorTreeFactory.*:PluginIssue1184Test.*:BasicTypes.TreeNodeManifestAsyncMetadata\nNot-tested: Full ctest discovery run from this shell without extra PATH injection for behaviortree_cppd.dll Signed-off-by: yurekami --- include/behaviortree_cpp/bt_factory.h | 23 ++++++++- include/behaviortree_cpp/tree_node.h | 57 +++++++++++++++++++++ include/behaviortree_cpp/xml_parsing.h | 3 +- src/bt_factory.cpp | 18 +++++++ src/xml_parsing.cpp | 33 ++++++------ tests/CMakeLists.txt | 12 ++++- tests/gtest_basic_types.cpp | 13 +++++ tests/gtest_factory.cpp | 54 +++++++++++++++++++ tests/gtest_plugin_issue1184.cpp | 52 +++++++++++++++++++ tests/gtest_reactive.cpp | 46 +++++++++++++++++ tests/plugin_issue1184/plugin_issue1184.cpp | 13 +++++ 11 files changed, 305 insertions(+), 19 deletions(-) create mode 100644 tests/gtest_plugin_issue1184.cpp create mode 100644 tests/plugin_issue1184/plugin_issue1184.cpp diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 0ba3c86bb..cdc76146d 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -40,15 +40,31 @@ inline NodeBuilder CreateBuilder(Args... args) }; } +template +inline constexpr bool IsManifestAsync() +{ + return std::is_base_of_v || + std::is_base_of_v || + std::is_base_of_v; +} + template inline TreeNodeManifest CreateManifest(const std::string& ID, PortsList portlist = getProvidedPorts()) { + TreeNodeManifest manifest; + manifest.type = getType(); + manifest.registration_ID = ID; + manifest.ports = std::move(portlist); if constexpr(has_static_method_metadata::value) { - return { getType(), ID, portlist, T::metadata() }; + manifest.metadata = T::metadata(); } - return { getType(), ID, portlist, {} }; + if constexpr(IsManifestAsync()) + { + SetNodeManifestAsync(manifest); + } + return manifest; } #ifdef BT_PLUGIN_EXPORT @@ -472,6 +488,9 @@ class BehaviorTreeFactory /// to with the function writeTreeNodesModelXML() void addMetadataToManifest(const std::string& node_id, const KeyValueVector& metadata); + /// Mark a registered node as asynchronous for XML validation. + void markNodeAsAsynchronous(const std::string& node_id, bool is_async = true); + /** * @brief Add an Enum to the scripting language. * For instance if you do: diff --git a/include/behaviortree_cpp/tree_node.h b/include/behaviortree_cpp/tree_node.h index 9fe48568d..0866cffd8 100644 --- a/include/behaviortree_cpp/tree_node.h +++ b/include/behaviortree_cpp/tree_node.h @@ -41,6 +41,63 @@ struct TreeNodeManifest KeyValueVector metadata; }; +[[nodiscard]] inline bool IsReservedNodeMetadataField(StringView key) +{ + return key == "__bt_async"; +} + +inline void SetNodeManifestAsync(TreeNodeManifest& manifest, bool is_async = true) +{ + auto async_it = manifest.metadata.end(); + for(auto it = manifest.metadata.begin(); it != manifest.metadata.end();) + { + if(IsReservedNodeMetadataField(it->first)) + { + if(async_it == manifest.metadata.end()) + { + async_it = it; + ++it; + } + else + { + it = manifest.metadata.erase(it); + } + } + else + { + ++it; + } + } + + if(is_async) + { + if(async_it == manifest.metadata.end()) + { + manifest.metadata.emplace_back("__bt_async", "true"); + } + else + { + async_it->second = "true"; + } + } + else if(async_it != manifest.metadata.end()) + { + manifest.metadata.erase(async_it); + } +} + +[[nodiscard]] inline bool IsNodeManifestAsync(const TreeNodeManifest& manifest) +{ + for(const auto& [key, value] : manifest.metadata) + { + if(IsReservedNodeMetadataField(key)) + { + return value == "true"; + } + } + return false; +} + using PortsRemapping = std::unordered_map; using NonPortAttributes = std::unordered_map; diff --git a/include/behaviortree_cpp/xml_parsing.h b/include/behaviortree_cpp/xml_parsing.h index d815ef85b..3595e28a7 100644 --- a/include/behaviortree_cpp/xml_parsing.h +++ b/include/behaviortree_cpp/xml_parsing.h @@ -44,7 +44,8 @@ class XMLParser : public Parser }; void VerifyXML(const std::string& xml_text, - const std::unordered_map& registered_nodes); + const std::unordered_map& + registered_nodes); /** * @brief writeTreeNodesModelXML generates an XMl that contains the manifests in the diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index 9040e80f1..ef385d960 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -119,6 +119,8 @@ BehaviorTreeFactory::BehaviorTreeFactory() : _p(new PImpl) registerNodeType("AsyncFallback", true); registerNodeType("Sequence"); registerNodeType("AsyncSequence", true); + markNodeAsAsynchronous("AsyncFallback"); + markNodeAsAsynchronous("AsyncSequence"); registerNodeType("SequenceWithMemory"); #ifdef USE_BTCPP3_OLD_NAMES @@ -485,7 +487,23 @@ void BehaviorTreeFactory::addMetadataToManifest(const std::string& node_id, { throw std::runtime_error("addMetadataToManifest: wrong ID"); } + const bool is_async = IsNodeManifestAsync(it->second); it->second.metadata = metadata; + if(is_async) + { + SetNodeManifestAsync(it->second); + } +} + +void BehaviorTreeFactory::markNodeAsAsynchronous(const std::string& node_id, + bool is_async) +{ + auto it = _p->manifests.find(node_id); + if(it == _p->manifests.end()) + { + throw std::runtime_error("markNodeAsAsynchronous: wrong ID"); + } + SetNodeManifestAsync(it->second, is_async); } void BehaviorTreeFactory::registerScriptingEnum(StringView name, int value) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index e37836d0f..90dbd50e8 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -439,18 +439,12 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) } // Collect the names of all nodes registered with the behavior tree factory - std::unordered_map registered_nodes; - for(const auto& it : factory->manifests()) - { - registered_nodes.insert({ it.first, it.second.type }); - } - XMLPrinter printer; doc->Print(&printer); auto xml_text = std::string(printer.CStr(), size_t(printer.CStrSize())); // Verify the validity of the XML before adding any behavior trees to the parser's list of registered trees - VerifyXML(xml_text, registered_nodes); + VerifyXML(xml_text, factory->manifests()); loadSubtreeModel(xml_root); @@ -473,7 +467,8 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) } void VerifyXML(const std::string& xml_text, - const std::unordered_map& registered_nodes) + const std::unordered_map& + registered_nodes) { XMLDocument doc; auto xml_error = doc.Parse(xml_text.c_str(), xml_text.size()); @@ -629,7 +624,7 @@ void VerifyXML(const std::string& xml_text, ThrowError(line_number, std::string("Node not recognized: ") + lookup_name); } - const auto node_type = search->second; + const auto node_type = search->second.type; const std::string& registered_name = search->first; if(node_type == NodeType::DECORATOR) @@ -665,11 +660,12 @@ void VerifyXML(const std::string& xml_text, ThrowError(child->GetLineNum(), std::string("Unknown node type: ") + child_name); } - const auto child_type = child_search->second; - if(child_type == NodeType::CONTROL && - ((child_name == "ThreadedAction") || - (child_name == "StatefulActionNode") || - (child_name == "CoroActionNode") || (child_name == "AsyncSequence"))) + const auto& child_manifest = child_search->second; + const bool is_async_child = + IsNodeManifestAsync(child_manifest) && + (child_manifest.type == NodeType::ACTION || + child_manifest.type == NodeType::CONTROL); + if(is_async_child) { ++async_count; if(async_count > 1) @@ -1340,12 +1336,19 @@ void addNodeModelToXML(const TreeNodeManifest& model, XMLDocument& doc, for(const auto& [name, value] : model.metadata) { + if(IsReservedNodeMetadataField(name)) + { + continue; + } auto metadata_element = doc.NewElement("Metadata"); metadata_element->SetAttribute(name.c_str(), value.c_str()); metadata_root->InsertEndChild(metadata_element); } - element->InsertEndChild(metadata_root); + if(metadata_root->FirstChildElement() != nullptr) + { + element->InsertEndChild(metadata_root); + } } model_root->InsertEndChild(element); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 49fcce397..fde6559de 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,14 @@ set_target_properties(plugin_issue953 PROPERTIES ) target_link_libraries(plugin_issue953 ${BTCPP_LIBRARY}) +add_library(plugin_issue1184 SHARED plugin_issue1184/plugin_issue1184.cpp) +target_compile_definitions(plugin_issue1184 PRIVATE BT_PLUGIN_EXPORT) +set_target_properties(plugin_issue1184 PROPERTIES + PREFIX "" + LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}" +) +target_link_libraries(plugin_issue1184 ${BTCPP_LIBRARY}) + ###################################################### set(BT_TESTS @@ -55,6 +63,7 @@ set(BT_TESTS gtest_simple_string.cpp gtest_polymorphic_ports.cpp gtest_plugin_issue953.cpp + gtest_plugin_issue1184.cpp gtest_blackboard_thread_safety.cpp gtest_xml_null_subtree_id.cpp @@ -123,7 +132,8 @@ endif() target_compile_definitions(behaviortree_cpp_test PRIVATE BT_TEST_FOLDER="${CMAKE_CURRENT_SOURCE_DIR}") # Ensure plugin is built before tests run, and tests can find it -add_dependencies(behaviortree_cpp_test plugin_issue953) +add_dependencies(behaviortree_cpp_test plugin_issue953 plugin_issue1184) target_compile_definitions(behaviortree_cpp_test PRIVATE BT_PLUGIN_ISSUE953_PATH="$" + BT_PLUGIN_ISSUE1184_PATH="$" ) diff --git a/tests/gtest_basic_types.cpp b/tests/gtest_basic_types.cpp index 680c45ce3..aa426bdf0 100644 --- a/tests/gtest_basic_types.cpp +++ b/tests/gtest_basic_types.cpp @@ -408,6 +408,19 @@ TEST(BasicTypes, TreeNodeManifest) ASSERT_EQ(manifest.ports.size(), 2u); } +TEST(BasicTypes, TreeNodeManifestAsyncMetadata) +{ + TreeNodeManifest manifest; + EXPECT_FALSE(IsNodeManifestAsync(manifest)); + + SetNodeManifestAsync(manifest); + EXPECT_TRUE(IsNodeManifestAsync(manifest)); + EXPECT_TRUE(IsReservedNodeMetadataField("__bt_async")); + + SetNodeManifestAsync(manifest, false); + EXPECT_FALSE(IsNodeManifestAsync(manifest)); +} + // ============ Result type tests ============ TEST(BasicTypes, Result_Success) diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 2c257b5b2..2df701b23 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -1,5 +1,7 @@ #include "behaviortree_cpp/xml_parsing.h" +#include "action_test_node.h" + #include #include #include @@ -495,6 +497,19 @@ TEST(BehaviorTreeFactory, addMetadataToManifest) EXPECT_EQ(modified_manifest.metadata, makeTestMetadata()); } +TEST(BehaviorTreeFactory, addMetadataToManifestPreservesAsyncMarker) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + factory.addMetadataToManifest("AsyncActionTest", makeTestMetadata()); + + const auto& manifest = factory.manifests().at("AsyncActionTest"); + EXPECT_TRUE(IsNodeManifestAsync(manifest)); + EXPECT_EQ(manifest.metadata[0], makeTestMetadata()[0]); + EXPECT_EQ(manifest.metadata[1], makeTestMetadata()[1]); +} + // Action node used to reproduce issue #1046 (use-after-free on // manifest pointer). It calls getInput() for a port name that is // NOT in the XML, so getInputStamped falls through to the @@ -777,3 +792,42 @@ TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType) BehaviorTreeFactory factory; EXPECT_THROW((void)factory.createTreeFromText(xml), RuntimeError); } + +TEST(BehaviorTreeFactory, VerifyXMLRejectsManualAsyncControlInReactiveSequence) +{ + const char* xml_text_issue = R"( + + + + + + + + + + + + + )"; + + BehaviorTreeFactory factory; + + TreeNodeManifest manifest{ NodeType::CONTROL, "ManualAsyncFallback", {}, {} }; + SetNodeManifestAsync(manifest); + factory.registerBuilder( + manifest, [](const std::string& name, const NodeConfig&) -> std::unique_ptr { + return std::make_unique(name, true); + }); + + EXPECT_THROW((void)factory.createTreeFromText(xml_text_issue), RuntimeError); +} + +TEST(BehaviorTreeFactory, WriteTreeNodesModelXMLSkipsInternalAsyncMetadata) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + const auto xml = writeTreeNodesModelXML(factory, false); + + EXPECT_EQ(xml.find("__bt_async"), std::string::npos); +} diff --git a/tests/gtest_plugin_issue1184.cpp b/tests/gtest_plugin_issue1184.cpp new file mode 100644 index 000000000..bcdf9d9f9 --- /dev/null +++ b/tests/gtest_plugin_issue1184.cpp @@ -0,0 +1,52 @@ +#include "behaviortree_cpp/bt_factory.h" + +#include + +#include + +using namespace BT; + +#ifndef BT_PLUGIN_ISSUE1184_PATH +#define BT_PLUGIN_ISSUE1184_PATH "plugin_issue1184.so" +#endif + +class PluginIssue1184Test : public testing::Test +{ +protected: + void SetUp() override + { + plugin_path_ = BT_PLUGIN_ISSUE1184_PATH; + + if(!std::filesystem::exists(plugin_path_)) + { + GTEST_SKIP() << "Plugin not found at: " << plugin_path_ << ". " + << "Make sure it's built before running this test."; + } + } + + std::string plugin_path_; +}; + +TEST_F(PluginIssue1184Test, VerifyXMLRejectsPluginAsyncControlInReactiveSequence) +{ + const char* xml_text = R"( + + + + + + + + + + + + + + )"; + + BehaviorTreeFactory factory; + factory.registerFromPlugin(plugin_path_); + + EXPECT_THROW((void)factory.createTreeFromText(xml_text), RuntimeError); +} diff --git a/tests/gtest_reactive.cpp b/tests/gtest_reactive.cpp index 55c9176de..c92ab1858 100644 --- a/tests/gtest_reactive.cpp +++ b/tests/gtest_reactive.cpp @@ -1,5 +1,6 @@ #include "test_helper.hpp" +#include "action_test_node.h" #include "behaviortree_cpp/bt_factory.h" #include "behaviortree_cpp/loggers/bt_observer.h" @@ -187,6 +188,51 @@ TEST(Reactive, TwoAsyncNodesInReactiveSequence) EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); } +TEST(Reactive, TwoAsyncActionNodesInReactiveSequence) +{ + static const char* reactive_xml_text = R"( + + + + + + + + +)"; + + BT::BehaviorTreeFactory factory; + factory.registerNodeType("AsyncActionTest"); + + EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); +} + +TEST(Reactive, AsyncFallbackAndAsyncSequenceInReactiveSequence) +{ + static const char* reactive_xml_text = R"( + + + + + + + + + + + + + + +)"; + + BT::BehaviorTreeFactory factory; + std::array counters{}; + RegisterTestTick(factory, "Test", counters); + + EXPECT_ANY_THROW(auto tree = factory.createTreeFromText(reactive_xml_text)); +} + // ============ Phase 4: Additional Reactive Tests ============ TEST(Reactive, ReactiveSequence_FirstChildFails) diff --git a/tests/plugin_issue1184/plugin_issue1184.cpp b/tests/plugin_issue1184/plugin_issue1184.cpp new file mode 100644 index 000000000..b2605f506 --- /dev/null +++ b/tests/plugin_issue1184/plugin_issue1184.cpp @@ -0,0 +1,13 @@ +#include "behaviortree_cpp/bt_factory.h" +#include "behaviortree_cpp/controls/fallback_node.h" + +BT_REGISTER_NODES(factory) +{ + BT::TreeNodeManifest manifest{ BT::NodeType::CONTROL, "PluginAsyncFallback", {}, {} }; + BT::SetNodeManifestAsync(manifest); + factory.registerBuilder( + manifest, + [](const std::string& name, const BT::NodeConfig&) -> std::unique_ptr { + return std::make_unique(name, true); + }); +}