Skip to content

Reject port names containing whitespace#18

Merged
JWhitleyWork merged 4 commits intomainfrom
port-name-whitespace-validation
Apr 22, 2026
Merged

Reject port names containing whitespace#18
JWhitleyWork merged 4 commits intomainfrom
port-name-whitespace-validation

Conversation

@L4co77
Copy link
Copy Markdown

@L4co77 L4co77 commented Mar 19, 2026 •
edited
Loading

  • Add std::isspace check to IsAllowedPortName() to reject port names containing whitespace
  • Update CreatePort error message to mention the whitespace constraint
  • Add tests for whitespace rejection in port names

@L4co77 @claude
Preserve numeric types for literal subtree port values
d248867 
When literal values are passed to SubTree ports (not blackboard remapping),
detect numeric types (int64_t, double) before storing them in the child
blackboard. Previously all literals were stored as std::string, which caused
type-mismatch errors in Script expressions that tried to do arithmetic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@L4co77 L4co77 requested a review from dsobek March 19, 2026 10:45
@L4co77 L4co77 self-assigned this Mar 19, 2026
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch from 6bf9c7a to a96649c Compare March 19, 2026 10:47
dsobek
dsobek reviewed Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dsobek dsobek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, just going to give this a test in against MoveIt Pro before approving.

For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this https://github.com/PickNikRobotics/moveit_pro/issues/17640.

@L4co77
Copy link
Copy Markdown
Author

L4co77 commented Mar 23, 2026 •
edited
Loading

Makes sense, just going to give this a test in against MoveIt Pro before approving.

For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this PickNikRobotics/moveit_pro#17640.

Commit 8f86eb9 adds comprehensive name validation that fully covers our whitespace check and more. Merging upstream into our fork would make this PR's BT.CPP change redundant. Should I take on #17640?

@dsobek
Copy link
Copy Markdown
Collaborator

dsobek commented Mar 23, 2026 •
edited
Loading

Makes sense, just going to give this a test in against MoveIt Pro before approving.
For the forward port, could you evaluate if it makes sense to merge the upstream of behaviortree.cpp into our fork? In particular, I realized that some of the validation logic I pointed to you exists in the upstream master branch, and so there might be some duplicate work there. I created a separate issue for this PickNikRobotics/moveit_pro#17640.

Commit 8f86eb9 adds comprehensive name validation that fully covers our whitespace check and more. Merging upstream into our fork would make this PR's BT.CPP change redundant. Should I take on #17640?

If we still want to get this fixed for 8.10 I think we should apply this more minimal fix. Updating to the most recent state of the upstream behaviortree.cpp repo is a bit too risky of a change.
No rush on taking on #17640, I've loaded it onto the next iteration so that we can take care of it then. For the forward port, it might be nice to bundle this work together, but if it ends up being too much of a hassle, I think pointing main to the same behaviortree.cpp commit hash as we have in 8.10 (to include this change) is fine for the time being.

dsobek
dsobek reviewed Mar 23, 2026
View reviewed changes
Comment thread src/xml_parsing.cpp Outdated
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch 3 times, most recently from 9080eff to 56b976a Compare March 24, 2026 08:51
@L4co77 L4co77 requested a review from dsobek March 24, 2026 10:26
dsobek
dsobek reviewed Mar 26, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dsobek dsobek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #18 (comment)

@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch 2 times, most recently from 78c8456 to 9dfb6a2 Compare March 30, 2026 11:32
@L4co77 L4co77 marked this pull request as draft March 30, 2026 11:35
@L4co77 L4co77 marked this pull request as ready for review March 30, 2026 11:35
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch from 9dfb6a2 to 54bafc0 Compare March 30, 2026 11:37
@L4co77 L4co77 requested a review from dsobek March 30, 2026 11:37
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch from 54bafc0 to e71f68d Compare March 30, 2026 12:12
@L4co77
Copy link
Copy Markdown
Author

L4co77 commented Apr 7, 2026

@dsobek could you check? thanks

@dsobek dsobek force-pushed the port-name-whitespace-validation branch from d7130f2 to a13dd02 Compare April 7, 2026 22:15
@dsobek dsobek requested a review from shaur-k April 7, 2026 22:16
@dsobek
Copy link
Copy Markdown
Collaborator

dsobek commented Apr 7, 2026

Since I added a commit here, I should no longer be the person approving. Added @shaur-k to review.

@L4co77
Copy link
Copy Markdown
Author

L4co77 commented Apr 20, 2026

@shaur-k could you check this ? thanks

shaur-k
shaur-k requested changes Apr 20, 2026
View reviewed changes
Comment thread src/xml_parsing.cpp Outdated
L4co77 added a commit that referenced this pull request Apr 22, 2026
@L4co77
refactor: extract ThrowIfPortNameContainsWhitespace helper
178d3cd 
Deduplicates the whitespace validation that was inlined in both
CreatePort (basic_types.h) and loadSubtreeModel (xml_parsing.cpp).
Addresses shaur-k's review comment on PR #18.
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch from a13dd02 to 178d3cd Compare April 22, 2026 08:43
@L4co77
refactor: extract ThrowIfPortNameContainsWhitespace helper
dc99a79 
Deduplicates the whitespace validation that was inlined in both
CreatePort (basic_types.h) and loadSubtreeModel (xml_parsing.cpp).
Addresses shaur-k's review comment on PR #18.
@L4co77 L4co77 force-pushed the port-name-whitespace-validation branch from 178d3cd to dc99a79 Compare April 22, 2026 09:51
@L4co77 L4co77 requested a review from shaur-k April 22, 2026 10:09
JWhitleyWork
JWhitleyWork requested changes Apr 22, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@JWhitleyWork JWhitleyWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment.

Comment thread src/xml_parsing.cpp
@L4co77 L4co77 requested a review from JWhitleyWork April 22, 2026 18:31
@JWhitleyWork JWhitleyWork enabled auto-merge April 22, 2026 19:02
@JWhitleyWork JWhitleyWork disabled auto-merge April 22, 2026 19:02
@JWhitleyWork JWhitleyWork merged commit eef8901 into main Apr 22, 2026
8 checks passed
@JWhitleyWork JWhitleyWork deleted the port-name-whitespace-validation branch April 22, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JWhitleyWork JWhitleyWork JWhitleyWork approved these changes

@dsobek dsobek Awaiting requested review from dsobek

@shaur-k shaur-k Awaiting requested review from shaur-k

Assignees

@L4co77 L4co77

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@L4co77 @dsobek @JWhitleyWork @shaur-k