refactor(ros): the rclpy transport lives below both robots that publish through it - #3873
Conversation
…sh through it The in-process rclpy node, the dynamic type resolution, the graph introspection, the pub/sub/service calls and the action-goal lifecycle lived inside the use_ros @tool, and both ROS 2 mesh robots imported that tool to reach them - two of the four drivers|mesh -> tools inversions the layer graph declares, and the last two that were a transport rather than a verb package. They move to strands_robots.ros, which sits in the same layer as its mesh callers. The tool keeps what is the agent's half of the call: the numeric-option domains, the operator gate and the docstring a model reads, routing each verb's arguments into ros_action. The gate is a required keyword-only argument of ros_action rather than a default, and all three callers pass the transport's own GATE_TOOL label, so one cmd_vel publish files one interrupt id and one audit source however it was reached. A caller cannot forget it and cannot move it.
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the in-process rclpy transport (node/executor singleton, dynamic type resolution, graph introspection, pub/sub/service calls, action-goal lifecycle with timeout cancel) out of the use_ros @tool into a new strands_robots.ros module, so RosBridgedRobot and AckermannRosRobot no longer import upward from the drivers|mesh layer into tools. The transport's ros_action takes the operator gate as a required keyword-only argument (CommandGate), all three callers key it with the shared GATE_TOOL = "use_ros" label, and read-only verbs are wired through the single never_gated spelling. I diffed the moved transport code against the base use_ros.py — it is a faithful move (annotations added, behavior identical), name/type validation and the gate-after-probe/after-required-args ordering are preserved, and the layer-roster pin plus the reworked AST gate suites cover the new shape. One blocker: a test this PR modified fails deterministically on the head SHA (inline comment).
What's good
- Gate as a required keyword-only argument closes the forgot-to-forward-context hole structurally;
never_gatedgives read paths one auditable spelling. GATE_TOOLshared across all three callers keeps one interrupt id / audit source per physical surface.- The AST gate suites were updated to grade the new
gate=_operator_gate(tool_context)shape rather than being deleted, and the layer test pins the new module's placement. - Moved code verified byte-equivalent modulo type annotations; error prefix (
use_ros:) unchanged for callers.
Verification suggestions
pytest tests/mesh/test_bridge_stop_tool_parity.py -qon the head SHA reproduces the blocker (fails ona8b053dc, passes on base969a110a).
strands-labs#3864 landed the sibling RTPS cut on main. Both changes coexist: the layer roster carries a row per shared transport, the cross-transport gate suite becomes one table with a row per transport that has a second caller, and the recorded-call comparisons drop the gate, which is a fresh closure per call and never on the wire.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the in-process rclpy transport (process-wide node/executor singleton, dynamic type resolution, graph introspection, pub/sub/service calls, action-goal lifecycle with timeout cancel) out of the use_ros @tool into a new strands_robots.ros module, removing the two drivers|mesh -> tools upward import edges the mesh robots paid to reach a publisher. The moved code is byte-faithful to the original (including the monotonic-clock spin_for contract and the cancel-before-timeout goal path), and the safety posture is preserved: ros_action takes the operator gate as a required keyword-only argument with no default a new caller could silently inherit, the gate is consulted at the same point in the flow as before (after the backend probe and the verb's required-argument checks), and all three callers key it with the transport's own GATE_TOOL label so one cmd_vel publish files one interrupt id and one audit source whichever surface asked. The prior review's blocker on test_bridge_stop_tool_parity.py (fresh gate closures breaking kwargs-dict equality) is resolved at this head via the _OFF_THE_WIRE filtering.
What's good
- The gate-as-required-argument design is the safe shape: forgetting it is a
TypeError, not an ungated command, andnever_gatedis the single spelling for read verbs so a caller cannot invent a permissive gate. - The gate suites were re-pointed at the new seam rather than deleted, and the shared-transport parity pins (
TestEveryCallerOfOneTransportAsksTheSameQuestion) now grade both transports that have a second caller. - Layer roster pin in
test_import_layers_are_a_dag.pyfails on main without the new module, per the pre-fix control described.
Verification suggestions
python3 -m pytest tests/mesh tests/tools tests/test_use_ros_command_blocklist.py tests/test_every_ros_transport_consults_the_command_gate.py tests/test_hitl_operator_response_audit.py tests/test_import_layers_are_a_dag.py -q- all PR-touched suites pass at this head (888 passed in a local spot-check; unrelated IoT-transport failures on a box with a staleawscrtare pre-existing).python3 scripts/check_import_layers.py- reports 0 runtime cycles and 1 declared upward edge, matching the PR table.- Note the PR currently shows a merge conflict with
main; a mechanical re-merge is needed before the button goes green.
The module-level logger was carried over from the tool with the code that moved, and nothing in strands_robots.ros logs. Dead by the no-dead- code rule, and the CodeQL unused-global thread names it.
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the in-process rclpy transport (process-wide node/executor singleton, dynamic rosidl_runtime_py type resolution, graph introspection, pub/sub/service calls, and the action-goal lifecycle with its monotonic-deadline timeout cancel) out of the use_ros @tool into a new strands_robots.ros module, so RosBridgedRobot and AckermannRosRobot no longer import upward from drivers|mesh into tools. The tool keeps the agent envelope (numeric-option domains, operator gate, model-facing docstring) and routes each verb into ros_action. Verified the moved backend/helper code against the pre-move use_ros.py: byte-faithful apart from added type annotations (Any on node/executor, Callable[[], bool] on spin_for, str() cast in _resolve_topic_type) and docstring wording — including the gate-inside-lock ordering, which is unchanged from main. The safety posture holds: gate is a required keyword-only argument on ros_action, all three command call sites in each caller build it from tool_context under the shared GATE_TOOL label (AST-pinned in both gate suites plus the dispatch-source guard in test_use_ros_command_blocklist.py), and reads are wired to the single never_gated spelling. The prior [MUST FIX] on test_bridge_stop_tool_parity.py (fresh gate closures breaking kwargs-equality) is resolved via the _OFF_THE_WIRE filter here and in test_drive_contract_fleet_scope.py, and the CodeQL unused-logger finding is fixed at the head commit.
What's good
scripts/check_import_layers.pyruns clean on this head: 0 runtime cycles, 0 upward edges,KNOWN_UPWARD_EDGESemptied rather than grown, and the layer roster pin intest_import_layers_are_a_dag.pygains thestrands_robots.rosrow.- All 18 PR-touched test files pass locally (941 passed, 7 skipped); the only failures in the wider
tests/meshrun are pre-existingtest_iot_*/test_robot_mesh_*environment issues on files this PR does not touch (0 diff lines). - Ruff clean on the four changed package modules; no non-ASCII characters introduced (AGENTS.md Unicode hygiene sweep).
- The
_operator_gatefactory docstring andGATE_TOOLcomment spell out the one-label/one-audit-source invariant, andtest_every_ros_transport_consults_the_command_gate.pynow grades all three shared transports from one table.
Verification suggestions
python scripts/check_import_layers.pyon the head reportsupward runtime edges: 0 (declared 0)— the PR description table says 1 remaining upward edge / 1 declared inversion, which appears stale after the #3871/#3872 merges; worth a one-line description touch-up but nothing in the code disagrees with the grader.
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the in-process rclpy transport (process-wide node/executor singleton, dynamic rosidl_runtime_py type resolution, graph introspection, pub/sub/service calls, and the action-goal lifecycle with its monotonic-deadline timeout cancel) out of the use_ros @tool into a new strands_robots.ros module, so RosBridgedRobot and AckermannRosRobot stop importing upward from drivers|mesh into tools and KNOWN_UPWARD_EDGES empties. The moved backend/helper code is byte-faithful to the base use_ros.py apart from added annotations, and the gate posture is structurally sound: ros_action takes the gate as a required keyword-only argument, all three command call sites in each caller build it from tool_context under the shared GATE_TOOL label, and reads are wired through the single never_gated spelling, mirroring the already-merged strands_robots.rosbridge cut (#3871). One regression found on a changed path, flagged inline: the numeric-option guard stayed in the tool envelope, so the mesh bridges' action_send_goal path lost the timeout domain check it previously inherited through use_ros.
What's good
- Gate-as-required-argument closes the forgot-to-forward-context hole as a
TypeErrorrather than an ungated command; the AST gate suites were re-pointed at the newgate=_operator_gate(tool_context)shape rather than deleted. - The module shape (
GATE_TOOL,never_gated,<name>_action(..., *, gate)) matches thestrands_robots.rosbridgeprecedent, so the three transports onto a ROS graph now share one seam grammar. - Layer roster pin gains the
strands_robots.rosrow and the pre-fix control on clean main is documented in the description.
Verification suggestions
- Repro for the inline finding (no ROS needed for the guard half):
RosBridgedRobot("tb", "/cmd_vel", "/odom", nav_action="/navigate_to_pose").navigate_to(1.0, 1.0, timeout=float("inf"))returned a structuredtimeout must be > 0refusal on the base and now proceeds into the transport; with rclpy sourced and no action server, it blocks inwait_for_server(timeout_sec=inf)holding_backend.lock.
`navigate_to` forwarded `timeout` through the `use_ros` tool, whose envelope graded it. The transport this branch moved below the tools carries no such envelope, so every unusable budget reached the wire: `inf`, `0`, `-1`, `nan`, `"2"` and `None` were all forwarded, where `inf` makes `wait_for_server` and every spin unbounded inside the backend's process-wide lock - so every later ROS call in the process waits behind a goal that can never time out. `1e999` in a tool call's JSON is that value. The budget now takes `duration`'s domain in the same chain that grades the goal pose, which is where `get_pose` and `get_scan` already grade their waits: a transport reached by more than one surface honors the budget it is handed and states no domain of its own. The comment and the docstring that claimed the transport guarded it say where the guard lives. Pinned by `TestTheGoalBudgetIsGradedBeforeTheGoalIsSent` over the same UNUSABLE_TIMEOUTS table the read verbs use: 10 cells fail on the pre-fix head, and the bound `navigate_*` tool reports rather than blocks.
yinsong1986
left a comment
yinsong1986
left a comment
There was a problem hiding this comment.
Summary
Moves the in-process rclpy mechanics (process-wide node/executor singleton, rosidl_runtime_py type resolution, graph introspection, pub/sub/service calls, and the action-goal lifecycle with its monotonic-deadline timeout cancel) out of the use_ros @tool into a new strands_robots.ros module, eliminating the two upward drivers|mesh -> tools runtime edges and emptying KNOWN_UPWARD_EDGES. The operator gate becomes a required keyword-only gate argument on ros_action, consulted at one fixed point (after the backend probe and required-arg checks) for exactly the three command verbs, with never_gated as the single sanctioned spelling for reads; all three callers key the gate with the shared GATE_TOOL label so one physical command files one interrupt id and one audit source. I verified the concern from the prior review round is resolved at this head: navigate_to now grades timeout via positive_finite_number_error in the same guard chain as the goal pose (ros_bridge.py:370), with a dedicated regression class (TestTheGoalBudgetIsGradedBeforeTheGoalIsSent) covering the programmatic path, the bound agent tool, and refusal ordering. The remaining seams all grade their own numeric domains (drive duration/count, construction-time publish_rate, read timeouts), so nothing reaches the transport unguarded.
What's good
- The gate-wiring test coverage moved with the code rather than being weakened: the AST structural pin in
test_use_ros_command_blocklist.pynow distinguishesoperator_gatefromnever_gatedper verb and asserts both sets are non-trivially populated, and_SHARED_TRANSPORTSgained the ros row so the same-question-same-label invariant is graded for all three transports. - Behavior-preserving move verified against base: the gate is consulted at the same point relative to the backend lock as before, name/type allowlist regexes moved intact, and
_erroutput keeps theuse_ros:prefix agents already parse. - The two unused module loggers CodeQL flagged are gone, and the diff is clean of non-ASCII additions and host paths.
4 participants
RosBridgedRobotandAckermannRosRobotopened a publisher by importing theuse_ros@tool, so two library classes reached up into the agent-tool layer for their transport. The rclpy mechanics - process-wide node and executor, dynamic type resolution, graph introspection, pub/sub/service calls, the action-goal lifecycle with its timeout cancel - move tostrands_robots.ros, in the same layer as the two robots.use_roskeeps what an agent envelope owns: the numeric-option table, the operator gate, the model-facing docstring.ros_action(..., *, gate)is required keyword-only, and all three callers key it with the transport's ownGATE_TOOL, so onecmd_velpublish files one interrupt id and one audit source whichever surface asked.Rebased on
95397b43(the rosbridge cut landed meanwhile), so the before column is re-measured against that base.mesh.ros_bridge,mesh.ackermann_robot->tools.use_ros)KNOWN_UPWARD_EDGES = ()strands_robots/LOCtests/LOC+184 package lines are docstring: the new module's contract and the gate's
Args. The two unused module loggers CodeQL flagged are gone.Tests. Pre-fix control on clean
mainwith the new pins copied in:test_import_layers_are_a_dag.py1 failed / 23 passed (the placement row forstrands_robots.ros),tests/mesh/test_bridge_read_timeout_domain.py30 failed / 20 passed, and the two gate suites cannot import the transport at all. After:tests/mesh tests/tools tests/rtps+ the four gate/layer suites +tests/drivers/microduck= 10,402 passed / 24 skipped;scripts/check_whole_tree_graders.py6,215 passed / 77 skipped;ruff+mypyclean on 2,218 files;check_merge_base_overlap.pyreports no overlap.Serves #3818 §0.8-0.9: the layered DAG is now a test with no declared exceptions.