Fullpath fix

[facontidavide]

Prevent issue with duplicated instance names, reported here: BehaviorTree/Groot2#56

Summary by CodeRabbit

• Bug Fixes

  • Added runtime validation to detect and prevent duplicate SubTree paths with descriptive error messages

• Documentation

  • Added comprehensive guide for project setup, build, testing, linting, and contribution guidelines

• Tests

  • Added extensive test suite validating SubTree naming, path uniqueness, and auto-generation behavior

• Chores

  • Updated .gitignore with additional entries

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request adds runtime validation to detect duplicate SubTree paths in the XML parsing pipeline and introduces comprehensive test coverage for SubTree naming behavior. Additional configuration entries and documentation have been included.

Changes

Cohort / File(s)	Summary
Configuration & Documentation .gitignore, CLAUDE.md	Added two new ignored file patterns for build artifacts. Introduced detailed documentation covering build setup, testing, linting, project architecture, and contribution guidelines for the Behaviortree_CPP project.
Runtime Validation src/xml_parsing.cpp	Implemented duplicate SubTree path detection in the recursivelyCreateSubtree method. Validates that subtree instance names are unique within the same parent context and raises a RuntimeError with guidance if duplicates are found.
Test Suite tests/gtest_subtree.cpp	Added extensive test coverage for SubTree naming behavior, including scenarios for duplicate detection, unique name verification, auto-generated paths, nested subtrees, and error message validation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

Hopping through XML trees so fine,
Duplicate detectors in a line,
Each SubTree path must be unique,
Our validation guards so chic,
With tests abound and docs so grand,
The forest stands more proud and planned! 🐰

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 2 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check	❓ Inconclusive	The title 'Fullpath fix' is vague and does not clearly convey the main change of preventing duplicate SubTree instance names, which is the primary objective of this PR.	Consider a more descriptive title like 'Detect and prevent duplicate SubTree instance names' that clearly communicates the core functionality being added.
Description check	❓ Inconclusive	The description is minimal and only references an external issue without explaining the problem, solution, testing approach, or implementation details as suggested by the template.	Expand the description to explain what issue is being fixed, how the solution works, and confirm that unit tests have been added as per the CONTRIBUTORS_GUIDE guidance.

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

CLAUDE.md (1)

100-115: Consider adding a language identifier to the fenced code block.

The static analysis tool flagged this code block for lacking a language specification. Since this is a directory tree structure, you could use ```text or ```plaintext to satisfy linters while preserving readability.

🔎 Proposed fix

-```
+```text
 src/
 ├── *.cpp              # Core: tree_node, blackboard, xml_parsing, bt_factory

tests/gtest_subtree.cpp (1)

868-892: Test name could be more descriptive.

The test name NestedDuplicateNames_ShouldFail suggests testing nested subtree structures, but it actually tests duplicates under a Sequence parent (similar to DuplicateSubTreeName_Groot2Issue56 which uses ParallelAll). Consider renaming to something like DuplicateSubTreeNames_UnderSequence_ShouldFail for clarity.

That said, the test itself is correct and provides valuable coverage for different parent node types.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 93f0f1a and 5d53e04.

📒 Files selected for processing (4)

• .gitignore
• CLAUDE.md
• src/xml_parsing.cpp
• tests/gtest_subtree.cpp

🧰 Additional context used

🧬 Code graph analysis (3)

CLAUDE.md (1)

conanfile.py (1)

• BehaviortreeCppConan (4-39)

src/xml_parsing.cpp (2)

include/behaviortree_cpp/exceptions.h (1)

• RuntimeError (58-67)

include/behaviortree_cpp/xml_parsing.h (2)

• BT (9-84)
• XMLParser (16-44)

tests/gtest_subtree.cpp (3)

include/behaviortree_cpp/exceptions.h (1)

• RuntimeError (58-67)

include/behaviortree_cpp/decorators/subtree_node.h (3)

• BT (6-86)
• SubTreeNode (52-84)
• setSubtreeID (66-69)

include/behaviortree_cpp/bt_factory.h (2)

• Tree (94-190)
• std (98-105)

🪛 markdownlint-cli2 (0.18.1)

CLAUDE.md

100-100: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: pixi_conda_build (ubuntu-latest)
• GitHub Check: pixi_conda_build (windows-latest)
• GitHub Check: build (ubuntu-22.04, asan_ubsan)
• GitHub Check: build (ubuntu-22.04, tsan)
• GitHub Check: build (ubuntu-22.04)
• GitHub Check: industrial_ci (humble, main)
• GitHub Check: industrial_ci (jazzy, main)
• GitHub Check: build (windows-latest)
• GitHub Check: clang-tidy
• GitHub Check: industrial_ci (rolling, main)

🔇 Additional comments (7)

.gitignore (1)

25-26: LGTM!

Sensible additions to ignore generated log and trace files from examples/tutorials.

src/xml_parsing.cpp (1)

1013-1025: LGTM! Clear duplicate path detection with actionable error message.

The implementation correctly addresses the Groot2 issue #56. The linear scan over existing subtrees is acceptable for typical tree sizes.

One minor observation: for very large trees with many subtrees, this O(n) check per subtree could accumulate, but this is unlikely to be a bottleneck in practice.

tests/gtest_subtree.cpp (5)

2-2: LGTM!

The <set> include is necessary for the path uniqueness verification in the new tests.

---

730-759: LGTM! Well-documented test case with clear issue reference.

The test correctly verifies that duplicate SubTree names under the same parent trigger a RuntimeError. The comment linking to Groot2 issue #56 provides excellent traceability.

---

761-796: LGTM! Thorough error message validation.

Testing the error message content ensures users get actionable feedback. The use of EXPECT_TRUE with std::string::find is appropriate for substring matching.

---

798-831: LGTM! Good positive test case.

This test complements the negative tests by verifying that unique names work correctly. The std::set approach for path uniqueness verification is clean and idiomatic.

---

833-866: LGTM! Validates the auto-generation fallback behavior.

This test verifies the recommended workaround mentioned in the error message: omitting the name attribute to auto-generate unique paths using UIDs.