Skip to content

Commit 907c176

Browse files
kingpanther13claude
kingpanther13
and
claude
authored
Issue 518 part5 flip default (final part) (#571)
* 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 * test: Update 8 E2E test files to handle ToolError via safe_call_tool (#518) Part 2 of the isError signaling fix. Updates all remaining E2E test files that expect error responses to use safe_call_tool, which handles both legacy dict returns and new ToolError exceptions. Updated test files: - test_config_entry_flow: 2 error-expecting tests - test_helper_crud: 3 error tests + wait_for_entity_registration helper - test_entity_management: 2 error-expecting tests - test_file_operations: 2 helper functions using parse_mcp_result - test_hacs: 3 error-expecting tests - test_integration_management: 1 error-expecting test - test_device_registry: 4 error-expecting tests - test_voice_assistant: 2 error-expecting tests Total: ~28 error-expecting assertions across 8 files Depends on PR 1 which provides safe_call_tool infrastructure. https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8 * fix: Signal tool errors via isError for remaining 9 tools (#518) Migrate remaining tool modules to use ToolError/raise_tool_error for MCP protocol-level error signaling. Includes updated unit tests for voice assistant and entity tools. https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8 * feat: Flip exception_to_structured_error default to raise_error=True (#518) Now that all tools are migrated to use ToolError, flip the default behavior of exception_to_structured_error to raise by default. This completes the ToolError migration - all error paths now signal errors at the MCP protocol level with isError=true. https://claude.ai/code/session_01MvDDV6qmWosBfGhmTLYzo8 * fix: remove duplicate ToolError imports in tools_areas and test_tools_entities Merge artifact from upstream integration — duplicate `from fastmcp.exceptions import ToolError` lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c835619 commit 907c176

12 files changed

Lines changed: 128 additions & 134 deletions

‎src/ha_mcp/tools/helpers.py‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,46 +88,48 @@ def exception_to_structured_error(
8888
error: Exception,
8989
context: dict[str, Any] | None = None,
9090
*,
91-
raise_error: Literal[False] = False,
92-
) -> dict[str, Any]: ...
91+
raise_error: Literal[True] = True,
92+
suggestions: list[str] | None = None,
93+
) -> NoReturn: ...
9394

9495

9596
@overload
9697
def exception_to_structured_error(
9798
error: Exception,
9899
context: dict[str, Any] | None = None,
99100
*,
100-
raise_error: Literal[True],
101-
) -> NoReturn: ...
101+
raise_error: Literal[False],
102+
suggestions: list[str] | None = None,
103+
) -> dict[str, Any]: ...
102104

103105

104106
def exception_to_structured_error(
105107
error: Exception,
106108
context: dict[str, Any] | None = None,
107109
*,
108-
raise_error: bool = False,
110+
raise_error: bool = True,
111+
suggestions: list[str] | None = None,
109112
) -> dict[str, Any]:
110113
"""
111114
Convert an exception to a structured error response.
112115
113116
This function maps common exception types to appropriate error codes
114-
and creates informative error responses.
117+
and creates informative error responses. By default, it raises a ToolError
118+
to signal the error at the MCP protocol level (isError=true).
115119
116120
Args:
117121
error: The exception to convert
118122
context: Additional context to include in the response
119-
raise_error: If True, raises ToolError with the structured error.
120-
If False (default), returns the error dict.
121-
122-
NOTE: The default will change to True in a future PR once
123-
all tools are updated to use ToolError. New code should
124-
explicitly pass raise_error=True for forward compatibility.
123+
raise_error: If True (default), raises ToolError with the structured error.
124+
If False, returns the error dict for further modification.
125+
suggestions: Optional list of actionable suggestions to embed in the error.
126+
Saves callers from manually inserting suggestions after the call.
125127
126128
Returns:
127129
Structured error response dictionary (only if raise_error=False)
128130
129131
Raises:
130-
ToolError: If raise_error=True, raises with JSON-serialized error
132+
ToolError: If raise_error=True (default), raises with JSON-serialized error
131133
"""
132134
error_str = str(error).lower()
133135
error_msg = str(error)
@@ -211,6 +213,9 @@ def exception_to_structured_error(
211213
context=context,
212214
)
213215

