feat: ROS2 tools allowlist #486
Conversation
refactor: rename MessageReceiver to MessageSubscriber
|
@coderabbitai full review |
WalkthroughThe changes introduce a new base class, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StartROS2ActionTool
participant BaseROS2Tool
participant ActionServer
User->>StartROS2ActionTool: Request action start with action_name
StartROS2ActionTool->>BaseROS2Tool: is_writable(action_name)?
alt Not Writable
BaseROS2Tool-->>StartROS2ActionTool: False
StartROS2ActionTool-->>User: Raise ValueError
else Writable
BaseROS2Tool-->>StartROS2ActionTool: True
StartROS2ActionTool->>ActionServer: Start action
ActionServer-->>StartROS2ActionTool: Action ID/Success Response
StartROS2ActionTool-->>User: Return success message
end
sequenceDiagram
participant Client
participant CallROS2ServiceTool
participant BaseROS2Tool
participant ServiceServer
Client->>CallROS2ServiceTool: Invoke service call with service_name
CallROS2ServiceTool->>BaseROS2Tool: is_writable(service_name)?
alt Not Writable
BaseROS2Tool-->>CallROS2ServiceTool: False
CallROS2ServiceTool-->>Client: Raise ValueError
else Writable
BaseROS2Tool-->>CallROS2ServiceTool: True
CallROS2ServiceTool->>ServiceServer: Call service
ServiceServer-->>CallROS2ServiceTool: Service Response
CallROS2ServiceTool-->>Client: Return response
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/rai_core/rai/tools/ros2/base.py (2)
1-1: Fix incorrect copyright yearThe copyright year is set to 2025, but it should be 2024 to match the current year and be consistent with other files in the codebase.
-# Copyright (C) 2025 Robotec.AI +# Copyright (C) 2024 Robotec.AI
30-35: Improve parameter docstring clarityThe parameter descriptions should be more consistent and generic since they apply to topics, services, and actions.
- readable : Optional[List[str]] - The topics that can be read. If the list is not provided, all topics can be read. - writable : Optional[List[str]] - The names (topics/actions/services) that can be written. If the list is not provided, all topics can be written. + readable : Optional[List[str]] + The names (topics/actions/services) that can be read. If the list is not provided, all names can be read. + writable : Optional[List[str]] + The names (topics/actions/services) that can be written. If the list is not provided, all names can be written.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/rai_core/rai/tools/ros2/actions.py(6 hunks)src/rai_core/rai/tools/ros2/base.py(1 hunks)src/rai_core/rai/tools/ros2/services.py(2 hunks)src/rai_core/rai/tools/ros2/topics.py(9 hunks)tests/communication/ros2/__init__.py(1 hunks)tests/communication/ros2/helpers.py(2 hunks)tests/communication/ros2/test_api.py(6 hunks)tests/communication/ros2/test_connectors.py(2 hunks)tests/tools/ros2/test_action_tools.py(1 hunks)tests/tools/ros2/test_service_tools.py(1 hunks)tests/tools/ros2/test_topic_tools.py(5 hunks)
🧰 Additional context used
🧬 Code Definitions (7)
tests/communication/ros2/__init__.py (1)
tests/communication/ros2/helpers.py (3)
MessageSubscriber(98-108)multi_threaded_spinner(188-202)shutdown_executors_and_threads(205-215)
tests/communication/ros2/test_connectors.py (1)
tests/communication/ros2/helpers.py (1)
MessageSubscriber(98-108)
tests/tools/ros2/test_service_tools.py (1)
tests/communication/ros2/helpers.py (4)
ros_setup(219-222)ServiceServer(66-76)multi_threaded_spinner(188-202)shutdown_executors_and_threads(205-215)
tests/communication/ros2/test_api.py (1)
tests/communication/ros2/helpers.py (1)
MessageSubscriber(98-108)
src/rai_core/rai/tools/ros2/services.py (1)
src/rai_core/rai/tools/ros2/base.py (2)
BaseROS2Tool(22-58)is_writable(53-58)
src/rai_core/rai/tools/ros2/actions.py (3)
src/rai_core/rai/communication/ros2/connectors.py (1)
ROS2ARIConnector(44-194)src/rai_core/rai/communication/ros2/messages.py (1)
ROS2ARIMessage(39-41)src/rai_core/rai/tools/ros2/base.py (2)
BaseROS2Tool(22-58)is_writable(53-58)
tests/tools/ros2/test_action_tools.py (1)
tests/communication/ros2/helpers.py (3)
ros_setup(219-222)multi_threaded_spinner(188-202)shutdown_executors_and_threads(205-215)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test-ros2 (humble)
🔇 Additional comments (52)
tests/communication/ros2/__init__.py (1)
18-18: Good renaming of the helper classThe renaming from
MessageReceivertoMessageSubscriberis a clearer and more descriptive name that better reflects the class's purpose in the ROS2 subscription pattern.Also applies to: 26-26
tests/communication/ros2/test_connectors.py (1)
35-35: Consistent class renaming applied correctlyThe import statement and instance creation have been properly updated to use
MessageSubscriberinstead ofMessageReceiver, maintaining consistency with the class renaming in the helpers module.Also applies to: 49-49
tests/communication/ros2/test_api.py (2)
35-35: Import statement updated correctlyThe import statement has been properly updated to use
MessageSubscriberinstead ofMessageReceiver.
50-50: Variable names consistently renamedAll variable instances have been properly renamed from
message_receivertomessage_subscriberto maintain consistency with the class renaming. The thoroughness in updating all occurrences is commendable.Also applies to: 130-130, 150-150, 171-171, 192-192
src/rai_core/rai/tools/ros2/services.py (2)
27-27: Good implementation of BaseROS2Tool inheritanceThe class now properly inherits from
BaseROS2Toolwhich provides the common functionality for ROS2 tools, including access control for topic/service/action operations. This refactoring aligns with the allowlist feature implementation.Also applies to: 38-38
46-47: Effective permission validation for servicesGood addition of the permission check that ensures a service can only be called if it's on the writable list. This implements the allowlist feature described in the PR objective, helping to restrict LLM interaction to only specified services.
src/rai_core/rai/tools/ros2/base.py (2)
22-45: LGTM: Good implementation of the base class for ROS2 toolsThe BaseROS2Tool class provides a solid foundation for implementing the allowlist functionality. The class structure and inheritance from BaseTool are appropriate.
46-58: LGTM: Well-implemented permission check methodsThe is_readable and is_writable methods have appropriate logic for checking permissions. The forbidden list takes precedence, followed by readable/writable lists, with a default of allowing all if those lists are None.
tests/tools/ros2/test_service_tools.py (2)
56-67: LGTM: Good test for forbidden service behaviorThe test correctly verifies that attempting to call a forbidden service raises a ValueError. The test's structure follows the same pattern as other tests in the file.
70-87: LGTM: Good test for writable service behaviorThe test correctly verifies that calling a service that's in the writable list works as expected. The structure follows the same pattern as other tests, including proper setup and teardown of resources.
tests/tools/ros2/test_action_tools.py (2)
56-67: LGTM: Good test for forbidden action behaviorThe test correctly verifies that attempting to start an action that is marked as forbidden raises a ValueError. The test follows the established pattern of other tests in the file.
70-87: LGTM: Good test for writable action behaviorThe test correctly verifies that starting an action that's in the writable list works as expected. The test includes proper setup of the action server and verification of the response.
tests/communication/ros2/helpers.py (2)
17-17: LGTM: Updated import to include Any typeThe import statement was correctly updated to include the Any type needed for the updated MessageSubscriber class.
98-108: LGTM: Good refactoring of MessageReceiver to MessageSubscriberRenaming the class to MessageSubscriber better reflects its purpose. The addition of the
msg_typeparameter with a default of String makes the class more flexible while maintaining backward compatibility.tests/tools/ros2/test_topic_tools.py (7)
42-42: Renaming consistency.The renaming to
MessageSubscriberaligns with the updated naming scheme and is consistent throughout the code.
71-83: Good test coverage for forbidden topic.This test properly ensures a
ValueErroris raised when attempting to publish to a forbidden topic, covering the negative path for write operations.
85-103: Validating writable topic logic.Publishing to a writable topic is validated here, and the test confirms the message is received correctly. This scenario thoroughly checks the positive path for write operations.
139-147: Verifying forbidden topic read logic.This test covers the scenario where read operations on forbidden topics must raise a
ValueError, confirming correct exception handling.
149-167: Ensuring readable topic functionality.The test exercises reading from an explicitly allowed topic, validating successful reception of image data. This effectively checks the positive path for read operations.
178-186: Confirming forbidden topic exclusion.Ensures that forbidden topics (like
/rosouthere) are properly excluded from the response. This prevents unintended visibility to restricted topics.
188-197: Verifying readable topic inclusion.Checks that the configured readable topic is included in the response, confirming the read-allowlist mechanism works as expected.
src/rai_core/rai/tools/ros2/actions.py (11)
26-26: Additional typing imports.Importing
Annotatedand other typing utilities is helpful for consistent type annotations and improved readability.
32-32: Switching to BaseROS2Tool.Using
BaseROS2Toolprovides unified permission checks (readable, writable, forbidden). This aligns action-related tools with topic and service tools.
39-58: Introducing access control fields.Defining
readable,writable, andforbiddenin theROS2ActionToolkitenables fine-grained control over action usage. This addition is consistent with the overall allowlist approach.
66-68: Model config allowance.Allowing arbitrary types in the model config ensures the fields storing ROS2 connectors and locks are permitted without pydantic validation issues.
70-79: Passing access control fields to action tools.This ensures each tool enforces permission checks against the relevant lists, preserving the single responsibility for permission logic in the tools.
81-84: ExtendingCancelROS2ActionToolwith permission fields.By including
readable,writable, andforbidden, the tool can uniformly enforce action-based permissions, even in cancellation flows.
87-90: Including permission fields inGetROS2ActionFeedbackTool.Uniformly enforces read restrictions when retrieving feedback from an action.
122-122: Refactoring toBaseROS2Tool.Inheriting from
BaseROS2Toolcentralizes permission checks for theStartROS2ActionTool.
134-135: Write permission check for actions.Ensuring the action name is writable prevents starting an action on a forbidden or read-only resource.
153-153: Feedback retrieval inherits fromBaseROS2Tool.Creates consistency with the rest of the tools and maintains a uniform approach to permission checks.
196-196: Unifying cancel action tool.Switching to
BaseROS2Toolmaintains consistent permission enforcement across all action-related operations.src/rai_core/rai/tools/ros2/topics.py (20)
23-23: Extended typing imports.Bringing in
Annotated,Literal, etc., improves type safety and clarity in the code.
37-37: Consistent use ofBaseROS2Tool.Transitioning from
BaseTooltoBaseROS2Toolensures the built-in permission checks are leveraged for topic interactions.
45-64: Adding allowlist and denylist fields.
readable,writable, andforbiddeninROS2TopicsToolkitalign with the approach to control inbound/outbound topics. This design systematically enforces constraints at the toolkit level.
72-77: Publish tool constructor with permission fields.The constructor now passes
readable,writable, andforbiddenfor robust access control on publish operations.
78-83: Receive tool constructor with permission fields.Similarly, including the same fields ensures read operations undergo uniform checks.
84-89: Image tool constructor with permission fields.Ensures
GetROS2ImageToolalso enforces read restrictions consistently when retrieving images from topics.
90-95: Transform tool constructor with permission fields.
GetROS2TransformToolsimilarly inherits the base to enforce read access constraints, preventing transforms from forbidden or unreadable frames.
96-101: Topics names/types tool constructor with permission fields.In
GetROS2TopicsNamesAndTypesTool, we can now exclude forbidden topics from queries and categorize the rest as readable, writable, or both.
102-107: Message interface tool constructor with permission fields.Allows read/writable checks when retrieving message interfaces, maintaining parity with other tools.
117-117:PublishROS2MessageToolextendsBaseROS2Tool.This refactor standardizes permission handling for publishing to a ROS2 topic.
123-124: Ensuring topic is writable before publish.Raises
ValueErrorotherwise, protecting from unintended writes to restricted topics.
137-137:ReceiveROS2MessageToolinherits fromBaseROS2Tool.This alignment guarantees read checks are consistently applied for message reception.
144-145: Verifying readability before receiving.Blocking access to disallowed topics with a clear error message fosters transparent permission enforcement.
155-155:GetROS2ImageToolinherits fromBaseROS2Tool.Centralizes code for read checks, matching the approach for other read-based tools.
165-166: Permission check on images.Raising an error for unreadable topics ensures consistent guardrails when fetching images.
186-186:GetROS2TopicsNamesAndTypesToolextendsBaseROS2Tool.Provides unified read/write/forbid checks for topic introspection.
193-193: Conditional check for access lists.The logic handles the “no restrictions set” scenario gracefully by including all topics if none of the attributes are defined.
194-218: Detailed filtering of topics.Categorizing topics into readable-only, writable-only, or both ensures clear visibility of allowed and forbidden resources.
226-226:GetROS2MessageInterfaceToolinheritsBaseROS2Tool.Ensures the tool can leverage the same permission system if future read checks are introduced for message types.
258-258: Aligned withBaseROS2Toolfor transforms.
GetROS2TransformToolnow follows the same consistent pattern of permission checks as other ROS2 tools.
There was a problem hiding this comment.
@maciejmajek Looks good! Left minor comments. Please fix CI ;)
Purpose
RAI 1.x had a allowlist feature in StateBasedAgent, allowing the LLM to interact with certain topics only. This PR reintroduces the feature with extension to readable, writable and forbidden names (topics/services/actions)
Proposed Changes
Issues
Closes #479
Testing
Tests
Summary by CodeRabbit
New Features
Refactor
Tests