[MatthijsBurgh]

Description

Setting moveit_controller_manager to an empty string crashes TrajectoryExecutionManager::initialize() with a segfault.

initialize() already expects an empty value: if (!controller.empty()) skips loading the plugin and creating controller_mgr_node_. But further down it reads the trajectory_execution.* parameters from controller_mgr_node_ and registers its parameter callback there, and that node is still null at that point.

An empty value is useful when a MoveItCpp/MoveItPy instance only plans. MoveItCpp always creates a TrajectoryExecutionManager, and every instance with the normal MoveIt config loads the controller manager plugin and creates action clients for every controller. We hit this with a second MoveItPy instance used only for reachability checks. Its parameters come from the same params files, so a per-instance override (moveit_controller_manager: "") is the natural way to switch execution off there.

This PR returns early on an empty value with an INFO log, like the existing branch for a missing parameter whose plugin cannot be guessed ("No paths can be executed"). In that case no plugin, controller manager node or executor thread is created. Most of the diff is the try block losing one indentation level now that its if (!controller.empty()) wrapper is gone.

A regression test (test_no_controller_manager) builds the manager with moveit_controller_manager: "" on a RobotModelBuilder model. It segfaults without this change and passes with it, tested on jazzy, where this part of the file is identical.

Backports: please add the backport-jazzy and backport-kilted labels; I can't set labels on this repository. The commit cherry-picks cleanly onto both branches. The test links moveit_core::moveit_test_utils instead of the moveit_core::moveit_core umbrella target, because Jazzy and Kilted don't export that target.

Related but independent: #3828 fixes a destructor crash in the same file and doesn't touch initialize().

Checklist

• Required by CI: Code is auto formatted using clang-format
• Extend the tutorials / documentation reference
• Document API changes relevant to the user in the MIGRATION.md notes
• Create tests, which fail without this PR reference
• Include a screenshot if changing a GUI
• While waiting for someone to review your request, please help review another open pull request to support the maintainers

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Trajectory execution can now be configured without a controller manager.
  • Empty controller-manager settings are handled gracefully without creating controller infrastructure or reporting unavailable controllers.
  • Existing controller-manager configurations continue to initialize and operate as before.

• Tests

  • Added automated coverage to verify behavior when no controller manager is configured.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 82f527af-5b71-47d7-aeac-2caf2571ac43

📥 Commits

Reviewing files that changed from the base of the PR and between a9004a4 and fd62253.

📒 Files selected for processing: 3

• moveit_ros/planning/trajectory_execution_manager/CMakeLists.txt
• moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp
• moveit_ros/planning/trajectory_execution_manager/test/test_no_controller_manager.cpp

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

When moveit_controller_manager is empty, TrajectoryExecutionManager::initialize() logs that no controller manager is loaded and returns before controller-manager setup. A new test checks that no controller-manager objects or joint controllers are available.

Changes: Empty controller-manager handling

Layer / File(s)	Summary
Initialization behavior and regression test moveit_ros/planning/trajectory_execution_manager/src/trajectory_execution_manager.cpp, moveit_ros/planning/trajectory_execution_manager/test/test_no_controller_manager.cpp, moveit_ros/planning/trajectory_execution_manager/CMakeLists.txt	Initialization returns early when the controller-manager parameter is empty. A new GoogleTest target verifies that no controller manager or joint controller is available.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to fd622

The empty-controller-manager behavior and its regression test have no identified merge-blocking issue. Merge after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: preventing a crash when moveit_controller_manager is empty.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[MatthijsBurgh]

@henningkayser @tylerjw @v4hn Could you please review this PR?

[codecov]

Codecov Report

❌ Patch coverage is 92.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.97%. Comparing base (a9004a4) to head (fd62253).

Files with missing lines	Patch %	Lines
...ution_manager/src/trajectory_execution_manager.cpp	89.48%	2 Missing ⚠️
...cution_manager/test/test_no_controller_manager.cpp	95.24%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3936      +/-   ##
==========================================
+ Coverage   48.92%   48.97%   +0.05%     
==========================================
  Files         735      736       +1     
  Lines       62684    62709      +25     
  Branches     7617     7615       -2     
==========================================
+ Hits        30663    30704      +41     
+ Misses      32021    32005      -16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.