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

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

Three direct-Supervisor httpx call sites all built fresh AsyncClients with
the same boilerplate (base URL http://supervisor, Authorization: Bearer
$SUPERVISOR_TOKEN). Move that into a single factory in client/supervisor_client.py
and have the call sites pass relative paths instead of full URLs:

- client/rest_client.py:_supervisor_logs_get
- tools/tools_bug_report.py:_fetch_addon_logs
- settings_ui.py:_restart_addon

Per-call clients (vs. a singleton) preserved — these endpoints are
low-frequency, connection-pool reuse is negligible, and singleton lifecycle
adds shutdown/reload coupling that the issue's #1126 G2 review already
declined for the same reason. verify and timeout stay caller-supplied so
each site keeps its existing settings-source choice (instance snapshot vs.
live read). Token is read from env in the helper at construction time,
matching the original sites; the absent-token policy stays at the call
site (rich exception, silent empty string, or 400 JSONResponse — these
don't share a common shape and the helper shouldn't dictate one).

Tests: new tests/src/unit/test_supervisor_client.py covers the factory
contract (base_url, env override, Bearer header, timeout/verify
forwarding, absent-token graceful degradation). Two existing tests in
test_tools_utility_supervisor_logs.py updated to the new contract: URL
passed to .get() is now relative; Authorization header asserted on the
constructor kwargs instead of the per-call kwargs.

Closes #1130.

* fix: address round-2 review findings on #1130

- Raise RuntimeError on absent/empty SUPERVISOR_TOKEN at the factory so
  the malformed Bearer header never reaches Supervisor.
- Add wire-shape unit tests for _fetch_addon_logs and _restart_addon
  asserting relative URLs, ctor-set Authorization, and no per-call
  Authorization kwarg.
- Add a header-layering test pinning per-call Accept overlay on the
  ctor-set Authorization.
- Drop ephemeral PR/issue refs from code and test comments per
  AGENTS.md.
- Broaden the verify parameter type to httpx's full surface, add an
  env-read-at-construction Note to the docstring, dedup the
  token-handling paragraph.
- Sibling: test_config_toggles_section_renders_in_templates now
  delenv's SUPERVISOR_TOKEN so the SimpleNamespace fake (missing
  verify_ssl) doesn't hit the addon-logs path on containers with
  the env var set.

---------

Co-authored-by: Patch76 <mkglasmoor@gmail.com>

Author: Patch76

src/ha_mcp/client/rest_client.py

@@ -13,6 +13,7 @@
 
 from .._version import get_supervisor_base_url, is_running_in_addon
 from ..config import get_global_settings
+from .supervisor_client import make_supervisor_httpx_client
 
 
 def _is_ssl_error(exc: BaseException) -> bool:
@@ -555,23 +556,21 @@ async def _supervisor_logs_get(self, path: str) -> str:
                 "(addon-mode gate fired but SUPERVISOR_TOKEN env var not set)"
             )
 
-        url = f"{get_supervisor_base_url()}/{path}/logs"
-        logger.debug("Fetching %s via Supervisor direct", url)
+        relative_path = f"/{path}/logs"
+        logger.debug(
+            "Fetching %s%s via Supervisor direct",
+            get_supervisor_base_url(),
+            relative_path,
+        )
 
         try:
-            async with httpx.AsyncClient(
+            async with make_supervisor_httpx_client(
                 timeout=httpx.Timeout(self.timeout),
-                # `verify` is a no-op for plain http://supervisor, but kept
-                # for symmetry with the other two direct-Supervisor httpx
-                # clients (#1128 establishes the 3-site convention).
                 verify=self.verify_ssl,
             ) as client:
                 response = await client.get(
-                    url,
-                    headers={
-                        "Authorization": f"Bearer {token}",
-                        "Accept": "text/plain",
-                    },
+                    relative_path,
+                    headers={"Accept": "text/plain"},
                 )
         except httpx.TimeoutException as e:
             raise HomeAssistantConnectionError(

src/ha_mcp/client/supervisor_client.py

@@ -0,0 +1,88 @@
+"""Shared factory for direct-Supervisor httpx clients.
+
+Three call sites in the codebase talk directly to the Home Assistant
+Supervisor REST API at ``http://supervisor`` rather than through
+``HomeAssistantClient.httpx_client`` (which is bound to HA Core, not the
+Supervisor — different base URL, different token, different role gate):
+
+- :meth:`ha_mcp.client.rest_client.HomeAssistantClient._supervisor_logs_get`
+  — fetches addon and system-service logs
+- :func:`ha_mcp.tools.tools_bug_report._fetch_addon_logs` — bundles ha-mcp's
+  own addon logs into a bug-report payload
+- :func:`ha_mcp.settings_ui._restart_addon` — POSTs ``/addons/self/restart``
+  from the settings UI
+
+All three share the same boilerplate (base URL, ``Authorization: Bearer
+${SUPERVISOR_TOKEN}`` header), so this module supplies a single factory and
+keeps the three sites consistent.
+"""
+
+from __future__ import annotations
+
+import os
+import ssl
+
+import httpx
+
+from .._version import get_supervisor_base_url
+
+__all__ = ["make_supervisor_httpx_client"]
+
+
+def make_supervisor_httpx_client(
+    *,
+    timeout: float | httpx.Timeout,
+    verify: bool | str | ssl.SSLContext,
+) -> httpx.AsyncClient:
+    """Construct an ``httpx.AsyncClient`` pre-configured for the Supervisor REST API.
+
+    Args:
+        timeout: Per-request timeout. Accepts either a plain ``float``
+            (seconds, applied to all phases) or a full :class:`httpx.Timeout`
+            for finer-grained control.
+        verify: TLS verify policy. A no-op for the default
+            ``http://supervisor`` base URL (plain HTTP — no TLS to verify),
+            but kept as a parameter because :func:`get_supervisor_base_url`
+            honours ``SUPERVISOR_BASE_URL`` env-var overrides that may be
+            HTTPS in non-add-on test rigs. The full httpx ``verify`` surface
+            (``bool``, CA-bundle path, or :class:`ssl.SSLContext`) is
+            accepted and forwarded verbatim.
+
+    Returns:
+        A new :class:`httpx.AsyncClient` bound to the Supervisor base URL
+        with ``Authorization: Bearer ${SUPERVISOR_TOKEN}`` preset. Callers
+        pass relative paths (``/addons/self/logs``) to ``client.get/post``;
+        ``base_url`` joins them onto the Supervisor host.
+
+    Raises:
+        RuntimeError: ``SUPERVISOR_TOKEN`` is unset or empty in the
+            environment. Each call site has its own absent-token policy
+            (a rich :class:`HomeAssistantAuthError`, a silent ``""``
+            return, or a 400 ``JSONResponse``) that does not share a
+            common shape, so the factory cannot translate. Detecting the
+            absence at construction time prevents a malformed
+            ``Authorization: Bearer `` header from being read as a token
+            rejection by Supervisor, which would mask the missing-env-var
+            root cause.
+
+    Note:
+        ``SUPERVISOR_TOKEN`` is read from env at construction time and
+        baked into the constructed client's ``Authorization`` header.
+        Reusing a single client across token rotations would not pick up
+        the new value — short-lived ``async with`` callers are unaffected,
+        but a future long-lived caller would need to discard and re-create.
+    """
+    token = os.environ.get("SUPERVISOR_TOKEN", "")
+    if not token:
+        raise RuntimeError(
+            "SUPERVISOR_TOKEN is not set; "
+            "make_supervisor_httpx_client cannot construct an "
+            "authenticated client. Callers must verify the token is "
+            "present before invoking the factory."
+        )
+    return httpx.AsyncClient(
+        base_url=get_supervisor_base_url(),
+        timeout=timeout,
+        verify=verify,
+        headers={"Authorization": f"Bearer {token}"},
+    )

src/ha_mcp/settings_ui.py

@@ -19,7 +19,8 @@
 from starlette.requests import Request
 from starlette.responses import HTMLResponse, JSONResponse
 
-from ._version import get_supervisor_base_url, is_running_in_addon
+from ._version import is_running_in_addon
+from .client.supervisor_client import make_supervisor_httpx_client
 from .errors import ErrorCode, create_error_response
 from .transforms import DEFAULT_PINNED_TOOLS
 from .utils.data_paths import get_data_dir
@@ -893,8 +894,7 @@ async def _save_tools(request: Request) -> JSONResponse:
         )
 
     async def _restart_addon(_: Request) -> JSONResponse:
-        token = os.environ.get("SUPERVISOR_TOKEN")
-        if not token:
+        if not os.environ.get("SUPERVISOR_TOKEN"):
             return JSONResponse(
                 create_error_response(
                     ErrorCode.CONFIG_VALIDATION_FAILED,
@@ -906,13 +906,10 @@ async def _restart_addon(_: Request) -> JSONResponse:
         # Short timeout — the supervisor kills our process during restart so
         # the connection will drop. A connection drop is actually success.
         try:
-            async with httpx.AsyncClient(
+            async with make_supervisor_httpx_client(
                 timeout=5.0, verify=server.settings.verify_ssl
             ) as client:
-                resp = await client.post(
-                    f"{get_supervisor_base_url()}/addons/self/restart",
-                    headers={"Authorization": f"Bearer {token}"},
-                )
+                resp = await client.post("/addons/self/restart")
         except (httpx.ReadError, httpx.RemoteProtocolError):
             # Connection dropped mid-request — restart is happening.
             # `ConnectError` is deliberately NOT in this tuple: it fires

src/ha_mcp/tools/tools_bug_report.py

@@ -20,7 +20,7 @@
 
 from ha_mcp import __version__
 
-from .._version import get_supervisor_base_url
+from ..client.supervisor_client import make_supervisor_httpx_client
 from ..config import Settings, get_global_settings
 from ..utils.usage_logger import (
     AVG_LOG_ENTRIES_PER_TOOL,
@@ -349,18 +349,14 @@ async def _fetch_addon_logs() -> str:
     """
     # Redundant with the caller's `install_method == "addon"` gate, but kept
     # as a defensive guard for any direct callers added later.
-    token = os.environ.get("SUPERVISOR_TOKEN", "")
-    if not token:
+    if not os.environ.get("SUPERVISOR_TOKEN"):
         return ""
 
     try:
-        async with httpx.AsyncClient(
+        async with make_supervisor_httpx_client(
             timeout=10.0, verify=get_global_settings().verify_ssl
         ) as http_client:
-            resp = await http_client.get(
-                f"{get_supervisor_base_url()}/addons/self/logs",
-                headers={"Authorization": f"Bearer {token}"},
-            )
+            resp = await http_client.get("/addons/self/logs")
             if resp.status_code != 200:
                 logger.info("Addon log fetch returned HTTP %s", resp.status_code)
                 return ""

tests/src/unit/test_settings_ui.py

@@ -598,3 +598,36 @@ async def test_supervisor_4xx_returns_502(self, monkeypatch):
         assert resp.status_code == 502
         body = json.loads(resp.body)
         assert body["success"] is False
+
+    @pytest.mark.asyncio
+    async def test_posts_relative_url_with_ctor_authorization(self, monkeypatch):
+        """Post-refactor wire shape: relative path on ``.post()``, with
+        ``Authorization`` assembled in the constructor kwargs rather than
+        per-call. Mirrors the parallel assertions in
+        ``test_tools_utility_supervisor_logs.py`` so a regression that
+        hard-codes the absolute URL or moves the Bearer header back to
+        per-call kwargs is caught on the restart-handler branch too.
+        """
+        restart = self._capture_handler(monkeypatch, with_token=True)
+        request = MagicMock()
+
+        response = MagicMock()
+        response.status_code = 200
+        mock_client = MagicMock()
+        mock_client.post = AsyncMock(return_value=response)
+        cm = MagicMock()
+        cm.__aenter__ = AsyncMock(return_value=mock_client)
+        cm.__aexit__ = AsyncMock(return_value=None)
+
+        client_class = MagicMock(return_value=cm)
+        with patch("ha_mcp.settings_ui.httpx.AsyncClient", client_class):
+            await restart(request)
+
+        mock_client.post.assert_awaited_once()
+        args, kwargs = mock_client.post.call_args
+        assert args[0] == "/addons/self/restart"
+        assert "Authorization" not in kwargs.get("headers", {})
+
+        ctor_kwargs = client_class.call_args.kwargs
+        assert ctor_kwargs["base_url"] == "http://supervisor"
+        assert ctor_kwargs["headers"]["Authorization"] == "Bearer fake-supervisor-token"