further improvements to error handling (#13)

* handle no devices found cleanly

* remove magic strings in requests/response creation

* further error handling refinement

Author: gilesknap

src/usb_remote/__main__.py

@@ -131,7 +131,7 @@ def list_command(
 
         logger.debug(f"Listing remote USB devices on hosts: {servers}")
 
-        results = list_devices(server_hosts=servers, server_port=5055)
+        results = list_devices(server_hosts=servers)
 
         for server, devices in results.items():
             typer.echo(f"\n=== {server} ===")
@@ -379,8 +379,9 @@ def main(args: Sequence[str] | None = None) -> None:
     """Argument parser for the CLI."""
     try:
         app()
-    except RuntimeError as e:
-        typer.echo(f"Error: {e}", err=True)
+    except Exception as e:
+        logger.debug("Exception caught in main()", exc_info=True)
+        typer.echo(f"ERROR: {e}", err=True)
 
 
 if __name__ == "__main__":

src/usb_remote/api.py

@@ -6,6 +6,8 @@
 
 from .usbdevice import UsbDevice
 
+PORT = 5055
+
 
 class StrictBaseModel(BaseModel):
     """Base model with strict validation - no extra fields allowed."""
@@ -19,6 +21,11 @@ class ListRequest(StrictBaseModel):
     command: Literal["list"] = "list"
 
 
+find_command = "find"
+attach_command = "attach"
+detach_command = "detach"
+
+
 class DeviceRequest(StrictBaseModel):
     """Request to find/attach/detach a USB device."""
 
@@ -44,6 +51,11 @@ class DeviceResponse(StrictBaseModel):
     data: UsbDevice
 
 
+error_response = "error"
+not_found_response = "not_found"
+multiple_matches_response = "multiple_matches"
+
+
 class ErrorResponse(StrictBaseModel):
     """Error response."""
 

src/usb_remote/client.py

@@ -4,11 +4,15 @@
 from pydantic import TypeAdapter
 
 from .api import (
+    PORT,
     DeviceRequest,
     DeviceResponse,
     ErrorResponse,
     ListRequest,
     ListResponse,
+    attach_command,
+    detach_command,
+    find_command,
 )
 from .config import get_timeout
 from .port import Port
@@ -24,7 +28,7 @@
 def send_request(
     request: ListRequest | DeviceRequest,
     server_host: str = "localhost",
-    server_port: int = 5055,
+    server_port: int = PORT,
     timeout: float | None = None,
 ) -> ListResponse | DeviceResponse:
     """
@@ -89,7 +93,6 @@ def send_request(
 
 def list_devices(
     server_hosts: list[str],
-    server_port: int = 5055,
     timeout: float | None = None,
 ) -> dict[str, list[UsbDevice]]:
     """
@@ -112,7 +115,7 @@ def list_devices(
     for server in server_hosts:
         try:
             request = ListRequest()
-            response = send_request(request, server, server_port, timeout=timeout)
+            response = send_request(request, server, timeout=timeout)
             assert isinstance(response, ListResponse)
             results[server] = response.data
             logger.debug(f"Server {server}: {len(response.data)} devices")
@@ -148,7 +151,6 @@ def attach_device(bus_id: str, server_host: str) -> None:
     Args:
         bus_id: The bus ID of the device to attach
         server_host: Server hostname or IP address
-        server_port: Server port number
         timeout: Connection timeout in seconds. If None, uses configured timeout.
     """
 
@@ -159,7 +161,7 @@ def attach_device(bus_id: str, server_host: str) -> None:
 
     logger.debug(f"Asking remote {server_host} to bind {bus_id} to usbip")
     request = DeviceRequest(
-        command="attach",
+        command=attach_command,
         bus=bus_id,
     )
     send_request(request, server_host)
@@ -186,14 +188,13 @@ def detach_device(bus_id: str, server_host: str) -> None:
     Args:
         bus_id: The bus ID of the device to detach
         server_host: Server hostname or IP address
-        server_port: Server port number
         timeout: Connection timeout in seconds. If None, uses configured timeout.
     """
     detach_local_device(bus_id, server_host)
 
     logger.debug(f"Asking remote {server_host} to unbind {bus_id} from usbip")
     request = DeviceRequest(
-        command="detach",
+        command=detach_command,
         bus=bus_id,
     )
     send_request(request, server_host)
@@ -219,7 +220,6 @@ def find_device(
     Args:
         args: AttachRequest with device search criteria
         server_hosts: list of server hostnames/IPs
-        server_port: Server port number
         timeout: Connection timeout in seconds. If None, uses configured timeout.
 
     Returns:
@@ -235,7 +235,7 @@ def find_device(
     )
 
     request = DeviceRequest(
-        command="find",
+        command=find_command,
         id=id,
         bus=bus,
         desc=desc,
@@ -258,26 +258,25 @@ def find_device(
             continue
         except MultipleDevicesError as e:
             # Multiple matches on this server
-            logger.error(f"Error on Server {server}:\n{e}")
-            exit(1)
-        except RuntimeError as e:
-            # Server returned a generic error
-            logger.error(f"Server {server} error: {e}")
+            raise RuntimeError(f"Server {server}:\n{e}") from e
+        except Exception as e:
+            # Server returned a generic error - continue to next server
+            logger.error(f"Server {server}:\n {e}")
             continue
 
     if len(matches) == 0:
         msg = f"No matching device found across {len(server_hosts)} servers"
-        logger.error(msg)
+        logger.debug(msg, exc_info=True)
         raise DeviceNotFoundError(msg)
 
     if len(matches) > 1 and not request.first:
         device_list = "\n".join(f"  {dev} (on {srv})" for dev, srv in matches)
         msg = (
             f"Multiple devices matched across servers:\n{device_list}\n\n"
-            "Use --first to attach the first match."
+            "Use --first to attach to the first match."
         )
-        logger.error(msg)
-        exit(1)
+        logger.debug(msg, exc_info=True)
+        raise MultipleDevicesError(msg)
 
     device, server = matches[0]
 

src/usb_remote/server.py

@@ -1,15 +1,20 @@
 import logging
 import socket
 import threading
+from typing import Literal
 
 from pydantic import TypeAdapter, ValidationError
 
 from .api import (
+    PORT,
     DeviceRequest,
     DeviceResponse,
     ErrorResponse,
     ListRequest,
     ListResponse,
+    error_response,
+    multiple_matches_response,
+    not_found_response,
 )
 from .usbdevice import (
     DeviceNotFoundError,
@@ -24,7 +29,7 @@
 
 
 class CommandServer:
-    def __init__(self, host: str = "0.0.0.0", port: int = 5055):
+    def __init__(self, host: str = "0.0.0.0", port: int = PORT):
         self.host = host
         self.port = port
         self.server_socket = None
@@ -77,28 +82,36 @@ def _send_response(
         """Send a JSON response to the client."""
         client_socket.sendall(response.model_dump_json().encode("utf-8") + b"\n")
 
+    def _send_error_response(
+        self,
+        client_socket: socket.socket,
+        status: Literal["error", "not_found", "multiple_matches"],
+        message: str,
+    ):
+        """Send an error response to the client."""
+        response = ErrorResponse(status=status, message=message)
+        self._send_response(client_socket, response)
+
     def handle_client(self, client_socket: socket.socket, address):
         """Handle individual client connections."""
 
         try:
             data = client_socket.recv(1024).decode("utf-8")
 
             if not data:
-                response = ErrorResponse(
-                    status="error", message="Empty or invalid command"
+                self._send_error_response(
+                    client_socket, error_response, "Empty or invalid command"
                 )
-                self._send_response(client_socket, response)
                 return
 
             # Try to parse as either ListRequest or AttachRequest
             request_adapter = TypeAdapter(ListRequest | DeviceRequest)
             try:
                 request = request_adapter.validate_json(data)
             except ValidationError as e:
-                response = ErrorResponse(
-                    status="error", message=f"Invalid request format: {str(e)}"
+                self._send_error_response(
+                    client_socket, error_response, f"Invalid request format: {str(e)}"
                 )
-                self._send_response(client_socket, response)
                 return
 
             logger.info(
@@ -117,23 +130,17 @@ def handle_client(self, client_socket: socket.socket, address):
 
         except DeviceNotFoundError as e:
             logger.warning(f"Device not found for client {address}: {e}")
-            response = ErrorResponse(status="not_found", message=str(e))
-            self._send_response(client_socket, response)
+            self._send_error_response(client_socket, not_found_response, str(e))
         except MultipleDevicesError as e:
             logger.warning(f"Multiple devices matched for client {address}: {e}")
-            response = ErrorResponse(status="multiple_matches", message=str(e))
-            self._send_response(client_socket, response)
+            self._send_error_response(client_socket, multiple_matches_response, str(e))
         except Exception as e:
             logger.error(f"Error handling client {address}: {e}")
-            response = ErrorResponse(status="error", message=str(e))
-            self._send_response(client_socket, response)
+            self._send_error_response(client_socket, error_response, str(e))
 
         finally:
             client_socket.close()
 
-    def _respond_to_client(self, client_socket, response):
-        self._send_response(client_socket, response)
-
     def start(self):
         """Start the server."""
         logger.debug(f"Starting server on {self.host}:{self.port}")

tests/test_cli.py

@@ -91,9 +91,7 @@ def test_list_with_host(self, mock_usb_devices):
         ) as mock_list:
             result = runner.invoke(app, ["list", "--host", "192.168.1.100"])
             assert result.exit_code == 0
-            mock_list.assert_called_once_with(
-                server_hosts=["192.168.1.100"], server_port=5055
-            )
+            mock_list.assert_called_once_with(server_hosts=["192.168.1.100"])
 
     def test_list_error_handling(self):
         """Test list command error handling."""

tests/test_protocol.py

@@ -34,7 +34,6 @@
     ListRequest,
     ListResponse,
 )
-from usb_remote.client import list_devices
 from usb_remote.server import CommandServer
 from usb_remote.usbdevice import UsbDevice
 
@@ -244,17 +243,6 @@ def test_error_response_deserialization(self):
 class TestClientServerIntegration:
     """Integration tests for client-server communication."""
 
-    def test_list_devices_integration(self, server, server_port, mock_usb_devices):
-        """Test full list devices flow from client to server."""
-        result = list_devices(server_hosts=["127.0.0.1"], server_port=server_port)
-
-        assert "127.0.0.1" in result
-        devices = result["127.0.0.1"]
-        assert len(devices) == 2
-        assert devices[0].bus_id == "1-1.1"
-        assert devices[0].vendor_id == "1234"
-        assert devices[1].bus_id == "2-2.1"
-
     def test_find_device_integration(self, server, server_port, mock_usb_devices):
         """Test full find device flow from client to server."""
         from usb_remote.client import find_device
@@ -324,17 +312,6 @@ def test_server_handles_unknown_command(self, server, server_port):
 
             assert parsed["status"] == "error"
 
-    def test_client_handles_error_response(self, server, server_port, mock_get_devices):
-        """Test that client properly handles error responses."""
-        # Override the mock to raise an exception
-        with patch(
-            "usb_remote.server.get_devices", side_effect=Exception("Test error")
-        ):
-            result = list_devices(server_hosts=["127.0.0.1"], server_port=server_port)
-            # In multi-server mode, errors are caught and the server returns empty list
-            assert "127.0.0.1" in result
-            assert result["127.0.0.1"] == []
-
 
 class TestProtocolRobustness:
     """Test protocol robustness and edge cases."""