refactor: align tools_config_automations.py error-handling with sibling pattern (#1290)#1298
Conversation
…ng pattern (homeassistant-ai#1290) Insert `except ToolError: raise` guard before `except Exception` in get_automation and remove_automation, matching set_automation (L743) and the three sibling files (scripts/scenes/dashboards). Drop manual 404 substring matching and delegate to exception_to_structured_error, which already maps 404 to RESOURCE_NOT_FOUND (helpers.py L116 + L203). Public error code unchanged; action context moves into the structured-error context arg consistent with test_tool_error_signaling.py:110. E2E test_lifecycle.py:1404-1485 asserts on error.code + 'not found'/'404' message keyword; both invariants preserved. Closes homeassistant-ai#1290.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors error handling within the automation configuration tools to align with the established patterns used across other configuration modules. By centralizing error classification and adding proper exception guards, the changes improve maintainability and ensure that structured error metadata is preserved during failure scenarios. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request simplifies error handling in tools_config_automations.py by delegating error response creation to a centralized utility. Feedback indicates that the changes inadvertently converted expected 'not found' debug logs into error logs, which should be corrected. Additionally, the reviewer recommended renaming the removal tool for better naming consistency, catching specific exceptions for improved debuggability, and ensuring stable response schemas.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors error handling in the automation configuration tools by removing manual 404 error detection in favor of the centralized exception_to_structured_error utility and ensuring ToolError exceptions are propagated. Feedback was provided to improve the actionability of error suggestions in ha_config_remove_automation by including a step to check Home Assistant logs, ensuring consistency with other configuration tools.
🧪 Your changes are now in the dev channel!Your PR has been merged to master and is available for testing in the dev channel. Test your changes before the next stable release (biweekly Wednesday): Quick start# Run dev version
uvx ha-mcp-dev
# Check version
uvx ha-mcp-dev --versionDocker: docker pull ghcr.io/homeassistant-ai/ha-mcp:dev
docker run --rm -i \
-e HOMEASSISTANT_URL=http://your-ha:8123 \
-e HOMEASSISTANT_TOKEN=your_token \
ghcr.io/homeassistant-ai/ha-mcp:devFound an issue? Please open a new bug report and mention this PR for context. |
Implementation SummaryChoices Made:
Problems Encountered:
Suggested Improvements:
|
Closes #1290.
What does this PR do?
Aligns
ha_config_get_automationandha_config_remove_automationinsrc/ha_mcp/tools/tools_config_automations.pywith the canonicalexception-handling pattern used by
set_automation(same file, L743) and thethree sibling files (
tools_config_scripts.py,tools_config_scenes.py,tools_config_dashboards.py).Two changes:
Insert
except ToolError: raiseguard beforeexcept Exception as e:in both functions. This is the primary fix requested in [FEATURE] Align tools_config_automations.py with sibling ToolError re-raise guard in get/remove paths #1290. Without it,
any future
raise_tool_error(...)inside the affectedtryblocks wouldbe caught by the bare
except Exceptionand silently re-wrapped throughexception_to_structured_error, losing the originalErrorCode,suggestions, andcontext.Drop manual 404/"not found" substring matching and delegate 404
classification to
exception_to_structured_error, matching the threesibling files. The helper already maps
HomeAssistantAPIError(status_code=404)and
"not found"/"404"-containing messages toErrorCode.RESOURCE_NOT_FOUND(
src/ha_mcp/tools/helpers.py_classify_api_statusL116 and_classify_by_messageL203). The publicerror.codeis unchanged;actioncontext moves from a top-level field into the structured-errorcontextarg, consistent withtest_tool_error_signaling.py:110.Scope note: the issue body suggested keeping (2) out of scope, but a
second pass on
helpers.pyshowed the 404→RESOURCE_NOT_FOUNDmapping isalready centralized in
exception_to_structured_error, so the manual detectionis duplicate logic in the same
exceptblocks as the primary fix. Aligning ithere removes ~30 LOC, matches the sibling pattern in the same paths, and
preserves both e2e invariants asserted in
tests/src/e2e/workflows/automation/test_lifecycle.py:1404-1485(
error.code == "RESOURCE_NOT_FOUND"and"not found"/"does not exist"/"404"keyword in the message). Net diff: -39/+10 LOC, one file.
Tested live: baseline probe against the installed addon (master) confirmed
both tools currently return
RESOURCE_NOT_FOUNDfor nonexistent identifiers;the structured-error delegation route produces the same
error.codeand amessage containing
"not found"from the raw HA API response.Type of change
Testing
uv run pytest)uv run ruff check)Future improvements
The two sibling-gaps surfaced in the issue body but deferred to separate
issues (filed in parallel with this PR):
[FEATURE] Add automation_id return key to ha_config_get_automation for sibling parity #1299 —
get_automationreturn key"identifier"vs"automation_id":scripts/scenes/dashboards return a single typed key; automations return
"identifier"because the input is polymorphic (entity_idORunique_id).Adding
"automation_id"is a return-shape decision (additive vs replacement,semantic guarantee on the value) tracked separately.
[FEATURE] Unify error-response top-level shape across config tool families (automations/scripts/scenes/dashboards) #1300 — error-response top-level shape across the four files: even after
this PR aligns the handling, the four files still emit subtly different
top-level fields (
resource_type,identifier, etc.) depending on whichhelper produces the response. Picking one shape across the family is a
multi-file decision tracked separately.
Checklist