216+
if suggestions and "error" in error_response and isinstance(error_response["error"], dict):
217+
error_response["error"]["suggestions"] = suggestions
218+
214219
if raise_error:
215220
raise_tool_error(error_response)
216221

‎src/ha_mcp/tools/tools_config_automations.py‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -258,19 +258,15 @@ async def ha_config_get_automation(
258258
raise_tool_error(error_response)
259259

260260
logger.error(f"Error getting automation: {e}")
261-
error_response = exception_to_structured_error(
261+
exception_to_structured_error(
262262
e,
263263
context={"identifier": identifier, "action": "get"},
264-
raise_error=False,
265-
)
266-
# Add automation-specific suggestions
267-
if "error" in error_response and isinstance(error_response["error"], dict):
268-
error_response["error"]["suggestions"] = [
264+
suggestions=[
269265
"Verify automation exists using ha_search_entities(domain_filter='automation')",
270266
"Check Home Assistant connection",
271267
"Use ha_get_domain_docs('automation') for configuration help",
272-
]
273-
raise_tool_error(error_response)
268+
],
269+
)
274270

275271
@mcp.tool(
276272
annotations={
@@ -483,21 +479,17 @@ async def ha_config_set_automation(
483479
raise
484480
except Exception as e:
485481
logger.error(f"Error upserting automation: {e}")
486-
error_response = exception_to_structured_error(
482+
exception_to_structured_error(
487483
e,
488484
context={"identifier": identifier},
489-
raise_error=False,
490-
)
491-
# Add automation-specific suggestions
492-
if "error" in error_response and isinstance(error_response["error"], dict):
493-
error_response["error"]["suggestions"] = [
485+
suggestions=[
494486
"Check automation configuration format",
495487
"Ensure required fields: alias, trigger, action",
496488
"Use entity_id format: automation.morning_routine or unique_id",
497489
"Use ha_search_entities(domain_filter='automation') to find automations",
498490
"Use ha_get_domain_docs('automation') for comprehensive configuration help",
499-
]
500-
raise_tool_error(error_response)
491+
],
492+
)
501493

502494
@mcp.tool(
503495
annotations={

‎src/ha_mcp/tools/tools_config_dashboards.py‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1688,7 +1688,7 @@ async def ha_dashboard_find_card(
16881688
f"error={e}",
16891689
exc_info=True,
16901690
)
1691-
error_response = exception_to_structured_error(
1691+
return exception_to_structured_error(
16921692
e,
16931693
context={
16941694
"action": "find_card",
@@ -1697,15 +1697,9 @@ async def ha_dashboard_find_card(
16971697
"card_type": card_type,
16981698
"heading": heading,
16991699
},
1700-
)
1701-
if "error" in error_response and isinstance(error_response["error"], dict):
1702-
error_response["error"]["suggestions"] = [
1700+
raise_error=False,
1701+
suggestions=[
17031702
"Check HA connection",
17041703
"Verify dashboard with ha_config_get_dashboard(list_only=True)",
1705-
]
1706-
else:
1707-
logger.warning(
1708-
f"Unexpected error response structure, could not add suggestions: "
1709-
f"{type(error_response.get('error'))}"
1710-
)
1711-
return error_response
1704+
],
1705+
)

‎src/ha_mcp/tools/tools_config_entry_flow.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,7 @@ async def ha_create_config_entry_helper(
157157
context = {"helper_type": helper_type}
158158
if flow_id:
159159
context["flow_id"] = flow_id
160-
exception_to_structured_error(e, context=context, raise_error=True)
160+
exception_to_structured_error(e, context=context)
161161

162162
@mcp.tool(
163163
annotations={
@@ -220,4 +220,4 @@ async def ha_get_helper_schema(
220220

221221
except Exception as e:
222222
logger.error(f"Error getting helper schema: {e}")
223-
exception_to_structured_error(e, context={"helper_type": helper_type}, raise_error=True)
223+
exception_to_structured_error(e, context={"helper_type": helper_type})

‎src/ha_mcp/tools/tools_entities.py‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -585,7 +585,7 @@ async def ha_set_entity(
585585
except Exception as e:
586586
logger.error(f"Error updating entity: {e}")
587587
eid_context = entity_id if isinstance(entity_id, str) else entity_ids
588-
exception_to_structured_error(e, context={"entity_id": eid_context}, raise_error=True)
588+
exception_to_structured_error(e, context={"entity_id": eid_context})
589589

590590
@mcp.tool(
591591
annotations={
@@ -772,6 +772,5 @@ async def _fetch_entity(eid: str) -> dict[str, Any]:
772772
except Exception as e:
773773
logger.error(f"Error getting entity: {e}")
774774
exception_to_structured_error(
775-
e, context={"entity_id": entity_id if isinstance(entity_id, str) else entity_ids},
776-
raise_error=True,
775+
e, context={"entity_id": entity_id if isinstance(entity_id, str) else entity_ids}
777776
)

‎src/ha_mcp/tools/tools_hacs.py‎

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -124,13 +124,12 @@ async def ha_hacs_info() -> dict[str, Any]:
124124
e,
125125
context={"tool": "ha_hacs_info"},
126126
raise_error=False,
127-
)
128-
if "error" in error_response and isinstance(error_response["error"], dict):
129-
error_response["error"]["suggestions"] = [
127+
suggestions=[
130128
"Verify HACS is installed: https://hacs.xyz/",
131129
"Check Home Assistant connection",
132130
"Restart Home Assistant if HACS was recently installed",
133-
]
131+
],
132+
)
134133
error_with_tz = await add_timezone_metadata(client, error_response)
135134
raise_tool_error(error_with_tz)
136135

@@ -244,13 +243,12 @@ async def ha_hacs_list_installed(
244243
e,
245244
context={"tool": "ha_hacs_list_installed", "category": category},
246245
raise_error=False,
247-
)
248-
if "error" in error_response and isinstance(error_response["error"], dict):
249-
error_response["error"]["suggestions"] = [
246+
suggestions=[
250247
"Verify HACS is installed: https://hacs.xyz/",
251248
"Check category name is valid: integration, lovelace, theme, appdaemon, python_script",
252249
"Check Home Assistant connection",
253-
]
250+
],
251+
)
254252
error_with_tz = await add_timezone_metadata(client, error_response)
255253
raise_tool_error(error_with_tz)
256254

@@ -425,13 +423,12 @@ async def ha_hacs_search(
425423
e,
426424
context={"tool": "ha_hacs_search", "query": query, "category": category},
427425
raise_error=False,
428-
)
429-
if "error" in error_response and isinstance(error_response["error"], dict):
430-
error_response["error"]["suggestions"] = [
426+
suggestions=[
431427
"Verify HACS is installed: https://hacs.xyz/",
432428
"Try a simpler search query",
433429
"Check category name is valid: integration, lovelace, theme, appdaemon, python_script",
434-
]
430+
],
431+
)
435432
error_with_tz = await add_timezone_metadata(client, error_response)
436433
raise_tool_error(error_with_tz)
437434

@@ -543,13 +540,12 @@ async def ha_hacs_repository_info(repository_id: str) -> dict[str, Any]:
543540
e,
544541
context={"tool": "ha_hacs_repository_info", "repository_id": repository_id},
545542
raise_error=False,
546-
)
547-
if "error" in error_response and isinstance(error_response["error"], dict):
548-
error_response["error"]["suggestions"] = [
543+
suggestions=[
549544
"Verify HACS is installed: https://hacs.xyz/",
550545
"Check repository ID format (e.g., 'hacs/integration' or 'owner/repo')",
551546
"Use ha_hacs_search() to find the correct repository ID",
552-
]
547+
],
548+
)
553549
error_with_tz = await add_timezone_metadata(client, error_response)
554550
raise_tool_error(error_with_tz)
555551

@@ -668,15 +664,14 @@ async def ha_hacs_add_repository(
668664
"category": category,
669665
},
670666
raise_error=False,
671-
)
672-
if "error" in error_response and isinstance(error_response["error"], dict):
673-
error_response["error"]["suggestions"] = [
667+
suggestions=[
674668
"Verify HACS is installed: https://hacs.xyz/",
675669
"Check repository format: 'owner/repo'",
676670
"Verify the repository exists on GitHub",
677671
"Ensure category matches repository type",
678672
"Check repository follows HACS guidelines: https://hacs.xyz/docs/publish/start",
679-
]
673+
],
674+
)
680675
error_with_tz = await add_timezone_metadata(client, error_response)
681676
raise_tool_error(error_with_tz)
682677

@@ -806,13 +801,12 @@ async def ha_hacs_download(
806801
"version": version,
807802
},
808803
raise_error=False,
809-
)
810-
if "error" in error_response and isinstance(error_response["error"], dict):
811-
error_response["error"]["suggestions"] = [
804+
suggestions=[
812805
"Verify HACS is installed: https://hacs.xyz/",
813806
"Check repository ID is valid (use ha_hacs_search() to find it)",
814807
"Ensure the repository is in HACS (use ha_hacs_add_repository() if needed)",
815808
"Check version format (e.g., 'v1.2.3' or '1.2.3')",
816-
]
809+
],
810+
)
817811
error_with_tz = await add_timezone_metadata(client, error_response)
818812
raise_tool_error(error_with_tz)

‎src/ha_mcp/tools/tools_integrations.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ async def ha_set_integration_enabled(
269269

270270
except Exception as e:
271271
logger.error(f"Failed to set integration enabled: {e}")
272-
exception_to_structured_error(e, context={"entry_id": entry_id}, raise_error=True)
272+
exception_to_structured_error(e, context={"entry_id": entry_id})
273273

274274
@mcp.tool(
275275
annotations={
@@ -334,4 +334,4 @@ async def ha_delete_config_entry(
334334

335335
except Exception as e:
336336
logger.error(f"Failed to delete config entry: {e}")
337-
exception_to_structured_error(e, context={"entry_id": entry_id}, raise_error=True)
337+
exception_to_structured_error(e, context={"entry_id": entry_id})

‎src/ha_mcp/tools/tools_mcp_component.py‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
from pydantic import Field
1616

17-
from .helpers import exception_to_structured_error, log_tool_usage, raise_tool_error
17+
from .helpers import exception_to_structured_error, log_tool_usage
1818
from .util_helpers import add_timezone_metadata
1919

2020
logger = logging.getLogger(__name__)
@@ -296,17 +296,12 @@ async def ha_install_mcp_tools(
296296
return await add_timezone_metadata(client, result)
297297

298298
except Exception as e:
299-
error_response = exception_to_structured_error(
299+
exception_to_structured_error(
300300
e,
301301
context={"tool": "ha_install_mcp_tools", "restart": restart},
302-
raise_error=False,
303-
)
304-
if "error" in error_response and isinstance(error_response["error"], dict):
305-
suggestions = [
302+
suggestions=[
306303
"Verify HACS is installed: https://hacs.xyz/",
307304
"Check Home Assistant logs for errors",
308305
"Ensure GitHub is accessible",
309-
]
310-
error_response["error"]["suggestions"] = suggestions
311-
error_response["error"]["suggestion"] = suggestions[0]
312-
raise_tool_error(error_response)
306+
],
307+
)

0 commit comments

Comments
 (0)