refactor(c901): fix backup subsystem (3 files) below C901 threshold, remove from grandfather list (#1937)

* refactor(c901): backup subsystem below C901 threshold, remove from grandfather list

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DxGQhYBQg7681uCdr9w9qB

* fix(c901-b2): drop now-unreachable terminal return, allowlist NoReturn mixed-returns FPs

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DxGQhYBQg7681uCdr9w9qB

---------

Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: kingpanther13

pyproject.toml

@@ -174,9 +174,6 @@ ignore = [
 "homeassistant-addon-webhook-proxy/mcp_proxy/__init__.py" = ["C901"]
 "homeassistant-addon-webhook-proxy/start.py" = ["C901"]
 "scripts/extract_tools.py" = ["C901"]
-"src/ha_mcp/backup_manager.py" = ["C901"]
-"src/ha_mcp/tools/auto_backup.py" = ["C901"]
-"src/ha_mcp/tools/backup.py" = ["C901"]
 "src/ha_mcp/tools/tools_config_automations.py" = ["C901"]
 "src/ha_mcp/tools/tools_config_scenes.py" = ["C901"]
 "src/ha_mcp/tools/tools_config_scripts.py" = ["C901"]

scripts/codeql_quality_gate.py

@@ -163,6 +163,20 @@
         "heuristic to have correctly caught. mypy already confirms this file is "
         "clean under that typing.",
     ),
+    # NOTE: same PATH-WIDE caveat as the server.py entry above — re-audit if
+    # backup.py grows genuinely mixed-returns functions.
+    (
+        "py/mixed-returns",
+        "src/ha_mcp/tools/backup.py",
+        "Mixing implicit and explicit returns",
+        "False positive on _finalize_backup_timeout and _perform_restore's "
+        "success-branch-vs-raise shape (helpers extracted from the C901 "
+        "refactor, issue #925): the non-return branch in each always goes "
+        "through raise_tool_error, typed '-> NoReturn' (see helpers.py), so "
+        "there is no implicit None return for CodeQL's heuristic to have "
+        "correctly caught. mypy already confirms this file is clean under "
+        "that typing.",
+    ),
     # ---- Security-suite entries (the workflow also gates the default
     # <language>-code-scanning suites; see codeql-quality.yml header). Entries
     # whose message substring is generic ("as clear text", "hashing algorithm")

src/ha_mcp/backup_manager.py

@@ -195,6 +195,13 @@ def _resolve_default_dir() -> Path:
     return (Path.home() / ".local" / "share" / "ha_mcp" / "backups").resolve()
 
 
+# Sentinel returned by ``BackupManager._fetch_config_for_snapshot`` when there
+# is nothing to snapshot — a handled transient fetch failure (already logged),
+# or a None config for an entity that did not exist. Distinct from any real
+# config value so the caller can tell "skip" from a genuine payload.
+_SNAPSHOT_SKIP: Any = object()
+
+
 class BackupManager:
     """Per-entity snapshot manager. One instance per server, cached on client."""
 
@@ -317,6 +324,48 @@ async def maybe_snapshot(
         apply (force can't conjure a snapshot for an entity that
         doesn't exist or has no registered handler).
         """
+        handler = self._resolve_snapshot_handler(
+            domain, entity_id, force=force, mandatory=mandatory
+        )
+        if handler is None:
+            return None
+
+        key = f"{domain}:{entity_id}"
+        lock = self._locks.setdefault(key, asyncio.Lock())
+        async with lock:
+            now = time.monotonic()
+            throttle = self.throttle_seconds
+            # Skip throttle if no prior snapshot exists for this key.
+            # Using ``get(key, 0.0)`` would falsely block the first capture
+            # whenever ``monotonic()`` < throttle (typical on a fresh process
+            # in CI), since 0.0 would be treated as "last snapshot at
+            # monotonic time 0".
+            if (
+                not force
+                and throttle
+                and key in self._last_snapshot
+                and (now - self._last_snapshot[key]) < throttle
+            ):
+                return None
+            config = await self._fetch_config_for_snapshot(
+                handler, entity_id, key, mandatory=mandatory
+            )
+            if config is _SNAPSHOT_SKIP:
+                return None
+            return await self._write_and_rotate(
+                domain, entity_id, key, config, tool_name, now, mandatory=mandatory
+            )
+
+    def _resolve_snapshot_handler(
+        self, domain: str, entity_id: str, *, force: bool, mandatory: bool
+    ) -> DomainHandler | None:
+        """Run the pre-capture guards; return the handler or None to skip.
+
+        Raises ``MandatoryBackupError`` for the fail-closed cases (an unusable
+        backup dir, an unregistered domain) under ``mandatory``. A None return
+        is a legitimate skip (feature disabled, no entity id, no handler) — the
+        caller returns None and lets the wrapped write proceed.
+        """
         if self._init_dir_error is not None:
             if mandatory:
                 raise MandatoryBackupError(
@@ -344,88 +393,101 @@ async def maybe_snapshot(
                 domain,
             )
             return None
+        return handler
 
-        key = f"{domain}:{entity_id}"
-        lock = self._locks.setdefault(key, asyncio.Lock())
-        async with lock:
-            now = time.monotonic()
-            throttle = self.throttle_seconds
-            # Skip throttle if no prior snapshot exists for this key.
-            # Using ``get(key, 0.0)`` would falsely block the first capture
-            # whenever ``monotonic()`` < throttle (typical on a fresh process
-            # in CI), since 0.0 would be treated as "last snapshot at
-            # monotonic time 0".
-            if (
-                not force
-                and throttle
-                and key in self._last_snapshot
-                and (now - self._last_snapshot[key]) < throttle
-            ):
-                return None
-            try:
-                config = await handler.fetch(self._client, entity_id)
-            except _CAPTURE_TRANSIENT_ERRORS as err:
-                # Degraded fetches (a non-list WS envelope from an
-                # auth-scope change or API drift) raise rather than return
-                # None — see ``_require_list``. During auto-backup we skip
-                # the snapshot with a WARNING (operator-visible) instead of
-                # crashing the pipeline; the same error during a diff/
-                # restore propagates to the tool layer as a structured
-                # error. The warning level (vs the debug log below) is what
-                # distinguishes "fetch broke" from "entity didn't exist".
-                if mandatory:
-                    raise MandatoryBackupError(
-                        f"could not read the current state of {key} to back "
-                        f"it up: {type(err).__name__}: {err}"
-                    ) from err
-                logger.warning(
-                    "Auto-backup: fetch failed for %s — %s: %s",
-                    key,
-                    type(err).__name__,
-                    err,
-                )
-                return None
-            if config is None:
-                # Entity didn't exist at fetch time (create operation, or
-                # already-deleted at remove time before our pre-fetch).
-                logger.debug(
-                    "Auto-backup: fetch returned None for %s — skipping snapshot",
-                    key,
-                )
-                return None
-            try:
-                path = await asyncio.to_thread(
-                    self._write_snapshot, domain, entity_id, config, tool_name
-                )
-            except (OSError, yaml.YAMLError) as err:
-                if mandatory:
-                    raise MandatoryBackupError(
-                        f"could not write the pre-write snapshot for {key}: "
-                        f"{type(err).__name__}: {err}",
-                        suggestions=[
-                            "Free up disk space, or delete old snapshots via "
-                            "ha_manage_backup(scope='edits', action='delete')",
-                        ],
-                    ) from err
-                logger.warning(
-                    "Auto-backup: write failed for %s — %s: %s",
-                    key,
-                    type(err).__name__,
-                    err,
-                )
-                return None
-            self._last_snapshot[key] = now
-            self._maybe_prune_trackers()
-            try:
-                await asyncio.to_thread(self._rotate, domain, entity_id)
-            except OSError as err:
-                logger.warning(
-                    "Auto-backup: rotation failed for %s — %s: %s",
-                    key,
-                    type(err).__name__,
-                    err,
-                )
-            return path
+    async def _fetch_config_for_snapshot(
+        self, handler: DomainHandler, entity_id: str, key: str, *, mandatory: bool
+    ) -> Any:
+        """Fetch the pre-write config; return ``_SNAPSHOT_SKIP`` to skip capture.
+
+        A handled transient fetch failure logs a WARNING and returns the
+        sentinel (or raises ``MandatoryBackupError`` under ``mandatory``); a
+        fetch that returns None because the entity did not exist logs a DEBUG
+        and returns the sentinel. Any other value is the config to snapshot.
+        """
+        try:
+            config = await handler.fetch(self._client, entity_id)
+        except _CAPTURE_TRANSIENT_ERRORS as err:
+            # Degraded fetches (a non-list WS envelope from an
+            # auth-scope change or API drift) raise rather than return
+            # None — see ``_require_list``. During auto-backup we skip
+            # the snapshot with a WARNING (operator-visible) instead of
+            # crashing the pipeline; the same error during a diff/
+            # restore propagates to the tool layer as a structured
+            # error. The warning level (vs the debug log below) is what
+            # distinguishes "fetch broke" from "entity didn't exist".
+            if mandatory:
+                raise MandatoryBackupError(
+                    f"could not read the current state of {key} to back "
+                    f"it up: {type(err).__name__}: {err}"
+                ) from err
+            logger.warning(
+                "Auto-backup: fetch failed for %s — %s: %s",
+                key,
+                type(err).__name__,
+                err,
+            )
+            return _SNAPSHOT_SKIP
+        if config is None:
+            # Entity didn't exist at fetch time (create operation, or
+            # already-deleted at remove time before our pre-fetch).
+            logger.debug(
+                "Auto-backup: fetch returned None for %s — skipping snapshot",
+                key,
+            )
+            return _SNAPSHOT_SKIP
+        return config
+
+    async def _write_and_rotate(
+        self,
+        domain: str,
+        entity_id: str,
+        key: str,
+        config: Any,
+        tool_name: str | None,
+        now: float,
+        *,
+        mandatory: bool,
+    ) -> Path | None:
+        """Write the snapshot then rotate old files; return the written Path.
+
+        Returns None on a handled (non-mandatory) write failure. Raises
+        ``MandatoryBackupError`` under ``mandatory`` when the write genuinely
+        fails (e.g. disk-full).
+        """
+        try:
+            path = await asyncio.to_thread(
+                self._write_snapshot, domain, entity_id, config, tool_name
+            )
+        except (OSError, yaml.YAMLError) as err:
+            if mandatory:
+                raise MandatoryBackupError(
+                    f"could not write the pre-write snapshot for {key}: "
+                    f"{type(err).__name__}: {err}",
+                    suggestions=[
+                        "Free up disk space, or delete old snapshots via "
+                        "ha_manage_backup(scope='edits', action='delete')",
+                    ],
+                ) from err
+            logger.warning(
+                "Auto-backup: write failed for %s — %s: %s",
+                key,
+                type(err).__name__,
+                err,
+            )
+            return None
+        self._last_snapshot[key] = now
+        self._maybe_prune_trackers()
+        try:
+            await asyncio.to_thread(self._rotate, domain, entity_id)
+        except OSError as err:
+            logger.warning(
+                "Auto-backup: rotation failed for %s — %s: %s",
+                key,
+                type(err).__name__,
+                err,
+            )
+        return path
 
     def _maybe_prune_trackers(self) -> None:
         """Cap per-entity tracker growth.
@@ -1024,44 +1086,11 @@ def _diff_node(
     if type(stored) is type(current):
         if isinstance(stored, dict):
             assert isinstance(current, dict)
-            for key in stored:
-                seg = _pointer_segment(str(key))
-                sub_path = f"{path}/{seg}"
-                if key not in current:
-                    out.append({"op": "add", "path": sub_path, "value": stored[key]})
-                    if len(out) >= max_ops:
-                        return
-                else:
-                    _diff_node(stored[key], current[key], sub_path, out, max_ops)
-                    if len(out) >= max_ops:
-                        return
-            for key in current:
-                if key not in stored:
-                    seg = _pointer_segment(str(key))
-                    out.append({"op": "remove", "path": f"{path}/{seg}"})
-                    if len(out) >= max_ops:
-                        return
+            _diff_dict_node(stored, current, path, out, max_ops)
             return
         if isinstance(stored, list):
             assert isinstance(current, list)
-            min_len = min(len(stored), len(current))
-            for i in range(min_len):
-                _diff_node(stored[i], current[i], f"{path}/{i}", out, max_ops)
-                if len(out) >= max_ops:
-                    return
-            if len(stored) > len(current):
-                for value in stored[len(current) :]:
-                    out.append({"op": "add", "path": f"{path}/-", "value": value})
-                    if len(out) >= max_ops:
-                        return
-            elif len(current) > len(stored):
-                # Remove tail entries from highest to lowest index so
-                # successive removes stay valid (RFC 6902 reindexes
-                # after each op).
-                for i in range(len(current) - 1, len(stored) - 1, -1):
-                    out.append({"op": "remove", "path": f"{path}/{i}"})
-                    if len(out) >= max_ops:
-                        return
+            _diff_list_node(stored, current, path, out, max_ops)
             return
         if stored != current:
             out.append({"op": "replace", "path": path or "", "value": stored})
@@ -1076,6 +1105,61 @@ def _diff_node(
     out.append({"op": "replace", "path": path or "", "value": stored})
 
 
+def _diff_dict_node(
+    stored: dict[Any, Any],
+    current: dict[Any, Any],
+    path: str,
+    out: list[dict[str, Any]],
+    max_ops: int,
+) -> None:
+    """Diff two dicts into JSON-Patch ops (add/remove/recurse per key)."""
+    for key in stored:
+        seg = _pointer_segment(str(key))
+        sub_path = f"{path}/{seg}"
+        if key not in current:
+            out.append({"op": "add", "path": sub_path, "value": stored[key]})
+            if len(out) >= max_ops:
+                return
+        else:
+            _diff_node(stored[key], current[key], sub_path, out, max_ops)
+            if len(out) >= max_ops:
+                return
+    for key in current:
+        if key not in stored:
+            seg = _pointer_segment(str(key))
+            out.append({"op": "remove", "path": f"{path}/{seg}"})
+            if len(out) >= max_ops:
+                return
+
+
+def _diff_list_node(
+    stored: list[Any],
+    current: list[Any],
+    path: str,
+    out: list[dict[str, Any]],
+    max_ops: int,
+) -> None:
+    """Diff two lists into JSON-Patch ops (positional recurse, then tail add/remove)."""
+    min_len = min(len(stored), len(current))
+    for i in range(min_len):
+        _diff_node(stored[i], current[i], f"{path}/{i}", out, max_ops)
+        if len(out) >= max_ops:
+            return
+    if len(stored) > len(current):
+        for value in stored[len(current) :]:
+            out.append({"op": "add", "path": f"{path}/-", "value": value})
+            if len(out) >= max_ops:
+                return
+    elif len(current) > len(stored):
+        # Remove tail entries from highest to lowest index so
+        # successive removes stay valid (RFC 6902 reindexes
+        # after each op).
+        for i in range(len(current) - 1, len(stored) - 1, -1):
+            out.append({"op": "remove", "path": f"{path}/{i}"})
+            if len(out) >= max_ops:
+                return
+
+
 def _pointer_segment(key: str) -> str:
     """Escape one JSON-Pointer reference token per RFC 6901 §3.