Port PickNik's fork onto upstream BehaviorTree.CPP 4.9.0

[dv-picknik]

[written by AI]

Important

DO NOT SQUASH. Merge commit only.
This branch is upstream tag 4.9.0 with our commits replayed on top, not a delta on main. A squash or rebase merge rewrites every SHA and severs the branch from tag 4.9.0, which breaks every future upstream merge, 4.10.0 included.

Advances the fork from the 4.7.2 line to upstream 4.9.0, which is what nav2's Jazzy debs are built against. This is the fix #20928 concluded was necessary. The full commit-by-commit breakdown is on moveit_pro#17640.

15 commits on upstream tag 4.9.0, plus the merge that records main. 525/525 tests pass.

Why a replay and not a merge

The fork's history contains duplicate commits. PR #15 rebased main onto the upstream mirror and then merged the pre-rebase line back in, so most features exist twice. A merge produces one unreviewable blob. Replaying commit by commit also answers the question that actually matters, which is whether each patch is still needed on 4.9.0. For 9 of the 22 the answer was no.

This is intentional and was agreed before the branch was built.

Since this branch was cut

main gained #28, #30, #32, #33 and #34 after 2026-08-04. All of it is carried here. Every fix found along the way is folded into the commit that caused it rather than stacked on top, so each commit still stands on its own.

From main	Where it lands here
13c9fe23 generic ReactiveSequence child IDs	its own commit, with eac4a51b folded in so the test never lands in the form that crashes the pixi Windows job. The gmock assertions are rewritten with std::string::find, since this branch does not link gmock
7b390305 the three red CI jobs	the ci: commit, reapplied rather than cherry-picked, together with ba2fa03b
c4b23ea5, b61a3989 empty subtree defaults	already here; c4b23ea5 is itself a backport of this branch's fix, and b61a3989 is folded into it

CI had never run on this branch before now. GitHub cannot build a merge ref for a conflicting PR, so no workflow ever started. Starting it surfaced five things nothing had ever checked. Each one is folded into the commit it came from.

• Three clang-format violations, one each in the commits that introduced them.
• A dead foonathan-lexy conan dependency, folded into the packaging commit. 4.9.0 deleted 3rdparty/lexy and nothing in the tree references it, but conan still built it from source on every Windows job, where it dies with "Could not create named generator Visual Studio 18 2026".
• gtest_json.cpp casting a uint64_t to uint, a POSIX typedef MSVC does not have. Folded into the JSON commit.
• A bitwise & between two bools in the port type-mismatch guard, folded into the vector<Any> commit. The value is unchanged, since both operands are side-effect-free locals.
• isVector never matched on MSVC. It matched ^std::vector<.*>$ against a demangled name, but MSVC has no demangler, so demangle() returns typeid().name() verbatim and that name reads class std::vector<double,class std::allocator<double> >. The anchor could never match, so the vector<Any> carve-out never applied and both PortTest.VectorAny and PortTest.LoopNodeAcceptsVector_Issue969 threw. Folded into the vector<Any> commit with a test pinning both toolchains' spellings.

The last three are on main too. They stayed invisible because the fork's Windows job died in conan before it ever reached a compiler or a test.

What this closes

All three ABI breaks from #20928:

#	Break	How 4.9.0 closes it
1	NodeConfig::auto_remapped inserted mid-struct, shifting manifest and everything after by 8 bytes	Ours to place. Appended, offsets verified byte-identical to stock
2	Fork reverted upstream BehaviorTree#965, dropping JsonExporter::from_json_array_converters_	We stop reverting it. The member is upstream's again
3	SimpleString move ctor gained noexcept in 4.9.0, flipping linb::any between heap and inline storage	Free, because we are 4.9.0

Break #3 is why the layout-patch approach on fix/20928-btcpp-abi was abandoned. Sizes, offsets and mangled names are all identical, so no layout diff can see it. That branch is retired.

ABI break #1, verified by measurement

offsetof against a stock 4.9.0 checkout, same toolchain:

