refactor(topics): migrate to rosapi_service()/rosapi_type(), add tool tests (step3)

ros_mcp/tools/connection.py

@@ -57,17 +57,24 @@ def connect_to_robot(
         ping_result = ping_ip_and_port(actual_ip, actual_port, ping_timeout, port_timeout)
 
         # Detect ROS version and cache rosapi type resolver
+        detection_warning = None
         if ping_result.get("port_check", {}).get("open"):
             try:
                 detect_rosapi_types(ws_manager)
-            except Exception:
-                pass  # Detection failure is non-fatal; tools will use defaults
+            except Exception as e:
+                detection_warning = (
+                    f"ROS version detection failed: {e}. "
+                    "Tools will assume ROS 2 type format (rosapi_msgs/srv/*)."
+                )
 
         # Combine the results
-        return {
+        result: dict[str, Any] = {
             "message": f"WebSocket IP set to {actual_ip}:{actual_port}",
             "connectivity_test": ping_result,
         }
+        if detection_warning:
+            result["warning"] = detection_warning
+        return result
 
     @mcp.tool(
         description=(

ros_mcp/tools/nodes.py

@@ -4,6 +4,7 @@
 from mcp.types import ToolAnnotations
 
 from ros_mcp.utils.response import _check_response, _safe_get_values
+from ros_mcp.utils.rosapi_types import rosapi_service, rosapi_type
 from ros_mcp.utils.websocket import WebSocketManager
 
 
@@ -31,8 +32,8 @@ def get_nodes() -> dict:
         # rosbridge service call to get node list
         message = {
             "op": "call_service",
-            "service": "/rosapi/nodes",
-            "type": "rosapi/Nodes",
+            "service": rosapi_service("nodes"),
+            "type": rosapi_type("Nodes"),
             "args": {},
             "id": "get_nodes_request_1",
         }
@@ -91,8 +92,8 @@ def get_node_details(node: str) -> dict:
         # rosbridge service call to get node details
         message = {
             "op": "call_service",
-            "service": "/rosapi/node_details",
-            "type": "rosapi/NodeDetails",
+            "service": rosapi_service("node_details"),
+            "type": rosapi_type("NodeDetails"),
             "args": {"node": node},
             "id": f"get_node_details_{node.replace('/', '_')}",
         }

ros_mcp/tools/topics.py

@@ -11,6 +11,7 @@
 
 from ros_mcp.tools.images import _encode_image_to_imagecontent, convert_expects_image_hint
 from ros_mcp.utils.response import _check_response, _safe_get_values
+from ros_mcp.utils.rosapi_types import rosapi_service, rosapi_type
 from ros_mcp.utils.websocket import WebSocketManager, parse_input
 
 
@@ -38,8 +39,8 @@ def get_topics() -> dict:
         # rosbridge service call to get topic list
         message = {
             "op": "call_service",
-            "service": "/rosapi/topics",
-            "type": "rosapi/Topics",
+            "service": rosapi_service("topics"),
+            "type": rosapi_type("Topics"),
             "args": {},
             "id": "get_topics_request_1",
         }
@@ -87,8 +88,8 @@ def get_topic_type(topic: str) -> dict:
         # rosbridge service call to get topic type
         message = {
             "op": "call_service",
-            "service": "/rosapi/topic_type",
-            "type": "rosapi/TopicType",
+            "service": rosapi_service("topic_type"),
+            "type": rosapi_type("TopicType"),
             "args": {"topic": topic},
             "id": f"get_topic_type_request_{topic.replace('/', '_')}",
         }
@@ -149,8 +150,8 @@ def get_topic_details(topic: str) -> dict:
             # Get topic type
             type_message = {
                 "op": "call_service",
-                "service": "/rosapi/topic_type",
-                "type": "rosapi/TopicType",
+                "service": rosapi_service("topic_type"),
+                "type": rosapi_type("TopicType"),
                 "args": {"topic": topic},
                 "id": f"get_topic_type_{topic.replace('/', '_')}",
             }
@@ -163,8 +164,8 @@ def get_topic_details(topic: str) -> dict:
             # Get publishers for this topic
             publishers_message = {
                 "op": "call_service",
-                "service": "/rosapi/publishers",
-                "type": "rosapi/Publishers",
+                "service": rosapi_service("publishers"),
+                "type": rosapi_type("Publishers"),
                 "args": {"topic": topic},
                 "id": f"get_publishers_{topic.replace('/', '_')}",
             }
@@ -177,8 +178,8 @@ def get_topic_details(topic: str) -> dict:
             # Get subscribers for this topic
             subscribers_message = {
                 "op": "call_service",
-                "service": "/rosapi/subscribers",
-                "type": "rosapi/Subscribers",
+                "service": rosapi_service("subscribers"),
+                "type": rosapi_type("Subscribers"),
                 "args": {"topic": topic},
                 "id": f"get_subscribers_{topic.replace('/', '_')}",
             }
@@ -226,8 +227,8 @@ def get_message_details(message_type: str) -> dict:
         # rosbridge service call to get message details
         message = {
             "op": "call_service",
-            "service": "/rosapi/message_details",
-            "type": "rosapi/MessageDetails",
+            "service": rosapi_service("message_details"),
+            "type": rosapi_type("MessageDetails"),
             "args": {"type": message_type},
             "id": f"get_message_details_request_{message_type.replace('/', '_')}",
         }

ros_mcp/utils/rosapi_types.py

@@ -149,7 +149,7 @@ def detect(self, ws_manager: WebSocketManager) -> None:
                         )
                         logger.info("ROS 1 distro: '%s'", self._distro)
         except Exception as e:
-            logger.debug("ROS 1 distro detection failed (non-fatal): %s", e)
+            logger.warning("ROS 1 distro detection failed (non-fatal): %s", e)
 
     def _reset(self) -> None:
         """Reset detection state. For testing only."""
@@ -168,8 +168,18 @@ def distro(self) -> str:
         return self._distro
 
     def get_type(self, short_name: str) -> str:
-        """Return the version-appropriate type string."""
-        type_prefix = _TYPE_PREFIX[self.version]
+        """Return the version-appropriate type string.
+
+        Falls back to ROS 2 format (rosapi_msgs/srv/) when version is unknown.
+        """
+        if self._version is None:
+            logger.warning(
+                "ROS version unknown — falling back to ROS 2 type format for '%s'",
+                short_name,
+            )
+            type_prefix = _TYPE_PREFIX[RosVersion.ROS2]
+        else:
+            type_prefix = _TYPE_PREFIX[self._version]
         return f"{type_prefix}/{short_name}"
 
     def get_service(self, service_name: str) -> str:

tests/integration/scripts/run-connection-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run connection integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-connection-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" connection

tests/integration/scripts/run-detect-version-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run detect_version integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-detect-version-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" detect_version

tests/integration/scripts/run-nodes-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run nodes integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-nodes-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" nodes

tests/integration/scripts/run-tests.sh

@@ -1,12 +1,25 @@
 #!/bin/bash
 # Run integration tests against a specific ROS distro.
-# Usage: ./tests/integration/scripts/run-tests.sh <distro>
+# Usage: ./tests/integration/scripts/run-tests.sh <distro> [module]
 # Example: ./tests/integration/scripts/run-tests.sh noetic
+# Example: ./tests/integration/scripts/run-tests.sh humble topics
 
 set -e
 cd "$(git rev-parse --show-toplevel)"
 
-DISTRO="${1:?Usage: $0 <melodic|noetic|humble|jazzy>}"
+if [ -z "${1:-}" ]; then
+    MODULES=$(ls tests/integration/test_*.py 2>/dev/null \
+        | sed 's|tests/integration/test_||;s|\.py||' \
+        | grep -v quick_detect \
+        | tr '\n' ', ' | sed 's/,$//')
+    echo "Usage: $0 <distro> [module]"
+    echo "Distros: melodic, noetic, humble, jazzy"
+    echo "Modules: $MODULES"
+    exit 1
+fi
+
+DISTRO="$1"
+MODULE="${2:-}"
 COMPOSE="tests/integration/docker-compose.yml"
 
 declare -A DOCKERFILES=(
@@ -54,7 +67,17 @@ uv run python tests/integration/test_quick_detect.py
 # Run pytest
 echo ""
 echo "--- Pytest ---"
-uv run pytest tests/integration/ -v --ros-distro "$DISTRO" --skip-compose
+if [ -n "$MODULE" ]; then
+    TEST_PATH="tests/integration/test_${MODULE}.py"
+    if [ ! -f "$TEST_PATH" ]; then
+        echo "Unknown module: $MODULE"
+        echo "Available: $(ls tests/integration/test_*.py | sed 's|tests/integration/test_||;s|\.py||' | tr '\n' ' ')"
+        exit 1
+    fi
+    uv run pytest "$TEST_PATH" -v --ros-distro "$DISTRO" --skip-compose
+else
+    uv run pytest tests/integration/ -v --ros-distro "$DISTRO" --skip-compose
+fi
 
 # Tear down
 echo ""

tests/integration/scripts/run-topics-tests.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+# Run topics integration tests against a specific ROS distro.
+# Usage: ./tests/integration/scripts/run-topics-tests.sh <distro>
+exec "$(dirname "$0")/run-tests.sh" "${1:?Usage: $0 <melodic|noetic|humble|jazzy>}" topics

tests/integration/test_nodes.py

@@ -0,0 +1,97 @@
+"""Integration tests for node tools (step 2).
+
+These tests call the actual MCP tool functions (get_nodes, get_node_details)
+against a live rosbridge container.
+
+Note: calling get_node_details for nonexistent nodes crashes rosapi_node
+on ROS 2 (#273). Tests only query nodes confirmed to exist.
+"""
+
+import pytest
+
+pytestmark = [pytest.mark.integration]
+
+
+class TestGetNodes:
+    """Verify get_nodes MCP tool returns the running node list."""
+
+    def test_returns_nodes(self, tools):
+        """get_nodes should return nodes and node_count."""
+        result = tools["get_nodes"]()
+        assert "nodes" in result
+        assert "node_count" in result
+        assert result["node_count"] > 0
+        assert result["node_count"] == len(result["nodes"])
+
+    def test_includes_turtlesim(self, tools):
+        """turtlesim node should be present (launched by Docker container)."""
+        result = tools["get_nodes"]()
+        nodes = result["nodes"]
+        assert any("/turtlesim" in n for n in nodes), f"turtlesim not in {nodes}"
+
+    def test_includes_rosbridge(self, tools):
+        """rosbridge node should be present."""
+        result = tools["get_nodes"]()
+        nodes = result["nodes"]
+        assert any("rosbridge" in n for n in nodes), f"rosbridge not in {nodes}"
+
+    def test_node_count_at_least_three(self, tools):
+        """Should have at least 3 nodes: turtlesim, rosbridge, rosapi."""
+        result = tools["get_nodes"]()
+        assert result["node_count"] >= 3, (
+            f"Expected >= 3, got {result['node_count']}: {result['nodes']}"
+        )
+
+    def test_all_nodes_are_ros_names(self, tools):
+        """Every node name should be a string starting with /."""
+        result = tools["get_nodes"]()
+        for node in result["nodes"]:
+            assert isinstance(node, str), f"Expected string, got {type(node)}: {node}"
+            assert node.startswith("/"), f"Node name should start with /: {node}"
+
+
+class TestGetNodeDetails:
+    """Verify get_node_details MCP tool returns publishers/subscribers/services.
+
+    Only queries nodes confirmed to exist — calling get_node_details for
+    nonexistent nodes crashes rosapi_node on ROS 2 (#273).
+    """
+
+    def test_turtlesim_details(self, tools):
+        """get_node_details for /turtlesim should return publishers and subscribers."""
+        result = tools["get_node_details"](node="/turtlesim")
+        assert result["node"] == "/turtlesim"
+        assert result["publisher_count"] > 0, "turtlesim should have publishers"
+        assert result["subscriber_count"] > 0, "turtlesim should have subscribers"
+        assert any("pose" in p for p in result["publishers"]), f"No pose in {result['publishers']}"
+        assert any("cmd_vel" in s for s in result["subscribers"]), (
+            f"No cmd_vel in {result['subscribers']}"
+        )
+
+    def test_rosbridge_has_services(self, tools):
+        """get_node_details for rosbridge should return services."""
+        # Find rosbridge node name dynamically (varies by distro)
+        nodes_result = tools["get_nodes"]()
+        rosbridge = next((n for n in nodes_result["nodes"] if "rosbridge" in n), None)
+        assert rosbridge is not None
+
+        result = tools["get_node_details"](node=rosbridge)
+        assert result["node"] == rosbridge
+        assert result["service_count"] > 0, "rosbridge should have services"
+
+    def test_detail_counts_match_lists(self, tools):
+        """publisher_count/subscriber_count/service_count should match list lengths."""
+        result = tools["get_node_details"](node="/turtlesim")
+        assert result["publisher_count"] == len(result["publishers"])
+        assert result["subscriber_count"] == len(result["subscribers"])
+        assert result["service_count"] == len(result["services"])
+
+    def test_empty_node_name_returns_error(self, tools):
+        """get_node_details with empty string should return error dict."""
+        result = tools["get_node_details"](node="")
+        assert "error" in result
+
+    def test_whitespace_node_name_returns_error(self, tools):
+        """get_node_details with whitespace should return error dict."""
+        result = tools["get_node_details"](node="   ")
+        assert "error" in result