fix: add ast-grep rule and fix hand-built error dicts - #895
Conversation
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 addresses several pre-existing violations where tool functions returned error dictionaries instead of raising 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 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 counter productive. 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
|
sergeykad
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors error handling across several tool modules—including backup, device control, and entity management—by replacing manual error dictionary returns with structured ToolError exceptions. The reviewer identified an improvement opportunity in tools_entities.py where a generic ValueError is raised; this should be replaced with a structured ToolError using the INTERNAL_ERROR code to comply with repository standards for consistent error reporting.
kingpanther13
left a comment
kingpanther13
left a comment
There was a problem hiding this comment.
Review
Good work identifying and fixing the {"success": False} dict-return pattern — this is a real MCP spec issue where isError=true doesn't get set. The ast-grep rule is a nice addition for regression prevention.
Merge order conflict: tools_hacs.py changes duplicate #871
The tools_hacs.py changes in this PR overlap almost entirely with #871. The same 3 error paths (ha_hacs_repository_info not-found, ha_hacs_add_repository invalid-format, ha_hacs_download not-found) were already converted from return await add_timezone_metadata(client, {"success": False, ...}) to raise_tool_error(create_error_response(...)) in #871 — at your request on that PR. Those changes are now being duplicated here.
Requested change: #871 should merge first since it restructures the file significantly (consolidating 4 HACS tools → 2). Please rebase this PR onto #871 once it merges and drop the tools_hacs.py hunks to avoid conflicts and duplicated work.
Test should still assert exposure_succeeded
In test_expose_only_entity_not_found_raises_tool_error, the old assertion assert "exposure_succeeded" in result was removed. Since create_error_response merges context into the top-level response via response.update(context), and the production code passes context={"entity_id": entity_id, "exposure_succeeded": exposure_result}, this data is still present at result["exposure_succeeded"].
Requested change: Re-add the assertion to verify the exposure context is preserved in the error:
assert result["exposure_succeeded"] == {"conversation": True}Remaining {"success": False} dict return in _update_single_entity
_update_single_entity (~line 237) still returns {"success": False, ...} for partial exposure failures via a variable (return response), so the new ast-grep rule won't catch it. This is the same pattern from the LLM's perspective — isError won't be set.
Requested change: Either convert this to raise_tool_error(create_error_response(...)) with the partial-failure context in context= (so isError=true is set and the LLM can still read exposure_succeeded/exposure_failed from the structured error), or document explicitly in a code comment why this case intentionally stays as a dict return.
Everything else looks clean — the ValueError choice in _fetch_entity is well-justified for the asyncio.gather(return_exceptions=True) pattern, the backup.py refactor simplifies both callers nicely, and the error codes are all appropriate.
- Revert tools_hacs.py changes (will be handled by #871) - Fix _update_single_entity exposure failure to raise ToolError instead of returning {"success": False} via variable assignment - Re-add exposure_succeeded assertion in test - Update 3 exposure failure tests to expect ToolError
Fixes 6 of 9 violations caught by the new `no-return-success-false` ast-grep rule (remaining 3 are in tools_hacs.py, addressed by #871). Returning `{"success": False, ...}` from tool functions doesn't set `isError=true` on the MCP response, so LLM agents may not recognize these as errors. Changes: - device_control.py: failed/timeout operation status now raises ToolError - tools_entities.py: ha_set_entity exposure failure now raises ToolError - tools_entities.py: _fetch_entity raises ValueError instead of returning error dict (callers already handle exceptions via return_exceptions) - backup.py: _get_backup_password raises ToolError directly instead of returning (None, error_dict) tuples; callers updated accordingly - test_tools_entities.py: updated test to expect ToolError
The existing `no-return-error-response` rule catches `return create_error_response(...)`
but misses hand-built `return {"success": False, ...}` dicts that bypass `raise_tool_error`.
These return `isError=false` in MCP responses, so LLM agents may not recognize them as errors.
The new rule catches any dictionary with `"success": False` inside a return statement,
regardless of key ordering. It correctly excludes batch item appends (`.append(...)`) and
dict assignments which are legitimate uses.
Currently flags 9 pre-existing violations across 4 files:
- tools_hacs.py (3), device_control.py (2), tools_entities.py (2), backup.py (2)
The previous commit dropped partial-success data when converting to raise_tool_error. Restore it via the context dict so LLM agents can see which exposure changes succeeded before the failure.
- Revert tools_hacs.py changes (will be handled by #871) - Fix _update_single_entity exposure failure to raise ToolError instead of returning {"success": False} via variable assignment - Re-add exposure_succeeded assertion in test - Update 3 exposure failure tests to expect ToolError
b7c9a49 to
7c3eea6
Compare
|
All three items addressed:
|
kingpanther13
left a comment
kingpanther13
left a comment
There was a problem hiding this comment.
All three requested changes addressed:
tools_hacs.pyhunks dropped — no more overlap with #871exposure_succeededassertion restored (and strengthened to check value, not just presence)_update_single_entitypartial failure path converted toraise_tool_errorwith full context, plus 4 related tests updated
CI all green. LGTM.
🧪 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. |
2 participants
What does this PR do?
Adds a new ast-grep rule (
no-return-success-false) and fixes all pre-existing violations where tool functions return{"success": False, ...}dicts instead of raisingToolError. Returning error dicts doesn't setisError=trueon the MCP response (per MCP spec), so LLM agents may not recognize these as errors.The 3
tools_hacs.pyviolations were already fixed by #871 (now merged).New ast-grep rule
The existing
no-return-error-responserule catchesreturn create_error_response(...)but misses hand-built error dicts likereturn await add_timezone_metadata(client, {"success": False, ...}). The new rule uses AST-level matching to catch any dict with"success": Falseinside areturnstatement, regardless of key ordering. Correctly excludes batch item appends (.append(...)) and dict assignments.Fixes
device_control.py: failed/timeout operation status inget_device_operation_statusnow raisesToolErrorwithSERVICE_CALL_FAILED/TIMEOUT_OPERATIONtools_entities.py:ha_set_entityexposure failures (both partial and expose-only) now raiseToolError, preservingexposure_succeeded/exposure_failed/partialcontexttools_entities.py:_fetch_entityinner function raisesValueErroron failure instead of returning error dict (callers handle viareturn_exceptions=True)backup.py:_get_backup_passwordraisesToolErrordirectly instead of returning(None, error_dict)tuples; callers updated accordinglytest_tools_entities.py: updated 4 unit tests to expectToolErrorType of change
Testing
uv run pytest) — 952 unit tests, E2E testsuv run ruff check)Checklist