member	stock 4.9.0	original fork	this branch
other_attributes	144	152	144
manifest	200	208	200
uid	208	216	208
path	216	224	216
pre_conditions	248	256	248
post_conditions	296	304	296

Warning

git cherry-pick auto-merges tree_node.h with no conflict and silently reinserts the member mid-struct. Resolving conflicts carefully is not enough to avoid reshipping the bug. A one-line offsetof assertion in the suite would have caught #20928 at the commit that introduced it, worth adding separately.

Names that 4.9.0 would refuse

4.9.0's new validateModelName rejects . / \ : < > & " ' * ? | and the space in <BehaviorTree ID> and <SubTree ID>. 4.7.2 did not validate model names at all, so every name an operator ever typed loads today. MoveIt Pro adds no validation of its own, so whatever an operator types becomes a model name.

e11edc21 carves out three characters. Each one appears in a config that works today:

	Where it comes from
' '	1948 violations across 334 files in moveit_pro and moveit_pro_example_ws, every one the space, including Close Gripper and Move to Pose. These IDs are referenced from saved customer configs.
'	Robot's Home, pinned by MoveIt Pro's REST suite on the grounds that supported names are XML attribute values rather than interpolated XPath expressions.
.	Test Presoak 1.2 in a customer workspace. Version-suffixed names are natural, and renaming is a migration we would be imposing for no benefit we can point at.

None of the three breaks what this validation exists for. All survive a filesystem round-trip, an apostrophe needs no escaping inside the double-quoted attribute value BT.CPP writes, and a dot is not structural here. Node paths are built from / and ::, and the . handling in script_tokenizer.cpp applies to script source, which model names never enter. Port names are a separate namespace and still reject . through IsAllowedPortName. This is ABI-safe, since validateModelName lives in src/xml_parsing.cpp rather than a header, and upstream already permits all three in instance names for the same readability reason.

The carve-out cannot be a comparison

findForbiddenChar returns only the first offender, so filtering on its result hides every later one. Pick & Place passed on the space while its & went unseen, and Robot's Home/Left on the apostrophe while its / did. The carved-out characters are removed first and the remainder is scanned, so the error still names the real offender.

This was live in the original space-only carve-out too, and CI stayed green because every case in the "everything else still throws" test put the offender first. That test now includes Pick & Place, Robot's Home/Left, Robot's <Home>, A B\C, Presoak 1.2|beta and v1.2:final.

Everything else still throws. < > & " break serialization, and / \ : * ? | collide with the path syntax above or with filesystem round-tripping. 4.9.0's ASCII-only script tokenizer and the tightened port-name rule ship as upstream wrote them. A scan of moveit_pro, moveit_pro_example_ws and the 30 most recently pushed PickNikRoboticsServices repos found zero occurrences of either, so they get a migration note rather than a carve-out.

A scan across the 30 most recently pushed PickNikRoboticsServices repos plus both of ours, 1280 BT XML files, reports 0 model-name rejections under the final rule, and no port-name, script, cycle, Root or instance-name findings.

Packaging collision found

