Issue #518 part1 (#551)

* fix: Signal tool errors via isError for automation, service, and registry tools (#518)

Part 1 of the isError signaling fix. Adds core infrastructure and applies
to tools that already have comprehensive E2E test coverage.

Core changes:
- Add raise_tool_error() helper to convert error dicts to ToolError exceptions
- Update exception_to_structured_error() to raise by default (raise_error=True)
- Add @overload type signatures for raise_error parameter
- Add safe_call_tool() and tool_error_to_result() test utilities

Tool changes (3 tools with existing E2E tests):
- tools_config_automations: 5 error paths now raise ToolError
- tools_service: 4 error paths now raise ToolError
- tools_registry: 1 error path now raises ToolError

Test updates:
- Update 11 E2E test files to use safe_call_tool for error path testing
- Add unit tests for raise_tool_error and exception_to_structured_error
- Add retry logic for entity rename tests (timing robustness)

Closes part of #518

https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8

* refactor: use match statement and keep raise_error=False as default

Per code review feedback:
1. Use match statement for HTTP status code dispatch (Python 3.13)
2. Keep raise_error=False as default to avoid regression during PR stack
   - Tools explicitly pass raise_error=True or use raise_error=False + raise_tool_error()
   - Part 5 will flip the default after all tools are migrated
3. Update unit tests to reflect new default behavior

https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8

* fix: Address code review feedback on PR #551

Fixes for HIGH priority issues:
- H1: Add `except ToolError: raise` before `except Exception` handlers in
  tools_config_automations.py and tools_service.py to prevent double-wrapping
- H2: Change return type annotation to `dict[str, Any]` (NoReturn in union is meaningless)
- H3: Add case 403 to match statement for auth/permission errors

Fixes for MEDIUM priority issues:
- M2: Add `default=str` to json.dumps in raise_tool_error for non-serializable fallback
- M3: Change pytest.skip to pytest.xfail in test_lights.py to not mask regressions

https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: kingpanther13

src/ha_mcp/tools/helpers.py

@@ -5,9 +5,12 @@
 """
 
 import functools
+import json
 import logging
 import time
-from typing import Any
+from typing import Any, Literal, NoReturn, overload
+
+from fastmcp.exceptions import ToolError
 
 from ..client.rest_client import (
     HomeAssistantAPIError,
@@ -29,6 +32,34 @@
 logger = logging.getLogger(__name__)
 
 
+def raise_tool_error(error_response: dict[str, Any]) -> NoReturn:
+    """
+    Raise a ToolError with structured error information.
+
+    This function converts a structured error response dictionary into a ToolError
+    exception, which signals to MCP clients that the tool execution failed via
+    the isError flag in the protocol response.
+
+    The structured error information is preserved as JSON in the error message,
+    allowing AI agents to parse and act on the detailed error information.
+
+    Args:
+        error_response: Structured error response dictionary with 'success': False
+                       and 'error' containing code, message, suggestions, etc.
+
+    Raises:
+        ToolError: Always raises with the JSON-serialized error response
+
+    Example:
+        >>> error = create_error_response(
+        ...     ErrorCode.ENTITY_NOT_FOUND,
+        ...     "Entity light.nonexistent not found"
+        ... )
+        >>> raise_tool_error(error)  # Raises ToolError with isError=true
+    """
+    raise ToolError(json.dumps(error_response, indent=2, default=str))
+
+
 async def get_connected_ws_client(
     base_url: str, token: str
 ) -> tuple[HomeAssistantWebSocketClient | None, dict[str, Any] | None]:
@@ -52,9 +83,29 @@ async def get_connected_ws_client(
     return ws_client, None
 
 
+@overload
+def exception_to_structured_error(
+    error: Exception,
+    context: dict[str, Any] | None = None,
+    *,
+    raise_error: Literal[False] = False,
+) -> dict[str, Any]: ...
+
+
+@overload
+def exception_to_structured_error(
+    error: Exception,
+    context: dict[str, Any] | None = None,
+    *,
+    raise_error: Literal[True],
+) -> NoReturn: ...
+
+
 def exception_to_structured_error(
     error: Exception,
     context: dict[str, Any] | None = None,
+    *,
+    raise_error: bool = False,
 ) -> dict[str, Any]:
     """
     Convert an exception to a structured error response.
@@ -65,83 +116,105 @@ def exception_to_structured_error(
     Args:
         error: The exception to convert
         context: Additional context to include in the response
+        raise_error: If True, raises ToolError with the structured error.
+                    If False (default), returns the error dict.
+
+                    NOTE: The default will change to True in a future PR once
+                    all tools are updated to use ToolError. New code should
+                    explicitly pass raise_error=True for forward compatibility.
 
     Returns:
-        Structured error response dictionary
+        Structured error response dictionary (only if raise_error=False)
+
+    Raises:
+        ToolError: If raise_error=True, raises with JSON-serialized error
     """
     error_str = str(error).lower()
     error_msg = str(error)
 
+    error_response: dict[str, Any]
+
     # Handle specific exception types
     if isinstance(error, HomeAssistantConnectionError):
         if "timeout" in error_str:
-            return create_connection_error(error_msg, timeout=True)
-        return create_connection_error(error_msg)
+            error_response = create_connection_error(error_msg, timeout=True)
+        else:
+            error_response = create_connection_error(error_msg)
 
-    if isinstance(error, HomeAssistantAuthError):
+    elif isinstance(error, HomeAssistantAuthError):
         if "expired" in error_str:
-            return create_auth_error(error_msg, expired=True)
-        return create_auth_error(error_msg)
+            error_response = create_auth_error(error_msg, expired=True)
+        else:
+            error_response = create_auth_error(error_msg)
 
-    if isinstance(error, HomeAssistantAPIError):
+    elif isinstance(error, HomeAssistantAPIError):
         # Check for specific error patterns
-        if error.status_code == 404:
-            # Entity or resource not found
-            entity_id = context.get("entity_id") if context else None
-            if entity_id:
-                return create_entity_not_found_error(entity_id, details=error_msg)
-            return create_error_response(
-                ErrorCode.RESOURCE_NOT_FOUND,
-                error_msg,
-                context=context,
-            )
-        if error.status_code == 401:
-            return create_auth_error(error_msg)
-        if error.status_code == 400:
-            return create_validation_error(error_msg, context=context)
-
-        # Generic API error
-        return create_error_response(
-            ErrorCode.SERVICE_CALL_FAILED,
-            error_msg,
-            context=context,
-        )
+        match error.status_code:
+            case 404:
+                # Entity or resource not found
+                entity_id = context.get("entity_id") if context else None
+                if entity_id:
+                    error_response = create_entity_not_found_error(entity_id, details=error_msg)
+                else:
+                    error_response = create_error_response(
+                        ErrorCode.RESOURCE_NOT_FOUND,
+                        error_msg,
+                        context=context,
+                    )
+            case 401 | 403:
+                error_response = create_auth_error(error_msg)
+            case 400:
+                error_response = create_validation_error(error_msg, context=context)
+            case _:
+                # Generic API error
+                error_response = create_error_response(
+                    ErrorCode.SERVICE_CALL_FAILED,
+                    error_msg,
+                    context=context,
+                )
 
-    if isinstance(error, TimeoutError):
+    elif isinstance(error, TimeoutError):
         operation = context.get("operation", "request") if context else "request"
         timeout_seconds = context.get("timeout_seconds", 30) if context else 30
-        return create_timeout_error(operation, timeout_seconds, details=error_msg)
+        error_response = create_timeout_error(operation, timeout_seconds, details=error_msg)
 
-    if isinstance(error, ValueError):
-        return create_validation_error(error_msg)
+    elif isinstance(error, ValueError):
+        error_response = create_validation_error(error_msg)
 
     # Check for common error patterns in error message
-    if "not found" in error_str or "404" in error_str:
+    elif "not found" in error_str or "404" in error_str:
         entity_id = context.get("entity_id") if context else None
         if entity_id:
-            return create_entity_not_found_error(entity_id, details=error_msg)
-        return create_error_response(
-            ErrorCode.RESOURCE_NOT_FOUND,
+            error_response = create_entity_not_found_error(entity_id, details=error_msg)
+        else:
+            error_response = create_error_response(
+                ErrorCode.RESOURCE_NOT_FOUND,
+                error_msg,
+                context=context,
+            )
+
+    elif "timeout" in error_str:
+        error_response = create_timeout_error("operation", 30, details=error_msg)
+
+    elif "connection" in error_str or "connect" in error_str:
+        error_response = create_connection_error(error_msg)
+
+    elif "auth" in error_str or "token" in error_str or "401" in error_str:
+        error_response = create_auth_error(error_msg)
+
+    else:
+        # Default to internal error
+        error_response = create_error_response(
+            ErrorCode.INTERNAL_ERROR,
             error_msg,
+            details="An unexpected error occurred",
             context=context,
         )
 
-    if "timeout" in error_str:
-        return create_timeout_error("operation", 30, details=error_msg)
-
-    if "connection" in error_str or "connect" in error_str:
-        return create_connection_error(error_msg)
-
-    if "auth" in error_str or "token" in error_str or "401" in error_str:
-        return create_auth_error(error_msg)
+    if raise_error:
+        raise_tool_error(error_response)
 
-    # Default to internal error
-    return create_error_response(
-        ErrorCode.INTERNAL_ERROR,
-        error_msg,
-        details="An unexpected error occurred",
-        context=context,
-    )
+    return error_response
 
 
 def log_tool_usage(func: Any) -> Any:

src/ha_mcp/tools/tools_config_automations.py

@@ -8,14 +8,15 @@
 import logging
 from typing import Annotated, Any, cast
 
+from fastmcp.exceptions import ToolError
 from pydantic import Field
 
 from ..errors import (
     create_config_error,
     create_resource_not_found_error,
     create_validation_error,
 )
-from .helpers import exception_to_structured_error, log_tool_usage
+from .helpers import exception_to_structured_error, log_tool_usage, raise_tool_error
 from .util_helpers import parse_json_param
 
 logger = logging.getLogger(__name__)
@@ -249,12 +250,13 @@ async def ha_config_get_automation(
                 )
                 error_response["action"] = "get"
                 error_response["reason"] = "not_found"
-                return error_response
+                raise_tool_error(error_response)
 
             logger.error(f"Error getting automation: {e}")
             error_response = exception_to_structured_error(
                 e,
                 context={"identifier": identifier, "action": "get"},
+                raise_error=False,
             )
             # Add automation-specific suggestions
             if "error" in error_response and isinstance(error_response["error"], dict):
@@ -263,7 +265,7 @@ async def ha_config_get_automation(
                     "Check Home Assistant connection",
                     "Use ha_get_domain_docs('automation') for configuration help",
                 ]
-            return error_response
+            raise_tool_error(error_response)
 
     @mcp.tool(
         annotations={
@@ -411,19 +413,19 @@ async def ha_config_set_automation(
             try:
                 parsed_config = parse_json_param(config, "config")
             except ValueError as e:
-                return create_validation_error(
+                raise_tool_error(create_validation_error(
                     f"Invalid config parameter: {e}",
                     parameter="config",
                     invalid_json=True,
-                )
+                ))
 
             # Ensure config is a dict
             if parsed_config is None or not isinstance(parsed_config, dict):
-                return create_validation_error(
+                raise_tool_error(create_validation_error(
                     "Config parameter must be a JSON object",
                     parameter="config",
                     details=f"Received type: {type(parsed_config).__name__}",
-                )
+                ))
 
             config_dict = cast(dict[str, Any], parsed_config)
 
@@ -441,11 +443,11 @@ async def ha_config_set_automation(
 
             missing_fields = [f for f in required_fields if f not in config_dict]
             if missing_fields:
-                return create_config_error(
+                raise_tool_error(create_config_error(
                     f"Missing required fields: {', '.join(missing_fields)}",
                     identifier=identifier,
                     missing_fields=missing_fields,
-                )
+                ))
 
             result = await client.upsert_automation_config(config_dict, identifier)
             return {
@@ -454,11 +456,14 @@ async def ha_config_set_automation(
                 "config_provided": config_dict,
             }
 
+        except ToolError:
+            raise
         except Exception as e:
             logger.error(f"Error upserting automation: {e}")
             error_response = exception_to_structured_error(
                 e,
                 context={"identifier": identifier},
+                raise_error=False,
             )
             # Add automation-specific suggestions
             if "error" in error_response and isinstance(error_response["error"], dict):
@@ -469,7 +474,7 @@ async def ha_config_set_automation(
                     "Use ha_search_entities(domain_filter='automation') to find automations",
                     "Use ha_get_domain_docs('automation') for comprehensive configuration help",
                 ]
-            return error_response
+            raise_tool_error(error_response)
 
     @mcp.tool(
         annotations={
@@ -513,6 +518,7 @@ async def ha_config_remove_automation(
                 error_response = exception_to_structured_error(
                     e,
                     context={"identifier": identifier},
+                    raise_error=False,
                 )
             error_response["action"] = "delete"
             # Add automation-specific suggestions
@@ -522,4 +528,4 @@ async def ha_config_remove_automation(
                     "Use entity_id format: automation.morning_routine or unique_id",
                     "Check Home Assistant connection",
                 ]
-            return error_response
+            raise_tool_error(error_response)

src/ha_mcp/tools/tools_registry.py

@@ -16,7 +16,7 @@
 from pydantic import Field
 
 from ..errors import ErrorCode, create_error_response
-from .helpers import log_tool_usage
+from .helpers import log_tool_usage, raise_tool_error
 from .util_helpers import coerce_bool_param, parse_string_list_param
 
 # Known voice assistant identifiers
@@ -785,10 +785,10 @@ async def ha_update_device(
             try:
                 parsed_labels = parse_string_list_param(labels, "labels")
             except ValueError as e:
-                return create_error_response(
+                raise_tool_error(create_error_response(
                     ErrorCode.VALIDATION_INVALID_PARAMETER,
                     f"Invalid labels parameter: {e}",
-                )
+                ))
 
         # Delegate to internal implementation
         return await _update_device_internal(