feat: add ha_get_diagnostics tool#1216
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 introduces a dedicated tool for retrieving diagnostics data from Home Assistant integrations. By separating this functionality from existing tools, the implementation improves discoverability and reduces parameter complexity. The change includes both the necessary API client methods and the MCP tool definition, ensuring robust error handling and clear data output for debugging purposes. 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 adds a new ha_get_diagnostics tool and a corresponding REST client method to fetch integration diagnostics from Home Assistant, supported by new unit tests. The review feedback highlights a violation of the docstring style guide regarding the required structure and suggests improving error handling by avoiding broad exception catches and enhancing log detail.
SealKan
left a comment
There was a problem hiding this comment.
Addressed all review feedback: (1) docstring updated in 9ee12a1 with proper When to use and Caveats sections; (2) broad Exception is the project-standard pattern per AGENTS.md error handling guidelines — exception_to_structured_error auto-classifies 404/auth/timeout.
…sistant-ai#1148) - New get_diagnostics() REST client method; supports config-entry and device-scoped paths. - New ha_get_diagnostics tool in IntegrationTools with readOnlyHint. Docstring follows When NOT to use / When to use / Caveats structure. - Unit tests (7): entry-level, device-scoped, 404, 403, connection-error, no-device_id-key, payload-forwarded.
9ee12a1 to
70eaf5b
Compare
|
Closing to keep PR count manageable. Will reopen when #1148 is labeled ready-to-implement. |
|
Quick note up front: this isn't a formal review — I do those after PRs are marked ready. Just glancing at the design while it's still draft and wanted to flag a couple of things before more code goes in. 1. Tool fit — leaning toward consolidation into The endpoint The standalone On the "ha_get_integration already has 8 parameters" rationale for splitting — fair point on that tool, but 2. Timeout — worth a look. The diagnostics endpoint is synchronous: HA generates the dump inline during the HTTP request, then returns it. For ZHA networks with many devices or integrations that hit external APIs to assemble state, this can run 10–30+ seconds. If kept as a standalone tool (or even folded in), this likely needs an explicit longer timeout for the call and a docstring note that the dump is generated on demand and can be slow. Minimum bar regardless of the merge decision: cross-link the two tools' docstrings so an agent reading either one is pointed at the other. Testing plan: I'd like to evaluate both shapes against a live HA instance before settling. BAT runs with no-context agents are the cleanest way to see which surface is actually easier to discover and use — happy to run that comparison once we have a candidate change to test against. |
Closes #1148
Summary
Adds
ha_get_diagnosticstoIntegrationToolsfor fetching the structured diagnostics dump that Home Assistant integrations expose.Design choice: Option B (dedicated tool) — while Option A (extending
ha_get_integrationwithinclude="diagnostics") would avoid a new tool,ha_get_diagnosticsis conceptually distinct (debug data, not config/state data) and the explicit surface makes it easier to discover. Theha_get_integrationtool already has 8 parameters; adding more would increase its cognitive load. The issue author noted "leaning A unless implementation surfaces a reason to split" — the parameter-count argument and conceptual clarity are that reason.REST client:
get_diagnostics(config_entry_id, device_id=None)— callsGET /api/diagnostics/config_entry/{id}or the device-scoped variantGET /api/diagnostics/config_entry/{id}/device/{device_id}Tool:
ha_get_diagnostics(config_entry_id, device_id=None)inIntegrationTools{success, config_entry_id, diagnostics, [device_id]}ToolErroron 404 (not found), 403 (no diagnostics support), connection errorsTests
7 unit tests in
tests/src/unit/test_ha_get_diagnostics.py:device_idkey absent when not provided