From 4bb2a3ad92823e346ab7d201bd6da9d86db790e8 Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Mon, 8 Jan 2024 12:23:06 -0600 Subject: [PATCH] Specific Assist errors for domain/device class (#107302) * Specific errors for domain/device class * Don't log exception * Check device class first * Refactor guard clauses * Test default error --- .../components/conversation/default_agent.py | 49 +++++++++++++++ homeassistant/helpers/intent.py | 31 ++++++++-- .../conversation/test_default_agent.py | 59 ++++++++++++++++++- 3 files changed, 134 insertions(+), 5 deletions(-) diff --git a/homeassistant/components/conversation/default_agent.py b/homeassistant/components/conversation/default_agent.py index 6af75b2f0a8..3f36e98f85a 100644 --- a/homeassistant/components/conversation/default_agent.py +++ b/homeassistant/components/conversation/default_agent.py @@ -269,7 +269,22 @@ class DefaultAgent(AbstractConversationAgent): language, assistant=DOMAIN, ) + except intent.NoStatesMatchedError as no_states_error: + # Intent was valid, but no entities matched the constraints. + error_response_type, error_response_args = _get_no_states_matched_response( + no_states_error + ) + return _make_error_result( + language, + intent.IntentResponseErrorCode.NO_VALID_TARGETS, + self._get_error_text( + error_response_type, lang_intents, **error_response_args + ), + conversation_id, + ) except intent.IntentHandleError: + # Intent was valid and entities matched constraints, but an error + # occurred during handling. _LOGGER.exception("Intent handling error") return _make_error_result( language, @@ -863,6 +878,40 @@ def _get_unmatched_response( return error_response_type, error_response_args +def _get_no_states_matched_response( + no_states_error: intent.NoStatesMatchedError, +) -> tuple[ResponseType, dict[str, Any]]: + """Return error response type and template arguments for error.""" + if not ( + no_states_error.area + and (no_states_error.device_classes or no_states_error.domains) + ): + # Device class and domain must be paired with an area for the error + # message. + return ResponseType.NO_INTENT, {} + + error_response_args: dict[str, Any] = {"area": no_states_error.area} + + # Check device classes first, since it's more specific than domain + if no_states_error.device_classes: + # No exposed entities of a particular class in an area. + # Example: "close the bedroom windows" + # + # Only use the first device class for the error message + error_response_args["device_class"] = next(iter(no_states_error.device_classes)) + + return ResponseType.NO_DEVICE_CLASS, error_response_args + + # No exposed entities of a domain in an area. + # Example: "turn on lights in kitchen" + assert no_states_error.domains + # + # Only use the first domain for the error message + error_response_args["domain"] = next(iter(no_states_error.domains)) + + return ResponseType.NO_DOMAIN, error_response_args + + def _collect_list_references(expression: Expression, list_names: set[str]) -> None: """Collect list reference names recursively.""" if isinstance(expression, Sequence): diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index ee326558467..26468f1fdb7 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -109,8 +109,8 @@ async def async_handle( except vol.Invalid as err: _LOGGER.warning("Received invalid slot info for %s: %s", intent_type, err) raise InvalidSlotInfo(f"Received invalid slot info for {intent_type}") from err - except IntentHandleError: - raise + except IntentError: + raise # bubble up intent related errors except Exception as err: raise IntentUnexpectedError(f"Error handling {intent_type}") from err @@ -135,6 +135,25 @@ class IntentUnexpectedError(IntentError): """Unexpected error while handling intent.""" +class NoStatesMatchedError(IntentError): + """Error when no states match the intent's constraints.""" + + def __init__( + self, + name: str | None, + area: str | None, + domains: set[str] | None, + device_classes: set[str] | None, + ) -> None: + """Initialize error.""" + super().__init__() + + self.name = name + self.area = area + self.domains = domains + self.device_classes = device_classes + + def _is_device_class( state: State, entity: entity_registry.RegistryEntry | None, @@ -421,8 +440,12 @@ class ServiceIntentHandler(IntentHandler): ) if not states: - raise IntentHandleError( - f"No entities matched for: name={name}, area={area}, domains={domains}, device_classes={device_classes}", + # No states matched constraints + raise NoStatesMatchedError( + name=name, + area=area_name, + domains=domains, + device_classes=device_classes, ) response = await self.async_handle_states(intent_obj, states, area) diff --git a/tests/components/conversation/test_default_agent.py b/tests/components/conversation/test_default_agent.py index ac126aa7c6b..2e815edf1e1 100644 --- a/tests/components/conversation/test_default_agent.py +++ b/tests/components/conversation/test_default_agent.py @@ -129,7 +129,7 @@ async def test_exposed_areas( # This should be an error because the lights in that area are not exposed assert result.response.response_type == intent.IntentResponseType.ERROR - assert result.response.error_code == intent.IntentResponseErrorCode.FAILED_TO_HANDLE + assert result.response.error_code == intent.IntentResponseErrorCode.NO_VALID_TARGETS # But we can still ask questions about the bedroom, even with no exposed entities result = await conversation.async_converse( @@ -455,6 +455,38 @@ async def test_error_missing_area(hass: HomeAssistant, init_components) -> None: assert result.response.speech["plain"]["speech"] == "No area named missing area" +async def test_error_no_exposed_for_domain( + hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry +) -> None: + """Test error message when no entities for a domain are exposed in an area.""" + area_registry.async_get_or_create("kitchen") + result = await conversation.async_converse( + hass, "turn on the lights in the kitchen", None, Context(), None + ) + + assert result.response.response_type == intent.IntentResponseType.ERROR + assert result.response.error_code == intent.IntentResponseErrorCode.NO_VALID_TARGETS + assert ( + result.response.speech["plain"]["speech"] == "kitchen does not contain a light" + ) + + +async def test_error_no_exposed_for_device_class( + hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry +) -> None: + """Test error message when no entities of a device class are exposed in an area.""" + area_registry.async_get_or_create("bedroom") + result = await conversation.async_converse( + hass, "open bedroom windows", None, Context(), None + ) + + assert result.response.response_type == intent.IntentResponseType.ERROR + assert result.response.error_code == intent.IntentResponseErrorCode.NO_VALID_TARGETS + assert ( + result.response.speech["plain"]["speech"] == "bedroom does not contain a window" + ) + + async def test_error_match_failure(hass: HomeAssistant, init_components) -> None: """Test response with complete match failure.""" with patch( @@ -471,6 +503,31 @@ async def test_error_match_failure(hass: HomeAssistant, init_components) -> None ) +async def test_no_states_matched_default_error( + hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry +) -> None: + """Test default response when no states match and slots are missing.""" + area_registry.async_get_or_create("kitchen") + + with patch( + "homeassistant.components.conversation.default_agent.intent.async_handle", + side_effect=intent.NoStatesMatchedError(None, None, None, None), + ): + result = await conversation.async_converse( + hass, "turn on lights in the kitchen", None, Context(), None + ) + + assert result.response.response_type == intent.IntentResponseType.ERROR + assert ( + result.response.error_code + == intent.IntentResponseErrorCode.NO_VALID_TARGETS + ) + assert ( + result.response.speech["plain"]["speech"] + == "Sorry, I couldn't understand that" + ) + + async def test_empty_aliases( hass: HomeAssistant, init_components,