fix: preserve empty subtree model defaults (backport for 10.1.1)

[dv-picknik]

[written by AI]

The parser treats a SubTree model port declared <inout_port name="note" default=""/> as mandatory, so a parent that invokes the subtree without remapping note fails to build with is defining a mandatory port called [note], but you are not remapping it. The instantiation check in src/xml_parsing.cpp tested defaultValueString().empty(), which cannot tell default="" from a missing default. It now tests defaultValue().empty(), the Any that is set only when the attribute exists. The model writer and XSD generator in the same file already use that test.

This cherry-picks d665d41 from #29 onto main so the fix can ship in MoveIt Pro 10.1.1 on the 4.7.2 package, instead of waiting for the 4.9.0 port in 10.2.0. The only conflict was the test file, where main has SubTree.WhitespaceInSubtreeModel in the same spot. I kept both tests. clang-format rewrapped two lines of the new test to main's column limit.

Fixes PickNikRobotics/moveit_pro#22069.

SubTree.EmptyModelDefaultIsNotMandatory covers the matrix from the issue:

Case	Model	Invocation	Result
A	default=""	not remapped	builds, note is "" (failed before this fix)
B	default=""	note=""	builds
C	default="something"	not remapped	builds, note is something
D	no default	not remapped	throws RuntimeError

Case B remaps the port explicitly, so it never reaches the changed line. A and D are the pair that pin it.

Validation ran in the moveit-pro-dev:10.1.0-jazzy image with LD_LIBRARY_PATH pointed at the fresh build so the installed 4.7.2-3 library does not shadow it.

• all 217 tests pass, at b61a3989
• with the one-line fix reverted, EmptyModelDefaultIsNotMandatory fails on the mandatory-port error
• pre-commit run --from-ref origin/main passes

The second commit replaces SCOPED_TRACE in the new test with << test_case.name on each assertion. SCOPED_TRACE crashes the pixi Windows job, and with it gone that job passes on #33. Here it now fails only on Reactive.MissingOrEmptyGenericChildIdIsRejected, which #33 fixes, so merge #33 first.

Merge with a merge commit. PickNikRobotics/apt_build_farm#68 packages b61a3989 as revision 4, and a squash or rebase would orphan that SHA. PickNikRobotics/moveit_pro#23046 then pins 4.7.2-4noble on v10.1.

🤖 Generated with Claude Code

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 3d528120-4def-49b7-8387-7d133d80e0a4

📥 Commits

Reviewing files that changed from the base of the PR and between c4b23ea and b61a398.

📒 Files selected for processing (1)

• tests/gtest_subtree.cpp

Included review availability: This review used your included allowance. 6 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 8 reviews per hour.

---

📝 Summary

Summary by CodeRabbit

• Bug Fixes
  • Subtree models now correctly allow unmapped ports with an explicitly empty default value, rather than treating those ports as mandatory.
• Tests
  • Expanded coverage for empty and non-empty defaults, explicit remapping, and ports without defaults. The tests also check that subtree creation succeeds or throws as expected for each case.

Walkthrough

Subtree model port validation now checks defaultValue() to determine whether an unmapped port is mandatory. Tests cover empty and non-empty defaults, explicit empty remapping, and a missing default.

Changes

Subtree port defaults

Layer / File(s)	Summary
Port default validation and coverage src/xml_parsing.cpp, tests/gtest_subtree.cpp	The mandatory-port check now uses defaultValue(). Tests check tree creation for buildable cases and retain the expected RuntimeError for an unmapped port without a default.

Priority: ⬆️ High

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to b61a3

The change addresses empty defaults for XML subtree models, with regression assertions for empty, non-empty, remapped-empty, and missing defaults. No actionable merge-blocking risk is established.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The parser change satisfies the core [#22069] cases. It uses PortInfo::defaultValue().empty() and retains defaultValueString() for the fallback value. EmptyModelDefaultIsNotMandatory tests omitt…	Add or cite reviewable evidence for the C++ empty-string default rule and each requested validation area. If existing behavior satisfies an item, add focused tests or documentation that establishes it.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes are limited to subtree-model default handling and its regression test. The parser fix and test cases directly support [#22069]. The test assertion changes improve failure diagnostics and r…
Human Review Check	✅ Passed	PASS: The PR changes only src/xml_parsing.cpp and tests/gtest_subtree.cpp. The source change is one targeted subtree-default validation change, and the test change adds coverage. The diff contains…
Description check	✅ Passed	The pull request description is complete and relevant. It explains the bug, fix, test coverage, validation results, formatting status, linked issue, and merge requirements. It also confirms that pre-c…

Full details: Linked Issues check

Explanation

The parser change satisfies the core [#22069] cases. It uses PortInfo::defaultValue().empty() and retains defaultValueString() for the fallback value. EmptyModelDefaultIsNotMandatory tests omitted and explicit empty defaults, non-empty defaults, and an absent default. The PR provides no implementation or tests for the issue's additional requests: the C++ empty-string default rule, server-side unbuildable-tree detection, registered-objective validation, objective_xml_string wrapper dispatch, frontend/runtime rule alignment, and integrator-style RC dispatch.

• Fix all pre-merge checks with AI

---

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