4.9.0 adds tools/bt_nodes_model.cpp, installing /opt/ros/*/bin/bt4_nodes_model. Stock ros-jazzy-behaviortree-cpp 4.9.0 installs that exact path, so shipping it unrenamed is a hard dpkg "trying to overwrite" failure. Per the coexistence design's own pitfall section a path-exclude does not help, because dpkg's ownership check runs before the exclude filter. Renamed to bt4_picknik_nodes_model. This block also auto-merges silently, below the conflict region.

The LEXY_ENABLE_INSTALL guard is now obsolete, because 4.9.0 deleted 3rdparty/lexy outright and every remaining vendored tree is STATIC or INTERFACE with no install() rule.

Testing

• System build, gcc, gtest 1.14: 525/525
• pixi run build && pixi run test under cmake 4.4.3: 525/525
• Each commit that absorbed a fix builds and passes on its own. The count climbs 492, 492, 511, 512, 515, 516, 525 across them
• Red-first checks on both new fixes: reverting only the xml_parsing.cpp hunk fails 5 of the ReactiveSequence tests, and reverting only the isVector regex fails the MSVC spelling
• pre-commit run --all-files: passes
• Library builds as libbehaviortree_cpp_picknik.so.4.9.0 with versioned soname .so.4.9
• NodeConfig offsets measured against a stock 4.9.0 worktree
• Downstream, per apt_build_farm#65: 75 MoveIt Pro packages build against this branch, moveit_pro_behavior_interface 347 tests, moveit_pro_behavior 3672, moveit_studio_agent 431

One thing is left alone. gtest_subtree.cpp trips MSVC warning C4834 by discarding a [[nodiscard]] return, which upstream fixed in 4.9.1 with (void) casts. It is a warning, not an error, so it can fold in later rather than churn this history now.

One upstream test, PortTest.LoopNodeAcceptsVector_Issue969, failed mid-port. The fork's setOutput stores vectors as vector<Any>, which upstream's new LoopNode does not recognise. A real fork-versus-upstream collision, not a merge artifact, fixed in its own commit.

Decisions that want a reviewer

1. JSON primitive wire format. Fork emits {"__type":"double","value":3.14}, stock emits bare 3.14. Kept the fork's toJson, since the Pro UI consumes it, and made fromJson accept both. The tagged path is gated on __type naming a primitive rather than on a value field merely existing. Otherwise a registered custom type with its own value member, or the diagnostic blob the non-registered-type commit emits, imports silently as a std::string.
2. Vector to string format. Fork gives 1;2;3;4, stock gives json:[1,2,3,4], and for an empty vector "" against json:[]. Both round-trip. The fork form additionally covers types with a toStr<T> specialization but no JSON converter, such as vector<float>, which stock throws on. This is the easiest thing to drop if nothing downstream needs ;.
3. '5' .. 3 returns an empty Any. The widened are_numbers guard steals the concat branch. This is live in the fork today; the port neither introduces nor fixes it. One-token fix (&& op != concat), left alone to keep the port faithful.

Known and accepted

• Soname .so.4.7 to .so.4.9 is customer-visible. A customer shipping a prebuilt Behavior plugin .so in a derived image gets a load failure until they rebuild. This is the versioned soname working as the coexistence design intends, and it is an accepted risk for this release.
• Subtree ports now split across input_ports and output_ports. bt_flatbuffer_helper.h iterates only input_ports, so Groot1 serialization omits subtree OUTPUT ports.
• {=} expansion now applies to every node type, not just subtrees, a deliberate widening.

Follow-on work, separate PRs

• apt_build_farm needs a git_sha bump with revision reset to 1, then publishes 4.9.0-1noble. Staged in apt_build_farm#65.
• moveit_pro Dockerfile pin moves from 4.7.2-3noble to 4.9.0-1noble, the .so.4.7 strings move, and every Pro binary is rebuilt.
• 4.9.0's package.xml adds libsqlite3-dev, libzmq3-dev, tinyxml2 and tinyxml2_vendor, which the build-farm chroot must satisfy.
• Re-run the zero-file-intersection deb proof against a 4.9.0-derived build. Both sides now derive from the same tree, so any missed scoping is a direct hit.
• fix: drop SCOPED_TRACE that crashes the pixi Windows job #33 and ci: run push workflows on main #34 are either moot or a trivial rebase once this lands.

Refs PickNikRobotics/moveit_pro#17640, PickNikRobotics/moveit_pro#21174, PickNikRobotics/moveit_pro#20928, PickNikRobotics/moveit_pro#22069, PickNikRobotics/moveit_pro#22500

🤖 Generated with Claude Code

[JWhitleyWork]

Logic makes sense. Checked new dependencies for license compliance - seems good there too. We'll probably need to add a release note in bold like MUST REBUILD WORKSPACE AFTER THIS RELEASE but I doubt people will read it anyway.