fix(screenshot): re-arm the theme guard for themed captures only

docs/beta.md

@@ -177,18 +177,22 @@ token grants. If the token is missing or invalid, Puppet lands on the login
 page and (by its design) restarts; ha-mcp surfaces this as a clear "set the
 engine's access token" error rather than a silent failure.
 
-Puppet's theme and dark-mode renderer controls used to dispatch Home
-Assistant's `settheme` event on every cold render, which Home Assistant
-persisted on the frontend profile of the user whose token the engine runs with
-— and synced to that user's real web and mobile sessions, flipping a dark-mode
-user's whole UI to light on every screenshot (#1909). Recent Puppet versions
-fixed that cold-render dispatch, so ha-mcp's snapshot/restore bracket around
-each capture is now disabled (#1991); the guard code is retained so it can be
-switched back on if a future engine regression reintroduces the write. If you
-run an older Puppet build, update the app (or your self-hosted sidecar
-image) — older engines still persist the theme selection and will keep
-flipping it. A dedicated Puppet account remains a sound belt-and-suspenders
-setup. Language selection is local to Puppet's browser session.
+Puppet's theme and dark-mode renderer controls dispatch Home Assistant's
+`settheme` event, which Home Assistant persists on the frontend profile of the
+user whose token the engine runs with — and syncs to that user's real web and
+mobile sessions, flipping that user's whole UI (#1909). Upstream stopped the
+dispatch for renders that request nothing
+(balloob/home-assistant-addons#89), but only for that case: passing `theme` or
+`dark_mode` still writes, on every engine version.
+
+ha-mcp therefore brackets **only themed captures** with a snapshot/restore of
+that user's saved theme, so the flip is undone automatically. Captures that
+request no theme are not bracketed and issue no writes at all, which is what
+keeps `readOnlyHint: True` accurate on the screenshot tools (#1991). Concurrent
+themed captures against the same engine user are serialized so their brackets
+cannot interleave. A dedicated Puppet account remains a sound
+belt-and-suspenders setup. Language selection is local to Puppet's browser
+session.
 
 To change the Puppet engine app's own options (such as `keep_browser_open`)
 or to restart it, use `ha_manage_app`; the screenshot tools only render and

src/ha_mcp/dashboard_screenshot/capture.py

@@ -855,15 +855,12 @@ async def capture_dashboard_images(
     preset's native orientation. ``full_page`` is a compatibility alias for
     requesting the engine's native ``WIDTHxauto`` viewport.
 
-    The batch used to be bracketed by a :class:`ThemeGuard` that restored the
-    engine user's saved frontend theme when a cold render changed it (issue
-    #1909). That bracket is currently disabled (#1991): upstream Puppet no
-    longer dispatches ``settheme`` on cold renders, so there is nothing to undo.
-    The guard construction and the ``client`` / ``capture_warnings`` plumbing
-    are retained so the bracket can be re-enabled by uncommenting the
-    snapshot/restore calls if a future engine regression reintroduces the write;
-    while disabled ``capture_warnings`` simply stays empty and never affects the
-    captures themselves.
+    A batch that requests a theme (``theme`` or ``dark_mode``) is bracketed by
+    a :class:`ThemeGuard` that restores the engine user's saved frontend theme
+    afterwards, because such renders make Puppet write it (issue #1909).
+    Unthemed batches issue no writes at all. Snapshot and restore failures
+    surface through ``capture_warnings``. A theme-lock timeout rejects the
+    themed capture to prevent an unserialized render.
     """
     path = _validate_dashboard_path(dashboard_path)
     options = validate_capture_parameters(
@@ -888,16 +885,48 @@ async def capture_dashboard_images(
     mime_type = _MIME_TYPES[options.image_format]
     captures: list[DashboardImageCapture] = []
 
-    # ThemeGuard bracket — currently DISABLED (#1991). Stock Puppet used to
-    # dispatch a theme write into the authenticated frontend on cold renders,
-    # which Home Assistant persisted to the engine user's real profile (#1909);
-    # ha-mcp snapshotted before and restored after to undo it. Upstream Puppet
-    # has since fixed the cold-render settheme dispatch, so the bracket is no
-    # longer needed. The guard is still constructed (and the snapshot/restore
-    # calls kept below, commented out) so it can be re-enabled by uncommenting
-    # if a future engine regression reintroduces the write.
-    guard = ThemeGuard.for_capture(engine_target.addon_credential, client)
-    # await guard.take_snapshot()
+    # ThemeGuard bracket, armed only for renders that request a theme.
+    #
+    # Puppet dispatches a ``settheme`` event into the authenticated frontend,
+    # which Home Assistant persists to the engine user's real profile and
+    # syncs to that user's live sessions (#1909). Upstream stopped the
+    # no-parameter dispatch (balloob/home-assistant-addons#89), but that fix
+    # is scoped by its own title — "don't dispatch settheme when no
+    # theme/dark was requested" — so an explicit ``theme=``/``dark`` render
+    # still writes, on every engine version.
+    #
+    # Arming only that path keeps unthemed captures free of any write, which
+    # is what makes ``readOnlyHint: True`` honest for the overwhelmingly
+    # common case (#1991, PR #2014).
+    theme_requested = options.theme is not None or options.dark_mode
+    guard = ThemeGuard.for_capture(
+        engine_target.addon_credential, client, armed=theme_requested
+    )
+    # Bound the wait by the caller's own render budget: they are already
+    # willing to wait that long, and a themed render can easily hold the
+    # bracket longer than a short fixed timeout.
+    await guard.take_snapshot(lock_timeout=options.render_timeout_seconds)
+    if guard.lock_timed_out:
+        # Rendering unserialized here would reintroduce exactly the interleave
+        # the lock exists to prevent (this batch would snapshot the other
+        # batch's transient theme and restore it afterwards), so fail instead.
+        raise_tool_error(
+            create_error_response(
+                ErrorCode.INTERNAL_ERROR,
+                "Another themed dashboard screenshot is still in progress for "
+                "this screenshot-engine account.",
+                details=(
+                    "Themed captures are serialized so their theme "
+                    "snapshot/restore brackets cannot interleave and strand "
+                    "the account on the wrong theme."
+                ),
+                suggestions=[
+                    "Retry once the in-flight capture finishes.",
+                    "Omit theme/dark_mode to capture without the bracket.",
+                ],
+                context={"path": path},
+            )
+        )
     batch_error: ToolError | None = None
     try:
         async with httpx.AsyncClient(
@@ -976,8 +1005,26 @@ async def capture_dashboard_images(
         # Held (not re-raised here) so the restore in ``finally`` runs first
         # and its outcome can be attached to the error payload below.
         batch_error = exc
+    except Exception as exc:
+        # A non-ToolError failure (e.g. raised while entering the HTTP client
+        # context) would otherwise bypass _reraise_with_guard_warnings and
+        # reach the caller's generic handler with the guard's warnings
+        # dropped. It must become a *structured* ToolError: the merge helper
+        # parses the message as JSON and re-raises unchanged when that is not
+        # a dict, so wrapping the bare text would still lose the warnings.
+        logger.exception("Dashboard capture batch failed unexpectedly")
+        batch_error = ToolError(
+            json.dumps(
+                create_error_response(
+                    ErrorCode.INTERNAL_ERROR,
+                    "Dashboard screenshot capture failed unexpectedly.",
+                    details=f"{exc.__class__.__name__}: {exc}",
+                    context={"path": path},
+                )
+            )
+        )
     finally:
-        # await guard.restore()  # ThemeGuard bracket disabled (#1991) — see above.
+        await guard.restore()
         if capture_warnings is not None:
             capture_warnings.extend(guard.warnings)
 

src/ha_mcp/dashboard_screenshot/theme_guard.py

@@ -1,23 +1,20 @@
 """Snapshot and restore the engine user's saved frontend theme (issue #1909).
 
-NOTE: This guard is currently disabled at its call site (``capture.py``)
-because upstream Puppet fixed the cold-render ``settheme`` dispatch that made
-the bracket necessary (#1991). The code here is retained unchanged so the
-bracket can be re-enabled by uncommenting the snapshot/restore calls in
-``capture.py`` if a future engine regression reintroduces the write.
-
-Stock Puppet dispatches Home Assistant's ``settheme`` event on every
-cold-browser render — its ``dark`` query flag is presence-based, so "not
-requested" reaches the frontend as an explicit "light". Home Assistant
-persists that selection server-side per user (``frontend/set_user_data``,
-key ``"theme"``) and syncs it to every session of the user whose long-lived
-token the engine runs with. A plain screenshot call therefore flips a
-dark-mode user's real web and mobile UI to light.
-
-ha-mcp cannot suppress the engine's write, so every capture batch is
+Puppet dispatches Home Assistant's ``settheme`` event for renders that ask
+for a theme. Home Assistant persists that selection server-side per user
+(``frontend/set_user_data``, key ``"theme"``) and syncs it to every session
+of the user whose long-lived token the engine runs with, so such a
+screenshot flips that user's real web and mobile UI.
+
+Upstream stopped the dispatch for renders that request nothing
+(balloob/home-assistant-addons#89), but by its own title only for that case —
+an explicit ``theme=``/``dark`` render still writes on every engine version.
+
+ha-mcp cannot suppress the engine's write, so themed capture batches are
 bracketed instead: read the engine user's saved theme before rendering and
 write it back afterwards when the render changed it (an unchanged value is
-never rewritten).
+never rewritten). Unthemed batches are not bracketed and issue no writes,
+keeping the screenshot tools honestly read-only (#1991).
 
 Credential resolution mirrors engine discovery:
 
@@ -75,6 +72,40 @@
 # restore, and let the late write survive).
 RESTORE_SETTLE_SECONDS = 1.0
 
+# The guard is best-effort and runs *before* the engine is contacted, so it
+# must never eat the caller's MCP timeout window. send_command defaults to a
+# 30s wait; a guard that hangs that long would stop a healthy engine ever
+# returning an image.
+COMMAND_TIMEOUT_SECONDS = 5.0
+
+# Fallback bound on waiting for another batch's bracket, used when the caller
+# supplies no render budget of its own. Callers should pass their render
+# timeout: a themed render routinely holds the bracket for its whole duration,
+# and giving up early would strand the capture.
+LOCK_WAIT_SECONDS = 60.0
+
+# Snapshot-through-restore must be serialized per engine user. Two concurrent
+# batches would otherwise interleave as: A renders and clobbers dark->light,
+# B snapshots that transient light, A restores dark, B restores light —
+# leaving the user permanently on the clobbered value.
+#
+# Keyed by running event loop as well as engine user: an asyncio.Lock binds
+# to the loop that first awaits it and raises if reused from another, so a
+# process-global-by-credential dict would break any second loop (and leaks
+# into unit tests, which get a fresh loop each). The server runs one loop, so
+# in production this holds exactly one entry per engine user.
+_ENGINE_LOCKS: dict[tuple[int, str, str], asyncio.Lock] = {}
+
+
+def _engine_lock(credential: EngineCredential) -> asyncio.Lock:
+    """Return this loop's lock for one engine user."""
+    key = (id(asyncio.get_running_loop()), credential.url, credential.token)
+    lock = _ENGINE_LOCKS.get(key)
+    if lock is None:
+        lock = asyncio.Lock()
+        _ENGINE_LOCKS[key] = lock
+    return lock
+
 
 @dataclass(frozen=True, slots=True)
 class EngineCredential:
@@ -88,6 +119,7 @@ class EngineCredential:
 
     url: str
     token: str
+    verify_ssl: bool | None = None
 
 
 def addon_credential_from_options(
@@ -119,7 +151,16 @@ def _client_credential(client: Any) -> EngineCredential | None:
     token = str(getattr(client, "token", "") or "").strip()
     if not base_url.startswith(("http://", "https://")) or not token:
         return None
-    return EngineCredential(url=base_url, token=token)
+    # Carry the client's own TLS setting: a direct client built with
+    # verify_ssl=False (self-signed HA) must not fall back to the global
+    # default here, or every guard session fails and the theme stays
+    # clobbered while the render itself succeeds.
+    verify_ssl = getattr(client, "verify_ssl", None)
+    return EngineCredential(
+        url=base_url,
+        token=token,
+        verify_ssl=verify_ssl if isinstance(verify_ssl, bool) else None,
+    )
 
 
 @dataclass
@@ -135,14 +176,25 @@ class ThemeGuard:
     warnings: list[str] = field(default_factory=list)
     _snapshot: Any = None
     _snapshot_taken: bool = False
+    _lock: Any = None
+    lock_timed_out: bool = False
 
     @classmethod
     def for_capture(
         cls,
         addon_credential: EngineCredential | None,
         client: Any,
+        *,
+        armed: bool = True,
     ) -> ThemeGuard:
-        """Resolve the engine user's credential for one capture batch."""
+        """Resolve the engine user's credential for one capture batch.
+
+        ``armed=False`` yields an inert guard: Puppet only writes the theme
+        for renders that request one, so unthemed captures need no bracket
+        and stay free of any write (#1991).
+        """
+        if not armed:
+            return cls(credential=None)
         credential = addon_credential or _client_credential(client)
         if credential is None:
             logger.debug(
@@ -157,7 +209,11 @@ async def _session(self) -> AsyncIterator[HomeAssistantWebSocketClient]:
         from ..client.websocket_client import HomeAssistantWebSocketClient
 
         assert self.credential is not None
-        ws = HomeAssistantWebSocketClient(self.credential.url, self.credential.token)
+        ws = HomeAssistantWebSocketClient(
+            self.credential.url,
+            self.credential.token,
+            verify_ssl=self.credential.verify_ssl,
+        )
         if not await ws.connect():
             reason = ws.last_connect_error
             detail = f": {reason}" if isinstance(reason, str) else ""
@@ -173,15 +229,41 @@ async def _session(self) -> AsyncIterator[HomeAssistantWebSocketClient]:
     async def _fetch_theme(ws: HomeAssistantWebSocketClient) -> Any:
         """Read the persisted ``theme`` frontend user-data value (may be None)."""
         response = await ws.send_command(
-            "frontend/get_user_data", key=THEME_USER_DATA_KEY
+            "frontend/get_user_data",
+            key=THEME_USER_DATA_KEY,
+            _wait_timeout=COMMAND_TIMEOUT_SECONDS,
         )
         payload = response.get("result") if isinstance(response, dict) else None
         return payload.get("value") if isinstance(payload, dict) else None
 
-    async def take_snapshot(self) -> None:
-        """Record the saved theme before the engine renders. Never raises."""
+    async def take_snapshot(self, *, lock_timeout: float | None = None) -> None:
+        """Record the saved theme before the engine renders. Never raises.
+
+        Holds the per-engine-user lock until :meth:`restore` releases it, so
+        two overlapping batches cannot interleave snapshot and restore. When
+        the lock cannot be acquired within ``lock_timeout`` this sets
+        :attr:`lock_timed_out` and takes no snapshot; the caller must then
+        abandon the render rather than proceed unserialized, since an
+        unserialized themed batch is precisely the interleave the lock exists
+        to prevent.
+        """
         if self.credential is None:
             return
+        # Acquired before the read so the whole snapshot->render->restore
+        # window is exclusive for this engine user.
+        lock = _engine_lock(self.credential)
+        timeout = LOCK_WAIT_SECONDS if lock_timeout is None else lock_timeout
+        try:
+            await asyncio.wait_for(lock.acquire(), timeout=timeout)
+            self._lock = lock
+        except TimeoutError:
+            logger.warning(
+                "Timed out after %ss waiting for another screenshot batch's "
+                "theme bracket for this engine user",
+                timeout,
+            )
+            self.lock_timed_out = True
+            return
         try:
             async with self._session() as ws:
                 self._snapshot = await self._fetch_theme(ws)
@@ -196,10 +278,26 @@ async def take_snapshot(self) -> None:
                 "Could not read the screenshot engine user's saved frontend "
                 f"theme before rendering; if the render changed it, {_RESTORE_HINT}."
             )
+        finally:
+            if not self._snapshot_taken:
+                # Nothing to restore, so never hold the lock across the
+                # render. This lives in ``finally`` rather than the ``except``
+                # because asyncio.CancelledError is a BaseException: a
+                # cancellation mid-session would otherwise strand the lock and
+                # time out every later themed capture.
+                self._release_lock()
+
+    def _release_lock(self) -> None:
+        """Drop the per-engine lock if this guard holds it."""
+        lock = self._lock
+        self._lock = None
+        if lock is not None and lock.locked():
+            lock.release()
 
     async def restore(self) -> None:
         """Write the snapshot back if the render changed it. Never raises."""
         if not self._snapshot_taken or self.credential is None:
+            self._release_lock()
             return
         try:
             # Puppet's settheme dispatch happens during page navigation, but
@@ -220,6 +318,7 @@ async def restore(self) -> None:
                         "frontend/set_user_data",
                         key=THEME_USER_DATA_KEY,
                         value=restore_value,
+                        _wait_timeout=COMMAND_TIMEOUT_SECONDS,
                     )
         except Exception as exc:
             logger.warning(
@@ -232,3 +331,5 @@ async def restore(self) -> None:
                 "theme of the engine token's user and restoring it failed; "
                 f"{_RESTORE_HINT}."
             )
+        finally:
+            self._release_lock()