Feature/bt impl#15
Conversation
WalkthroughThis PR implements a functional behavior tree node system in ChangesBehavior Tree Node Implementation
App Init Minor Edit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/runtime/ai/behavior_tree.h (3)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty virtual method lacks explanatory comment.
SonarCloud flags this empty body. A short comment clarifying that the default is a no-op (leaf nodes have no children) would address it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/ai/behavior_tree.h` at line 72, The empty default implementation of addChild in BehaviorTreeNode should be documented as an intentional no-op for nodes that cannot have children. Add a short explanatory comment near addChild in BehaviorTreeNode clarifying that leaf nodes ignore child attachment by design, so the empty body is intentional and not missing logic.Source: Linters/SAST tools
139-139: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePass
funcby const reference.
ActionNode(std::function<NodeStatus()> func)takes the callable by value, incurring an unnecessary copy.♻️ Proposed fix
-explicit ActionNode(std::function<NodeStatus()> func) : mFunc(func) { +explicit ActionNode(const std::function<NodeStatus()>& func) : mFunc(func) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/ai/behavior_tree.h` at line 139, ActionNode currently takes its std::function<NodeStatus()> callable by value, which creates an unnecessary copy; update the ActionNode constructor signature to take func by const reference and keep the existing mFunc initialization in place so callers can pass the callable without extra copying.Source: Linters/SAST tools
71-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic API lacks Doxygen documentation.
BehaviorTreeNodeand its derived classes (SequenceNode,ConditionNode,ActionNode,SelectorNode) and their public methods have no@brief/@param/@returncomments, unlike the rest of the codebase (e.g.ArgumentParser,FileArchive).As per coding guidelines, "**/*.h: Use Doxygen-style comments in header files for public APIs (
@brief,@param,@return, etc.)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/ai/behavior_tree.h` around lines 71 - 179, Add Doxygen-style documentation to the public API in BehaviorTreeNode and its derived classes so the header matches the project’s comment guidelines. Document each class and public method in SequenceNode, ConditionNode, ActionNode, and SelectorNode with `@brief` plus `@param/`@return where applicable, and include the base BehaviorTreeNode methods like addChild, getNodeType, and getNodeStatus. Use the existing public class/method names to place the comments directly above their declarations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/ai/behavior_tree.cpp`:
- Around line 45-51: The node factory in behavior_tree.cpp is using raw new for
SequenceNode, SelectorNode, ActionNode, and ConditionNode, which matches the
fragile ownership pattern called out in behavior_tree.h. Update the node
creation path so these branches return and store
std::unique_ptr<BehaviorTreeNode> instead of raw pointers, and adjust the
related BehaviorTree / node ownership flow to use std::make_unique where the
nodes are instantiated. Keep the existing class names and factory logic intact,
but remove manual delete-based ownership from the destructor path by making the
tree manage node lifetime automatically.
- Around line 36-56: `createNode` in `BehaviorTree` only instantiates the
current node from `"type"` and never builds its descendants, so JSON-loaded
trees stay flat. Update the factory to read any `"children"` collection from
`nodeData`, recursively call `createNode` for each child, and attach the results
with `addChild()` on the created `SequenceNode`/`SelectorNode`/other composite
node types. Keep the recursion in `createNode` (and any helper it uses) so
`BehaviorTree::init` produces a full hierarchy instead of only the root node.
- Line 41: The behavior-tree loader is calling get<std::string>() on
nodeData["type"] without verifying the value is actually a string. In
behavior_tree.cpp, update the node parsing logic around nodeData and nodeType to
validate the "type" field before conversion, using the existing JSON checks in
that flow to reject or skip non-string values safely. Make the fix in the code
path that constructs nodeType so behavior-tree loading does not throw
nlohmann::json::type_error on malformed input.
In `@src/runtime/ai/behavior_tree.h`:
- Around line 132-149: `ActionNode(const json&)` leaves `mFunc` unset, so
`tick()` can call an empty function and throw when nodes are built through
`BehaviorTreeNodeFactory::createNode`. Update the JSON-based constructor in
`ActionNode` to initialize `mFunc` the same way the function-based constructor
does, or otherwise ensure factory-created nodes receive a valid callable before
`tick()` runs; apply the same fix pattern used for `ConditionNode`.
- Around line 82-112: SequenceNode and SelectorNode currently own raw-pointer
children in mChildren and delete them manually, but they still allow
copying/moving, which can shallow-copy the pointer list and lead to double-free.
Update the behavior tree node classes by either switching mChildren to
std::unique_ptr ownership or, at minimum, explicitly deleting the copy
constructor, copy assignment operator, move constructor, and move assignment
operator on SequenceNode and SelectorNode so the ownership model is unambiguous
and safe.
- Around line 34-50: The enum members in NodeStatus and NodeType do not follow
the PascalCase convention used elsewhere in the codebase. Update the enum values
in behavior_tree.h so NodeStatus and NodeType use PascalCase names consistently
(matching patterns like ModuleState and LogType), and make sure all references
to those symbols are updated accordingly wherever they are used.
- Around line 74-179: `BehaviorTreeNode`’s `mNodeType` and `mNodeStatus` are
never initialized or updated by `SequenceNode`, `ConditionNode`, `ActionNode`,
or `SelectorNode`, so the getters always return `INVALID`. Add a way for derived
nodes to set the base state in the `BehaviorTreeNode` hierarchy (either via
protected setters or a base constructor), then update each derived constructor
to assign the correct `NodeType` and each `tick()` override to store the
returned `NodeStatus` before returning it.
- Around line 114-130: ConditionNode is wired incorrectly: it should be a leaf
condition, not a SequenceNode, and its callable is never initialized. Update
ConditionNode to derive from BehaviorTreeNode (or the appropriate leaf base),
remove inherited child-sequence behavior, and ensure the JSON constructor or
factory path assigns a valid predicate to mFunc. Also fix tick() to evaluate
mFunc only once, storing the result before returning SUCCESS/FAILURE, and make
sure the JSON name-to-callable registry in behavior_tree.cpp supplies the
function used by ConditionNode.
---
Nitpick comments:
In `@src/runtime/ai/behavior_tree.h`:
- Line 72: The empty default implementation of addChild in BehaviorTreeNode
should be documented as an intentional no-op for nodes that cannot have
children. Add a short explanatory comment near addChild in BehaviorTreeNode
clarifying that leaf nodes ignore child attachment by design, so the empty body
is intentional and not missing logic.
- Line 139: ActionNode currently takes its std::function<NodeStatus()> callable
by value, which creates an unnecessary copy; update the ActionNode constructor
signature to take func by const reference and keep the existing mFunc
initialization in place so callers can pass the callable without extra copying.
- Around line 71-179: Add Doxygen-style documentation to the public API in
BehaviorTreeNode and its derived classes so the header matches the project’s
comment guidelines. Document each class and public method in SequenceNode,
ConditionNode, ActionNode, and SelectorNode with `@brief` plus `@param/`@return
where applicable, and include the base BehaviorTreeNode methods like addChild,
getNodeType, and getNodeStatus. Use the existing public class/method names to
place the comments directly above their declarations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8ba1775-df87-4736-b21f-510f32eaa182
📒 Files selected for processing (3)
src/runtime/ai/behavior_tree.cppsrc/runtime/ai/behavior_tree.hsrc/runtime/application/app.cpp
| static BehaviorTreeNode* createNode(const json& nodeData) { | ||
| if (!nodeData.contains("type")) { | ||
| return nullptr; | ||
| } | ||
|
|
||
| std::string nodeType = nodeData["type"].get<std::string>(); | ||
|
|
||
| // Create the appropriate node based on the type | ||
| if (nodeType == "Sequence") { | ||
| return new SequenceNode(nodeData); | ||
| } else if (nodeType == "Selector") { | ||
| return new SelectorNode(nodeData); | ||
| } else if (nodeType == "Action") { | ||
| return new ActionNode(nodeData); | ||
| } else if (nodeType == "Condition") { | ||
| return new ConditionNode(nodeData); | ||
| } else { | ||
| // Unknown node type | ||
| return nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Factory builds only the root node type — never recurses into children, so JSON-driven tree construction doesn't actually build a tree.
createNode instantiates a single node based on "type" but never reads/iterates a "children" (or similar) array in nodeData to recursively construct and addChild() sub-nodes. As written, any tree loaded via BehaviorTree::init will only ever consist of the single root node — SequenceNode/SelectorNode instances built this way will always have an empty mChildren, making tick() trivially return SUCCESS/FAILURE with no actual behavior-tree traversal.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 47-47: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 45-45: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 51-51: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 49-49: Replace the use of "new" with an operation that automatically manages the memory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.cpp` around lines 36 - 56, `createNode` in
`BehaviorTree` only instantiates the current node from `"type"` and never builds
its descendants, so JSON-loaded trees stay flat. Update the factory to read any
`"children"` collection from `nodeData`, recursively call `createNode` for each
child, and attach the results with `addChild()` on the created
`SequenceNode`/`SelectorNode`/other composite node types. Keep the recursion in
`createNode` (and any helper it uses) so `BehaviorTree::init` produces a full
hierarchy instead of only the root node.
| return nullptr; | ||
| } | ||
|
|
||
| std::string nodeType = nodeData["type"].get<std::string>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and surrounding context.
git ls-files 'src/runtime/ai/behavior_tree.cpp' 'CMakeLists.txt' 'vcpkg.json' 'conanfile.*' 'package.json' '**/json.hpp' '**/nlohmann*' | sed -n '1,200p'
echo
echo '--- behavior_tree.cpp ---'
nl -ba src/runtime/ai/behavior_tree.cpp | sed -n '1,120p'
echo
echo '--- JSON dependency/version hints ---'
rg -n --hidden --glob '!**/.git/**' 'nlohmann|json.hpp|json version|JSON_VERSION|NLOHMANN_JSON' .Repository: kimkulling/Segfault
Length of output: 283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- behavior_tree.cpp (selected range) ---'
sed -n '1,120p' src/runtime/ai/behavior_tree.cpp
echo
echo '--- CMakeLists.txt ---'
sed -n '1,220p' CMakeLists.txt
echo
echo '--- vcpkg.json ---'
sed -n '1,220p' vcpkg.json
echo
echo '--- nlohmann/json references ---'
rg -n --hidden --glob '!**/.git/**' 'nlohmann|json.hpp|NLOHMANN_JSON|JSON_VERSION|version' .Repository: kimkulling/Segfault
Length of output: 6588
Guard type before get<std::string>() contains("type") only checks presence, so a non-string value still throws nlohmann::json::type_error here and can abort behavior-tree loading.
Suggested fix
- std::string nodeType = nodeData["type"].get<std::string>();
+ if (!nodeData["type"].is_string()) {
+ return nullptr;
+ }
+ std::string nodeType = nodeData["type"].get<std::string>();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string nodeType = nodeData["type"].get<std::string>(); | |
| if (!nodeData["type"].is_string()) { | |
| return nullptr; | |
| } | |
| std::string nodeType = nodeData["type"].get<std::string>(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.cpp` at line 41, The behavior-tree loader is
calling get<std::string>() on nodeData["type"] without verifying the value is
actually a string. In behavior_tree.cpp, update the node parsing logic around
nodeData and nodeType to validate the "type" field before conversion, using the
existing JSON checks in that flow to reject or skip non-string values safely.
Make the fix in the code path that constructs nodeType so behavior-tree loading
does not throw nlohmann::json::type_error on malformed input.
Source: Coding guidelines
| return new SequenceNode(nodeData); | ||
| } else if (nodeType == "Selector") { | ||
| return new SelectorNode(nodeData); | ||
| } else if (nodeType == "Action") { | ||
| return new ActionNode(nodeData); | ||
| } else if (nodeType == "Condition") { | ||
| return new ConditionNode(nodeData); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Raw new for node ownership — pairs with missing smart-pointer ownership in the header.
SonarCloud flags each new SequenceNode/SelectorNode/ActionNode/ConditionNode(...) call. Combined with the manual delete in the corresponding destructors (see behavior_tree.h), this is a fragile raw-pointer ownership chain. Switching node creation to std::make_unique/std::unique_ptr<BehaviorTreeNode> would resolve this at the source.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 47-47: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 45-45: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 51-51: Replace the use of "new" with an operation that automatically manages the memory.
[failure] 49-49: Replace the use of "new" with an operation that automatically manages the memory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.cpp` around lines 45 - 51, The node factory in
behavior_tree.cpp is using raw new for SequenceNode, SelectorNode, ActionNode,
and ConditionNode, which matches the fragile ownership pattern called out in
behavior_tree.h. Update the node creation path so these branches return and
store std::unique_ptr<BehaviorTreeNode> instead of raw pointers, and adjust the
related BehaviorTree / node ownership flow to use std::make_unique where the
nodes are instantiated. Keep the existing class names and factory logic intact,
but remove manual delete-based ownership from the destructor path by making the
tree manage node lifetime automatically.
Source: Linters/SAST tools
| enum class NodeStatus { | ||
| INVALID = -1, | ||
| IDLE = 0, | ||
| RUNNING = 1, | ||
| SUCCESS = 2, | ||
| FAILURE = 3, | ||
| SKIPPED = 4, | ||
| COUNT | ||
| }; | ||
|
|
||
| enum class NodeType { | ||
| INVALID = -1, | ||
| SEQUENCE, | ||
| SELECTOR, | ||
| CONDITION, | ||
| ACTION, | ||
| Count |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Enum values violate PascalCase naming convention.
NodeStatus uses ALL_CAPS (INVALID, IDLE, RUNNING, ...) and NodeType mixes ALL_CAPS values with one PascalCase value (Count). The existing codebase convention for similar enums (e.g. ModuleState, LogType in segfault.h) is PascalCase (Invalid, Init, Running, ...).
♻️ Proposed fix
enum class NodeStatus {
- INVALID = -1,
- IDLE = 0,
- RUNNING = 1,
- SUCCESS = 2,
- FAILURE = 3,
- SKIPPED = 4,
- COUNT
+ Invalid = -1,
+ Idle = 0,
+ Running = 1,
+ Success = 2,
+ Failure = 3,
+ Skipped = 4,
+ Count
};
enum class NodeType {
- INVALID = -1,
- SEQUENCE,
- SELECTOR,
- CONDITION,
- ACTION,
+ Invalid = -1,
+ Sequence,
+ Selector,
+ Condition,
+ Action,
Count
};As per coding guidelines, "Name classes, functions, enums, enum values, and typedefs in PascalCase".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum class NodeStatus { | |
| INVALID = -1, | |
| IDLE = 0, | |
| RUNNING = 1, | |
| SUCCESS = 2, | |
| FAILURE = 3, | |
| SKIPPED = 4, | |
| COUNT | |
| }; | |
| enum class NodeType { | |
| INVALID = -1, | |
| SEQUENCE, | |
| SELECTOR, | |
| CONDITION, | |
| ACTION, | |
| Count | |
| enum class NodeStatus { | |
| Invalid = -1, | |
| Idle = 0, | |
| Running = 1, | |
| Success = 2, | |
| Failure = 3, | |
| Skipped = 4, | |
| Count | |
| }; | |
| enum class NodeType { | |
| Invalid = -1, | |
| Sequence, | |
| Selector, | |
| Condition, | |
| Action, | |
| Count | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.h` around lines 34 - 50, The enum members in
NodeStatus and NodeType do not follow the PascalCase convention used elsewhere
in the codebase. Update the enum values in behavior_tree.h so NodeStatus and
NodeType use PascalCase names consistently (matching patterns like ModuleState
and LogType), and make sure all references to those symbols are updated
accordingly wherever they are used.
Source: Coding guidelines
| NodeType getNodeType() const { return mNodeType; } | ||
| NodeStatus getNodeStatus() const { return mNodeStatus; } | ||
|
|
||
| private: | ||
| NodeType mNodeType{ NodeType::INVALID }; | ||
| NodeStatus mNodeStatus{ NodeStatus::INVALID }; | ||
| }; | ||
|
|
||
| class LeafNode : public BehaviorTreeNode { | ||
| class SequenceNode : public BehaviorTreeNode { | ||
| public: | ||
| explicit SequenceNode(const json& nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the leaf node's behavior | ||
| return NodeStatus::IDLE; | ||
| for (auto *child : mChildren) { | ||
| assert(child != nullptr); | ||
| auto status = child->tick(); | ||
| if (status != NodeStatus::SUCCESS) { | ||
| return status; | ||
| } | ||
| } | ||
| return NodeStatus::SUCCESS; | ||
| } | ||
|
|
||
| ~SequenceNode() override { | ||
| for (auto child : mChildren) { | ||
| delete child; | ||
| } | ||
| mChildren.clear(); | ||
| } | ||
|
|
||
| void addChild(BehaviorTreeNode* node) override { | ||
| mChildren.push_back(node); | ||
| } | ||
|
|
||
| private: | ||
| std::vector< BehaviorTreeNode*> mChildren; | ||
| }; | ||
|
|
||
| class SequenceNode : public BehaviorTreeNode { | ||
| class ConditionNode : public SequenceNode { | ||
| public: | ||
| explicit ConditionNode(const json& nodeData) : SequenceNode(nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the sequence node's behavior | ||
| return NodeStatus::IDLE; | ||
| if (mFunc()) { | ||
| return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE; | ||
| } | ||
|
|
||
| return NodeStatus::FAILURE; | ||
| } | ||
|
|
||
| private: | ||
| std::function<bool()> mFunc; | ||
| }; | ||
|
|
||
| class ConditionNode : public LeafNode { | ||
| class ActionNode : public BehaviorTreeNode { | ||
| public: | ||
| explicit ActionNode(const json& nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
|
|
||
| explicit ActionNode(std::function<NodeStatus()> func) : mFunc(func) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the condition node's behavior | ||
| return NodeStatus::IDLE; | ||
| return mFunc(); | ||
| } | ||
|
|
||
| private: | ||
| std::function<NodeStatus()> mFunc; | ||
| }; | ||
|
|
||
| class ActionNode : public LeafNode { | ||
| class SelectorNode : public BehaviorTreeNode { | ||
| public: | ||
| explicit SelectorNode(const json& nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
| ~SelectorNode() override { | ||
| for (auto* child : mChildren) { | ||
| delete child; | ||
| } | ||
| mChildren.clear(); | ||
| } | ||
|
|
||
| void addChild(BehaviorTreeNode* node) override { | ||
| mChildren.push_back(node); | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the action node's behavior | ||
| return NodeStatus::IDLE; | ||
| for (auto* child : mChildren) { | ||
| assert(child != nullptr); | ||
|
|
||
| auto status = child->tick(); | ||
| if (status != NodeStatus::FAILURE) return status; | ||
| } | ||
| return NodeStatus::FAILURE; | ||
| } | ||
|
|
||
| private: | ||
| std::vector< BehaviorTreeNode*> mChildren; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
mNodeType/mNodeStatus are never set by any derived node — getters always report INVALID.
mNodeType and mNodeStatus are private with no setter and no parameterized base constructor, so SequenceNode, ConditionNode, ActionNode, and SelectorNode have no way to set them. getNodeType()/getNodeStatus() will therefore always return NodeType::INVALID/NodeStatus::INVALID regardless of the actual node, and tick() overrides never update mNodeStatus either. This makes both getters effectively dead/incorrect for every instantiated node.
🐛 Suggested fix sketch
private:
- NodeType mNodeType{ NodeType::INVALID };
- NodeStatus mNodeStatus{ NodeStatus::INVALID };
+ NodeType mNodeType{ NodeType::Invalid };
+ NodeStatus mNodeStatus{ NodeStatus::Invalid };
+
+ protected:
+ void setNodeType(NodeType type) { mNodeType = type; }
+ void setNodeStatus(NodeStatus status) { mNodeStatus = status; }
};Each derived constructor would then call setNodeType(NodeType::Sequence) etc., and each tick() override would call setNodeStatus(status) before returning.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 159-159: Rewrite the code so that you no longer need this "delete".
[warning] 139-139: Pass large object "func" by reference to const.
[warning] 134-134: Remove the unused parameter "nodeData", make it unnamed, or declare it "[[maybe_unused]]".
[warning] 114-114: Add a using-declaration to this derived class to inherit the constructors of "SequenceNode", and remove the ones you manually duplicated.
[warning] 84-84: Remove the unused parameter "nodeData", make it unnamed, or declare it "[[maybe_unused]]".
[failure] 101-101: Rewrite the code so that you no longer need this "delete".
[warning] 153-153: Remove the unused parameter "nodeData", make it unnamed, or declare it "[[maybe_unused]]".
[warning] 120-120: Reduce verbosity with "using enum" for "segfault::ai::NodeStatus".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.h` around lines 74 - 179, `BehaviorTreeNode`’s
`mNodeType` and `mNodeStatus` are never initialized or updated by
`SequenceNode`, `ConditionNode`, `ActionNode`, or `SelectorNode`, so the getters
always return `INVALID`. Add a way for derived nodes to set the base state in
the `BehaviorTreeNode` hierarchy (either via protected setters or a base
constructor), then update each derived constructor to assign the correct
`NodeType` and each `tick()` override to store the returned `NodeStatus` before
returning it.
| class SequenceNode : public BehaviorTreeNode { | ||
| public: | ||
| explicit SequenceNode(const json& nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the leaf node's behavior | ||
| return NodeStatus::IDLE; | ||
| for (auto *child : mChildren) { | ||
| assert(child != nullptr); | ||
| auto status = child->tick(); | ||
| if (status != NodeStatus::SUCCESS) { | ||
| return status; | ||
| } | ||
| } | ||
| return NodeStatus::SUCCESS; | ||
| } | ||
|
|
||
| ~SequenceNode() override { | ||
| for (auto child : mChildren) { | ||
| delete child; | ||
| } | ||
| mChildren.clear(); | ||
| } | ||
|
|
||
| void addChild(BehaviorTreeNode* node) override { | ||
| mChildren.push_back(node); | ||
| } | ||
|
|
||
| private: | ||
| std::vector< BehaviorTreeNode*> mChildren; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Raw-pointer child ownership without deleted copy/move ops risks double-free.
SequenceNode/SelectorNode own children via std::vector<BehaviorTreeNode*> and manually delete them in the destructor (flagged by SonarCloud at lines 101 and 159), but neither class deletes its copy constructor/assignment or move operations. Copying either node (e.g. by value) would shallow-copy the pointer vector, and both destructors would then double-delete the same children.
🔒 Suggested fix
class SequenceNode : public BehaviorTreeNode {
public:
+ SequenceNode(const SequenceNode&) = delete;
+ SequenceNode& operator=(const SequenceNode&) = delete;
explicit SequenceNode(const json& nodeData) {Consider switching mChildren to std::vector<std::unique_ptr<BehaviorTreeNode>> instead, which removes the need for manual delete entirely and resolves the SonarCloud findings at the root.
Also applies to: 151-179
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 84-84: Remove the unused parameter "nodeData", make it unnamed, or declare it "[[maybe_unused]]".
[failure] 101-101: Rewrite the code so that you no longer need this "delete".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.h` around lines 82 - 112, SequenceNode and
SelectorNode currently own raw-pointer children in mChildren and delete them
manually, but they still allow copying/moving, which can shallow-copy the
pointer list and lead to double-free. Update the behavior tree node classes by
either switching mChildren to std::unique_ptr ownership or, at minimum,
explicitly deleting the copy constructor, copy assignment operator, move
constructor, and move assignment operator on SequenceNode and SelectorNode so
the ownership model is unambiguous and safe.
Source: Linters/SAST tools
| class ConditionNode : public SequenceNode { | ||
| public: | ||
| explicit ConditionNode(const json& nodeData) : SequenceNode(nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the sequence node's behavior | ||
| return NodeStatus::IDLE; | ||
| if (mFunc()) { | ||
| return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE; | ||
| } | ||
|
|
||
| return NodeStatus::FAILURE; | ||
| } | ||
|
|
||
| private: | ||
| std::function<bool()> mFunc; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
ConditionNode is fundamentally broken: inherits from SequenceNode, double-evaluates mFunc(), and mFunc is never initialized.
Three compounding issues:
ConditionNodeinheritsSequenceNode(gaining a publicaddChild/mChildrenit never ticks), rather than deriving directly fromBehaviorTreeNodeas a leaf condition node should.tick()callsmFunc()to check truthiness, then callsmFunc()again inside the ternary — redundant and unsafe if the predicate has side effects.mFuncis a default-constructedstd::function<bool()>with no target; the only constructor (ConditionNode(const json&)) never assigns it. Since the factory only ever uses this constructor, everyConditionNodeproduced via JSON will throwstd::bad_function_callthe first timetick()is invoked.
🐛 Minimal fix for the double-call (does not fix the unset `mFunc` root cause)
NodeStatus tick() override {
- if (mFunc()) {
- return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE;
- }
-
- return NodeStatus::FAILURE;
+ return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class ConditionNode : public SequenceNode { | |
| public: | |
| explicit ConditionNode(const json& nodeData) : SequenceNode(nodeData) { | |
| // empty | |
| } | |
| NodeStatus tick() override { | |
| // Implementation of the sequence node's behavior | |
| return NodeStatus::IDLE; | |
| if (mFunc()) { | |
| return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE; | |
| } | |
| return NodeStatus::FAILURE; | |
| } | |
| private: | |
| std::function<bool()> mFunc; | |
| }; | |
| class ConditionNode : public SequenceNode { | |
| public: | |
| explicit ConditionNode(const json& nodeData) : SequenceNode(nodeData) { | |
| // empty | |
| } | |
| NodeStatus tick() override { | |
| return mFunc() ? NodeStatus::SUCCESS : NodeStatus::FAILURE; | |
| } | |
| private: | |
| std::function<bool()> mFunc; | |
| }; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 114-114: Add a using-declaration to this derived class to inherit the constructors of "SequenceNode", and remove the ones you manually duplicated.
[warning] 120-120: Reduce verbosity with "using enum" for "segfault::ai::NodeStatus".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.h` around lines 114 - 130, ConditionNode is
wired incorrectly: it should be a leaf condition, not a SequenceNode, and its
callable is never initialized. Update ConditionNode to derive from
BehaviorTreeNode (or the appropriate leaf base), remove inherited child-sequence
behavior, and ensure the JSON constructor or factory path assigns a valid
predicate to mFunc. Also fix tick() to evaluate mFunc only once, storing the
result before returning SUCCESS/FAILURE, and make sure the JSON name-to-callable
registry in behavior_tree.cpp supplies the function used by ConditionNode.
| class ActionNode : public BehaviorTreeNode { | ||
| public: | ||
| explicit ActionNode(const json& nodeData) { | ||
| // empty | ||
| } | ||
|
|
||
|
|
||
| explicit ActionNode(std::function<NodeStatus()> func) : mFunc(func) { | ||
| // empty | ||
| } | ||
|
|
||
| NodeStatus tick() override { | ||
| // Implementation of the condition node's behavior | ||
| return NodeStatus::IDLE; | ||
| return mFunc(); | ||
| } | ||
|
|
||
| private: | ||
| std::function<NodeStatus()> mFunc; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
ActionNode(const json&) never sets mFunc; ticking a factory-built ActionNode throws.
The JSON-only constructor (the one the factory uses) leaves mFunc default-constructed (empty). tick() unconditionally calls mFunc(), which throws std::bad_function_call for any node built through BehaviorTreeNodeFactory::createNode. This mirrors the same gap in ConditionNode.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 139-139: Pass large object "func" by reference to const.
[warning] 134-134: Remove the unused parameter "nodeData", make it unnamed, or declare it "[[maybe_unused]]".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/ai/behavior_tree.h` around lines 132 - 149, `ActionNode(const
json&)` leaves `mFunc` unset, so `tick()` can call an empty function and throw
when nodes are built through `BehaviorTreeNodeFactory::createNode`. Update the
JSON-based constructor in `ActionNode` to initialize `mFunc` the same way the
function-based constructor does, or otherwise ensure factory-created nodes
receive a valid callable before `tick()` runs; apply the same fix pattern used
for `ConditionNode`.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes