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

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,70 @@
+"""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 (#1116, #1126)
+- :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.
+
+The token is read from env at construction time; the caller is responsible
+for token-absent handling because each site has its own policy (a rich
+:class:`HomeAssistantAuthError`, a silent ``""`` return, or a 400
+``JSONResponse``) that does not share a common shape.
+"""
+
+from __future__ import annotations
+
+import os
+
+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,
+) -> 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.
+
+    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.
+
+        ``SUPERVISOR_TOKEN`` is read from env at construction time. An absent
+        or empty value produces a literal ``"Bearer "`` header — callers are
+        expected to short-circuit before reaching here (see module docstring
+        for the per-site policies).
+    """
+    token = os.environ.get("SUPERVISOR_TOKEN", "")
+    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_supervisor_client.py

@@ -0,0 +1,153 @@
+"""Unit tests for the shared Supervisor httpx client factory.
+
+Covers :func:`ha_mcp.client.supervisor_client.make_supervisor_httpx_client`,
+which builds the ``httpx.AsyncClient`` instances used by the three direct-
+Supervisor call sites (``rest_client._supervisor_logs_get``,
+``tools_bug_report._fetch_addon_logs``, ``settings_ui._restart_addon``).
+
+The factory itself is a one-liner around ``httpx.AsyncClient`` — these tests
+guard the contract the three call sites rely on:
+
+- ``base_url`` resolved through :func:`ha_mcp._version.get_supervisor_base_url`
+  (so the ``SUPERVISOR_BASE_URL`` E2E override flows through)
+- ``Authorization: Bearer ${SUPERVISOR_TOKEN}`` preset on the client
+- ``timeout`` / ``verify`` forwarded verbatim, both as plain ``float`` and as
+  :class:`httpx.Timeout`
+- Absent or empty ``SUPERVISOR_TOKEN`` produces a ``"Bearer "`` header (the
+  call-site policy is to short-circuit before reaching here; this test pins
+  the documented graceful-degradation behaviour)
+"""
+
+from unittest.mock import patch
+
+import httpx
+import pytest
+
+from ha_mcp.client.supervisor_client import make_supervisor_httpx_client
+
+
+@pytest.fixture
+def supervisor_token(monkeypatch: pytest.MonkeyPatch) -> str:
+    """Set a non-empty ``SUPERVISOR_TOKEN`` for the duration of one test."""
+    token = "test-supervisor-token-abc123"
+    monkeypatch.setenv("SUPERVISOR_TOKEN", token)
+    return token
+
+
+@pytest.fixture
+def no_supervisor_token(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Ensure ``SUPERVISOR_TOKEN`` is unset for the duration of one test."""
+    monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
+
+
+@pytest.fixture
+def no_base_url_override(monkeypatch: pytest.MonkeyPatch) -> None:
+    """Ensure no ``SUPERVISOR_BASE_URL`` override leaks in from the host env."""
+    monkeypatch.delenv("SUPERVISOR_BASE_URL", raising=False)
+
+
+@pytest.mark.asyncio
+async def test_returns_async_client(
+    supervisor_token: str, no_base_url_override: None
+) -> None:
+    """Factory returns a ready-to-use ``httpx.AsyncClient``."""
+    async with make_supervisor_httpx_client(timeout=5.0, verify=True) as client:
+        assert isinstance(client, httpx.AsyncClient)
+
+
+@pytest.mark.asyncio
+async def test_base_url_defaults_to_supervisor(
+    supervisor_token: str, no_base_url_override: None
+) -> None:
+    """Default base URL is the in-addon ``http://supervisor`` hostname."""
+    async with make_supervisor_httpx_client(timeout=5.0, verify=True) as client:
+        assert str(client.base_url) == "http://supervisor"
+
+
+@pytest.mark.asyncio
+async def test_base_url_honors_env_override(
+    supervisor_token: str, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """``SUPERVISOR_BASE_URL`` env override flows through to the client.
+
+    This is the path E2E tests use to point the call sites at a local mock
+    without /etc/hosts hacks — guarding it here keeps that wiring intact.
+    """
+    monkeypatch.setenv("SUPERVISOR_BASE_URL", "http://127.0.0.1:9876")
+    async with make_supervisor_httpx_client(timeout=5.0, verify=True) as client:
+        assert str(client.base_url) == "http://127.0.0.1:9876"
+
+
+@pytest.mark.asyncio
+async def test_authorization_header_uses_token_from_env(
+    supervisor_token: str, no_base_url_override: None
+) -> None:
+    """``Authorization`` header is ``Bearer <SUPERVISOR_TOKEN>``."""
+    async with make_supervisor_httpx_client(timeout=5.0, verify=True) as client:
+        assert client.headers["Authorization"] == f"Bearer {supervisor_token}"
+
+
+@pytest.mark.asyncio
+async def test_authorization_header_with_absent_token(
+    no_supervisor_token: None, no_base_url_override: None
+) -> None:
+    """Missing ``SUPERVISOR_TOKEN`` falls back to a literal ``Bearer `` header.
+
+    Each call site short-circuits before reaching the factory when the token
+    is absent (see module docstring); this test pins the graceful-degradation
+    contract for any future direct caller that forgets to guard.
+    """
+    async with make_supervisor_httpx_client(timeout=5.0, verify=True) as client:
+        assert client.headers["Authorization"] == "Bearer "
+
+
+@pytest.mark.asyncio
+async def test_timeout_forwarded_as_float(
+    supervisor_token: str, no_base_url_override: None
+) -> None:
+    """A plain ``float`` timeout is forwarded to the underlying client.
+
+    httpx normalises ``float`` to a :class:`httpx.Timeout` with the same
+    value applied to all four phases — assert connect + read carry the
+    value through.
+    """
+    async with make_supervisor_httpx_client(timeout=7.5, verify=True) as client:
+        assert client.timeout.connect == 7.5
+        assert client.timeout.read == 7.5
+
+
+@pytest.mark.asyncio
+async def test_timeout_forwarded_as_httpx_timeout(
+    supervisor_token: str, no_base_url_override: None
+) -> None:
+    """An :class:`httpx.Timeout` instance is forwarded verbatim.
+
+    ``rest_client._supervisor_logs_get`` passes ``httpx.Timeout(self.timeout)``
+    rather than a bare float; this test guards that the helper accepts the
+    rich type unchanged.
+    """
+    timeout = httpx.Timeout(connect=2.0, read=30.0, write=30.0, pool=30.0)
+    async with make_supervisor_httpx_client(timeout=timeout, verify=True) as client:
+        assert client.timeout.connect == 2.0
+        assert client.timeout.read == 30.0
+
+
+def test_verify_forwarded_to_async_client_kwarg(supervisor_token: str) -> None:
+    """``verify`` is forwarded verbatim to the underlying ``httpx.AsyncClient``.
+
+    httpx does not expose the constructor's ``verify`` value via any public
+    attribute on the client, so a real-instance assertion can't tell apart
+    ``verify=True`` from ``verify=False``. Patch the constructor and check
+    the kwarg directly — this is the only way to guard against a regression
+    that drops the parameter on the floor while still returning a valid
+    client. The ``verify`` flag is a no-op for plain ``http://supervisor``
+    today, but call sites pass it for symmetry with the HTTPS-override path
+    (#1128) and that contract has to hold.
+    """
+    with patch("ha_mcp.client.supervisor_client.httpx.AsyncClient") as mock_ctor:
+        make_supervisor_httpx_client(timeout=5.0, verify=False)
+        assert mock_ctor.call_args.kwargs["verify"] is False
+
+        mock_ctor.reset_mock()
+        make_supervisor_httpx_client(timeout=5.0, verify=True)
+        assert mock_ctor.call_args.kwargs["verify"] is True

tests/src/unit/test_tools_utility_supervisor_logs.py

@@ -251,15 +251,20 @@ async def test_uses_direct_supervisor_url_and_supervisor_token(
         assert "addon log line 1" in result
         inner_client.get.assert_awaited_once()
         args, kwargs = inner_client.get.call_args
-        assert args[0] == "http://supervisor/addons/81f33d0f_ha_mcp/logs"
-        assert kwargs["headers"]["Authorization"] == "Bearer supervisor-token-test"
+        # After the supervisor_client refactor (#1130), absolute-URL +
+        # per-call Authorization split into base_url + Bearer header on the
+        # constructor; only Accept stays per-call.
+        assert args[0] == "/addons/81f33d0f_ha_mcp/logs"
         assert kwargs["headers"]["Accept"] == "text/plain"
-        # Constructor kwargs (verify_ssl + timeout) propagated from the client
-        # instance — guards against a regression that hard-codes either
-        # (#1126 review item 13).
+        assert "Authorization" not in kwargs.get("headers", {})
+        # Constructor kwargs propagated — guards against a regression that
+        # hard-codes verify, timeout, base_url, or the Bearer token (#1126
+        # review item 13, #1130 helper extraction).
         ctor_kwargs = client_class.call_args.kwargs
         assert ctor_kwargs["verify"] is True  # mirrors mock_client.verify_ssl
         assert isinstance(ctor_kwargs["timeout"], httpx.Timeout)
+        assert ctor_kwargs["base_url"] == "http://supervisor"
+        assert ctor_kwargs["headers"]["Authorization"] == "Bearer supervisor-token-test"
         # The HA-Core-proxy path must NOT have been touched.
         mock_client.httpx_client.request.assert_not_called()
 
@@ -571,12 +576,16 @@ async def test_uses_service_url_with_supervisor_token(
 
         assert "supervisor service log line" in result
         args, kwargs = inner_client.get.call_args
-        assert args[0] == "http://supervisor/supervisor/logs"
-        assert kwargs["headers"]["Authorization"] == "Bearer supervisor-token-test"
+        # After the supervisor_client refactor (#1130): relative path on the
+        # call, base_url + Authorization on the constructor.
+        assert args[0] == "/supervisor/logs"
+        assert "Authorization" not in kwargs.get("headers", {})
         # Constructor kwargs propagated (parity with addon-logs branch).
         ctor_kwargs = client_class.call_args.kwargs
         assert ctor_kwargs["verify"] is True
         assert isinstance(ctor_kwargs["timeout"], httpx.Timeout)
+        assert ctor_kwargs["base_url"] == "http://supervisor"
+        assert ctor_kwargs["headers"]["Authorization"] == "Bearer supervisor-token-test"
 
     @pytest.mark.asyncio
     async def test_raises_auth_error_on_empty_supervisor_token(