refactor: extract shared Supervisor httpx client helper (#1130)

[Patch76]

What does this PR do?

Closes #1130. Extracts the shared boilerplate from the three direct-Supervisor httpx.AsyncClient call sites into a single factory in a new src/ha_mcp/client/supervisor_client.py. Resolves the needs-choices label by picking the simplest sensible answer for each design question raised in the issue body.

Sites consolidated

• src/ha_mcp/client/rest_client.py:_supervisor_logs_get — carries the addon-logs and system-service-logs branches landed in fix: route addon and HA core log fetches directly to Supervisor on addon installs #1126 (the _get_addon_logs_via_supervisor named in the issue body now delegates to this lower-level helper)
• src/ha_mcp/tools/tools_bug_report.py:_fetch_addon_logs
• src/ha_mcp/settings_ui.py:_restart_addon

Wire-level behaviour preserved: base_url=http://supervisor + relative path on .get/.post joins to the same http://supervisor/<path> URL the originals built by f-string. Authorization: Bearer ${SUPERVISOR_TOKEN} moves from per-call kwargs onto the constructor. verify and timeout stay caller-supplied so each site keeps its existing settings-source choice.

Design decisions on the three open questions

Question	Decision	Why
Per-call vs singleton client	per-call	Three low-frequency endpoints; connection-pool reuse is negligible. Singleton lifecycle (shutdown hook, settings reload) adds coupling that #1126 G2 already declined for the same reason. The helper removes duplication, doesn't change architecture.
Module placement	src/ha_mcp/client/supervisor_client.py	client/ already houses rest_client.py + websocket_client.py + websocket_listener.py — all external-service clients. utils/ is for cross-cutting helpers; a Supervisor-specific wrapper isn't one.
Settings access	verify/timeout parameters, not get_global_settings() inside the helper	Each site already has the right source for its scope (self.verify_ssl snapshot for RESTClient, live get_global_settings() for the module-level helper, server.settings.verify_ssl for the closure). Helper stays a pure factory with no settings coupling — easier to test, no behaviour change at any site.

Token-absent policy stays at the call sites

SUPERVISOR_TOKEN is read in the helper at construction time (matching the originals), but each site keeps its own absent-token branch:

• _supervisor_logs_get raises HomeAssistantAuthError with the rich f"Supervisor token absent at call time for /{path}/logs ..." message
• _fetch_addon_logs returns "" silently (matching the docstring's failure contract)
• _restart_addon returns 400 + CONFIG_VALIDATION_FAILED

These three don't share a common shape, so the helper deliberately doesn't dictate one.

Type of change

• 🐛 Bug fix
• ✨ New feature
• 📚 Documentation
• 🔧 Maintenance/refactor
• 🧪 Tests only
• 💥 Breaking change

Testing

• I have tested these changes with a LLM agent
• All automated tests pass (uv run pytest)
• Code follows style guidelines (uv run ruff check)

New tests

tests/src/unit/test_supervisor_client.py — 8 tests covering the factory contract:

• httpx.AsyncClient returned
• Default base_url == "http://supervisor"
• SUPERVISOR_BASE_URL env override flows through (the path E2E tests use to point at a local mock without /etc/hosts hacks)
• Authorization: Bearer <token> read from env at construction time
• Absent SUPERVISOR_TOKEN produces a literal "Bearer " — graceful-degradation contract for any future direct caller that forgets to guard
• timeout forwarded as plain float (connect/read both carry the value)
• timeout forwarded as httpx.Timeout(...) instance (the rich form _supervisor_logs_get passes)
• verify=True/False kwarg forwarded — patches httpx.AsyncClient and asserts both values, since httpx doesn't expose verify on the public client

Existing tests updated

Two tests in tests/src/unit/test_tools_utility_supervisor_logs.py updated for the post-refactor wire shape:

• URL passed to client.get() is now relative (/addons/.../logs instead of the full URL)
• Authorization header moves from per-call kwargs onto the constructor kwargs
• Added asserts on ctor_kwargs["base_url"] so a regression that drops the base_url surfaces here too

Local: 166 passed across the 4 affected test files (test_supervisor_client, test_tools_utility_supervisor_logs, test_tools_bug_report, test_settings_ui); ruff check + format clean.

Future improvements

• The fourth direct-Supervisor httpx call site, when one materialises, lands here naturally — that's the cost-benefit tipping point the issue body called out.

Checklist

• I have updated documentation if needed

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 consolidates redundant httpx.AsyncClient configuration logic used for communicating with the Home Assistant Supervisor API. By centralizing the base URL and authentication header management into a dedicated factory, the codebase becomes more maintainable and consistent across its various Supervisor-facing endpoints. The changes preserve existing per-call settings like timeouts and SSL verification while ensuring robust behavior for token-absent scenarios.

Highlights

• Refactored Supervisor HTTP Client: Extracted shared boilerplate for Supervisor REST API calls into a new factory function make_supervisor_httpx_client in src/ha_mcp/client/supervisor_client.py.
• Consolidated Call Sites: Updated three locations (rest_client.py, tools_bug_report.py, and settings_ui.py) to use the new factory, standardizing base_url and Authorization header handling.
• Enhanced Test Coverage: Added a new unit test suite tests/src/unit/test_supervisor_client.py to verify the factory contract and updated existing tests to reflect the new wire-level behavior.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request centralizes Home Assistant Supervisor API interactions by introducing a make_supervisor_httpx_client factory. It refactors call sites in the REST client, settings UI, and bug reporting tools to use this helper, ensuring consistent handling of base URLs, timeouts, and authentication. New unit tests were added to validate the factory's logic. Feedback suggests improving robustness when the supervisor token is missing to avoid malformed headers and recommends aligning configuration access with project standards.

[Patch76]

Implementation Summary

Choices Made:

• Per-call httpx.AsyncClient instances, not a singleton — three low-frequency endpoints (addon logs, system-service logs, addon restart) where connection-pool reuse is negligible. Singleton lifecycle (shutdown hook, settings reload) would add coupling that fix: route addon and HA core log fetches directly to Supervisor on addon installs #1126 G2 already declined for the same reason. The helper consolidates duplication, doesn't change architecture.
• Module placement at src/ha_mcp/client/supervisor_client.py — client/ already houses rest_client.py, websocket_client.py, websocket_listener.py (all external-service clients). utils/ is for cross-cutting helpers; a Supervisor-specific wrapper isn't one.
• verify and timeout as caller-supplied parameters, not get_global_settings() inside the helper — each site already has the right source for its scope: self.verify_ssl snapshot in RESTClient, live get_global_settings() for tools_bug_report._fetch_addon_logs, server.settings.verify_ssl for the settings_ui._restart_addon closure. Pure-factory design keeps the helper testable and adds no settings coupling.
• Token-absent policy stays at the call sites, not in the factory — the three sites have three different failure shapes: _supervisor_logs_get raises HomeAssistantAuthError with a rich "addon-mode gate fired but env var not set" message; _fetch_addon_logs returns "" silently per its docstring contract; _restart_addon returns 400 + CONFIG_VALIDATION_FAILED. Centralising the guard would force one of those three shapes on the other two.
• Gemini G1 (medium) declined — flagged the malformed Bearer header as a robustness gap; declined because each call site guards os.environ.get("SUPERVISOR_TOKEN") before invoking the factory and the factory docstring at supervisor_client.py:59-62 documents the contract explicitly. Thread resolved in discussion_r3215198121.

Problems Encountered:

• None during implementation.

Suggested Improvements:

• The fourth direct-Supervisor httpx call site, when one materialises, lands in this helper naturally — that's the cost-benefit tipping point [FEATURE] Add shared Supervisor httpx helper module #1130 called out as the trigger condition for opening this refactor in the first place.

[kingpanther13]

Refactor is mechanically clean — wire-level behaviour, exception flow, and per-site absent-token policies are preserved byte-for-byte across the three migrated call sites. A few changes before this merges.

1. Empty Bearer shouldn't be a documented contract

make_supervisor_httpx_client silently emits Authorization: Bearer when SUPERVISOR_TOKEN is unset, and test_supervisor_client.py::test_authorization_uses_supervisor_token_env pins that as supported behaviour. The three current call sites guard before calling, so it's latent today, but the framing inverts the upstream _supervisor_logs_get posture (fast-fail with a distinct message so operators don't read 401 as "token rejected"). A future direct caller that forgets the guard turns "env var missing" into "Supervisor rejected our token" — exactly the silent-failure mode rest_client.py was written to avoid.

Pick one:

• Raise inside the factory on empty token (preferred — pushes detection to construction time, matches the upstream design).
• Downgrade the docstring + test from "contract" to "hazard," log a warning when the empty-Bearer path is taken.

2. Two call-site test files don't pin the new wire shape

test_tools_bug_report.py (_fetch_addon_logs tests) and test_settings_ui.py (_restart_addon tests) patch httpx.AsyncClient at module scope and pass through transitively after the refactor, but never assert that the relative URL (/addons/self/logs, /addons/self/restart) or the Authorization header actually reach the client. A regression that broke the relative-path conversion or the ctor-set header at either site wouldn't be caught here — only test_tools_utility_supervisor_logs.py:257 pins it for _supervisor_logs_get. Add one call_args assertion per site for parity.

3. Header layering is untested

_supervisor_logs_get passes per-call headers={"Accept": "text/plain"} on top of the factory's ctor-level Authorization. httpx layers these correctly today, but no unit test in test_supervisor_client.py covers the case — a regression that moved Authorization to per-call defaults would silently strip it. Add a test that constructs the factory client, overlays a per-call Accept header, and asserts both headers reach the outbound request.

4. Drop issue/PR refs from code and test comments

Per AGENTS.md, comments shouldn't reference ephemeral context like PR numbers — they rot once the PR is merged and the call sites move. Locations:

• src/ha_mcp/client/supervisor_client.py:8-13 — drop #1116, #1126
• tests/src/unit/test_supervisor_client.py:7-8, 64, 119, 142 — drop (#1128) and "three call sites" framing
• tests/src/unit/test_tools_utility_supervisor_logs.py:254-256, 579-580 — drop "after the supervisor_client refactor (#1130)"; replace with neutral statements about what the code does now

While you're in there

• supervisor_client.py:65 — env read happens at construction, frozen into the header. Fine for today's short-lived async with callers, latent footgun for any future long-lived/reused client across token rotations. One-line note in the function docstring, or move the env read into an httpx event hook.
• supervisor_client.py:36 — verify: bool narrows httpx's bool | str | ssl.SSLContext surface. Intentional? If yes, leave; if not, broaden.
• supervisor_client.py:19-22 vs :59-62 — the token-handling paragraph is duplicated almost verbatim in module + function docstrings. Pick one.
• supervisor_client.py:47-51 — the verify rationale ("SUPERVISOR_BASE_URL env-var overrides that may be HTTPS in non-add-on test rigs") is worth spot-checking that get_supervisor_base_url actually permits HTTPS overrides; otherwise the justification is misleading.
• Across the migrated sites — pinning "no caller-supplied Authorization in per-call kwargs" with an assert "Authorization" not in kwargs.get("headers", {}) (already done at test_tools_utility_supervisor_logs.py:259) is worth replicating at the other two test sites while you're adding the URL assertions.

[Patch76]

Round-2 fixes pushed. Walkthrough of each item from the review:

1. Empty Bearer contract → Option A (raise in factory). make_supervisor_httpx_client now raises RuntimeError if SUPERVISOR_TOKEN is unset or empty. The existing test_authorization_header_with_absent_token is rewritten as test_raises_on_absent_token; a sibling test_raises_on_empty_token covers the empty-string case. All three call sites already guard before invoking the factory, so the raise is a pure future-proof defensive — no current caller reaches it.

2. Wire-shape asserts on the two call-site test files. New tests test_passes_relative_url_with_ctor_authorization (test_tools_bug_report.py) and test_posts_relative_url_with_ctor_authorization (test_settings_ui.py) mirror the pattern at test_tools_utility_supervisor_logs.py:253-266: relative path on call, Authorization absent from per-call kwargs, base_url + Authorization on ctor kwargs.

3. Header-layering test. test_per_call_headers_layer_over_ctor_authorization in test_supervisor_client.py constructs a real client, calls client.build_request("GET", "/addons/self/logs", headers={"Accept": "text/plain"}), and asserts both ctor-Authorization and per-call Accept reach the merged request headers.

4. Drop PR/issue refs. All #1116, #1126, #1128, #1130 framing dropped from supervisor_client.py, test_supervisor_client.py, and the cited blocks in test_tools_utility_supervisor_logs.py. Remaining #1126 review item N / #1116 refs in that file are regression-test descriptions tied to those prior PRs' findings, predate this PR, and stay scoped to their own fixes.

"While you're in there"

• :65 env-read frozen at construction: added a Note: paragraph to the function docstring covering the long-lived/reused-client footgun.
• :36 verify narrowing: broadened to bool | str | ssl.SSLContext to match httpx's full surface; docstring updated.
• :19-22 vs :59-62 docstring dedup: module-level token-handling paragraph removed; function-docstring Raises: block carries the policy.
• :47-51 verify rationale spot-check: get_supervisor_base_url at _version.py:69 returns os.environ.get("SUPERVISOR_BASE_URL", "http://supervisor") without protocol restriction, so HTTPS overrides flow through unchanged. Rationale stands.
• Cross-site Authorization-not-in-kwargs asserts: bundled into the new wire-shape tests in finding 2.

Sibling fix

test_config_toggles_section_renders_in_templates (test_tools_bug_report.py) now delenvs SUPERVISOR_TOKEN before invoking ha_report_issue_func. The SimpleNamespace fake settings carry only the toggle fields (no verify_ssl); without the delenv, the addon-logs path runs on containers with the token set and the fake hits an AttributeError. Pre-existing on master; CI passed because the runner has no token. In scope per the boy-scout rule since the file is touched by this PR.

[kingpanther13]

Round-2 fixes verified. All four review findings addressed: factory now raises RuntimeError on absent/empty SUPERVISOR_TOKEN (loud fail-fast replaces the empty-Bearer footgun); wire-shape pinned at all three call sites; header layering test exercises the Accept-over-Authorization merge through client.build_request; PR/issue refs neutralised in the right places. "While you're in there" items all picked up: verify type broadened to the full httpx surface, module-docstring dedup, long-lived-caller footgun noted, verify rationale spot-checked. Sibling delenv fix in test_config_toggles_section_renders_in_templates correctly addresses a pre-existing latent AttributeError.

[github-actions]

🧪 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):
📖 Dev Channel Documentation

Quick start

# Run dev version
uvx ha-mcp-dev

# Check version
uvx ha-mcp-dev --version

Docker:

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:dev

---

Found an issue? Please open a new bug report and mention this PR for context.