fix(code-mode): address round-2 review findings

Implements the 4 real bugs, 3 doc fixes, and 3 test gaps identified by
the second-pass review of the persistence + blocklist work.

Bugs
----

* ``_API_POST_BLOCKED_PREFIXES``: removed the ``config/scene/config/``
  entry. The error message it returned told the LLM to use
  ``ha_config_set_scene``, which does not exist in the registered tool
  catalogue (no scene-related ``set`` tool exists at all). Blocking the
  REST path without a validated alternative was net-negative — it just
  removed capability. The block can come back when a wrapping tool
  lands; comment in the code records that decision.

* ``_save_saved_tools`` now returns ``bool`` and the persistence
  failure path is no longer silent. The save_as branch in
  ``ha_manage_custom_tool`` rolls back the in-memory cache on a False
  return and surfaces a ``save_warning`` field in the response, with
  ``saved_as`` reset to ``None``. ``_delete_saved_tool`` also rolls
  back on failure and returns ``{"error": ...}`` instead of the prior
  misleading ``{"deleted": True}``. Persistence was the documented
  contract of this PR, so the prior "log at WARNING and lie with
  success: True" behaviour was a real reliability gap, not just
  cosmetic.

* ``_load_saved_tools`` distinguishes ``FileNotFoundError`` (legitimate
  "starting empty") from other ``OSError``s (genuine I/O failure on
  an existing file). When the latter fires, a new module-level
  ``_saved_tools_load_failed`` flag suppresses subsequent persistence
  for the session — preventing a transient ``PermissionError`` at
  startup from cascading into "next save wipes out the unreadable
  file with empty content" data loss. The flag is cleared on the
  next successful load.

* The audit-log line now uses ``sorted(map(str, data.keys()))`` instead
  of ``sorted(data.keys())``. Monty allows mixed-type dict keys, and
  the previous form would raise ``TypeError`` on the first
  ``api_post("/foo", {1: "x", "a": "y"})`` invocation, propagating
  through the audit-log step into the catch-all ``except Exception``
  and surfacing as a generic "api_post failed" with no hint that the
  audit-log step was the real culprit.

Schema-version contract honoured
--------------------------------

``_load_saved_tools`` now actually reads ``data.get("version")`` and
refuses to interpret anything that isn't ``_SAVED_TOOLS_SCHEMA_VERSION``
(currently 1). The prior code wrote the version field but never
checked it, so a future v2 file would have been silently downgraded
to v1 semantics by current code. Mismatch sets the load-failed flag
so we don't atomically replace the unfamiliar file with our v1
shape.

Docs
----

* ``docs/beta.md`` audit-log paragraph now shows the correct
  ``configuration.yaml`` / ``logger.logs.<name>: debug`` snippet
  instead of the previous bogus "set ``logger
  ha_mcp.tools.tools_code: debug`` in your add-on configuration."
  (HA's logger integration lives in configuration.yaml, not addon
  options.)
* The "blocked endpoints" paragraph in beta.md was updated to match
  the now-shorter ``_API_POST_BLOCKED_PREFIXES`` and explicitly notes
  the scene exception.
* The stale ``ha_config_set_scene`` reference in the
  ``ha_config_set_yaml`` Known Limitations section (preexisting) was
  also removed since I was in the file.
* The ``_MAX_SAVED_TOOLS`` constant comment now mentions both load-
  and save-time enforcement; the ``_SAVED_TOOLS_SCHEMA_VERSION``
  comment matches reality (the load path actually consults it).
* The ``_API_POST_BLOCKED_PREFIXES`` block comment splits the
  conflated rationale into two flavours (no-legitimate-use-case vs
  has-wrapping-tool).

Tests
-----

* ``TestSaveSavedTools.test_returns_true_on_success`` and
  ``test_returns_true_when_path_unset`` pin the new bool contract.
* ``TestSchemaVersionGuard`` (3 tests) covers refusing unknown
  version, missing version field, and not overwriting an unfamiliar
  file.
* ``TestLoadFailedFlag`` (2 tests) covers flag-clear-on-success and
  save-skipped-when-set.
* ``TestHydrationRoundTrip`` (2 tests) covers the load→modify→save→
  reload lifecycle, which the original suite was missing.
* ``TestSaveCapEnforcement`` (2 tests) covers the load-side cap and
  pins that ``_save_saved_tools`` itself does not self-cap (the
  registration-site code is the upper-bound guard).

24 unit tests now pass (was 13). Lint/mypy/ast-grep clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: kingpanther13

docs/beta.md

@@ -55,7 +55,7 @@ This tool edits `configuration.yaml` and package files directly, bypassing Home
 
 **Recommended prerequisites:**
 - Comfort with editing `configuration.yaml` via SSH or File Editor when things go wrong
-- Understanding that dedicated tools (`ha_config_set_helper`, `ha_config_set_automation`, `ha_config_set_script`, `ha_config_set_scene`, etc.) should be preferred for anything they support
+- Understanding that dedicated tools (`ha_config_set_helper`, `ha_config_set_automation`, `ha_config_set_script`, etc.) should be preferred for anything they support
 
 ### `ha_list_files`, `ha_read_file`, `ha_write_file`, `ha_delete_file`
 
@@ -85,13 +85,25 @@ This tool exposes a sandboxed Python interpreter (`pydantic-monty`) to the AI as
 
 **Safer-path enforcement on REST and WebSocket.** Several endpoints have wrapping MCP tools that perform validation, lint, hash-locking, or invariant checks; raw `api_post` / `ws_send` would skip those. The sandbox blocks a small denylist on each surface:
 
-- `api_post`: writes to `/api/states/<entity_id>` (which can conjure ghost entities), `/api/events/<HA-internal-event-name>` (Core internal events that can fan out into user automations), and `/api/config/{automation,script,scene}/config/*` (forced through `ha_config_set_automation` / `ha_config_set_script` / `ha_config_set_scene`).
+- `api_post`: writes to `/api/states/<entity_id>` (which can conjure ghost entities), `/api/events/<HA-internal-event-name>` (Core internal events that can fan out into user automations), and `/api/config/{automation,script}/config/*` (forced through `ha_config_set_automation` / `ha_config_set_script`). `config/scene/config/*` is intentionally not blocked because no `ha_config_set_scene` wrapping tool exists yet — the block would just remove capability with no validated alternative path.
 - `ws_send`: `config/core/update` (rewrites HA's location/timezone/currency in `.storage/core.config`), `lovelace/config/save` and `lovelace/dashboards/{create,delete,update}` (forced through `ha_config_set_dashboard`), and `config/{area,device,entity}_registry/{delete,disable,update}` (forced through `ha_config_set_area` / `ha_update_device` / `ha_set_entity` etc.).
 - Service calls (`POST /api/services/<domain>/<service>`), webhook firing (`POST /api/webhook/<id>`), custom event types (`POST /api/events/my_event_name`), and registry **read** queries (e.g. `config/area_registry/list`) all stay allowed.
 
 **Sandbox failures are classified.** When sandboxed code raises, the error response now uses one of three codes — `SANDBOX_LIMIT_EXCEEDED` (memory / time / recursion / invocation cap), `SANDBOX_SYNTAX_UNSUPPORTED` (imports, classes, `with`, `match`, hard syntax errors) or `SANDBOX_RUNTIME_ERROR` (everything else) — with suggestions tailored to the category. Previously every Monty failure surfaced as `INTERNAL_ERROR` with "check the Python code for syntax errors" advice, which actively misled callers when the real cause was a memory cap or a missing module import.
 
-**Sandbox actions are auditable.** Every state-changing sandbox call (`POST /api/...`, every `ws_send`) logs a structured `sandbox.api_post`/`sandbox.ws_send` line at DEBUG level. Blocked attempts log at INFO level. To get a forensic trail, set `logger ha_mcp.tools.tools_code: debug` (or equivalent) in your add-on configuration.
+**Sandbox actions are auditable.** Every state-changing sandbox call (`POST /api/...`, every `ws_send`) logs a structured `sandbox.api_post` / `sandbox.ws_send` line at DEBUG level. Blocked attempts (e.g. a refused `POST /api/states/...` or a refused `config/core/update`) log a `sandbox.api_post.blocked` / `sandbox.ws_send.blocked` line at INFO level so they're visible in default operator logs.
+
+To get a full forensic trail of allowed calls, escalate the `ha_mcp.tools.tools_code` logger to DEBUG. This is HA's [`logger:` integration](https://www.home-assistant.io/integrations/logger/) and goes in **`configuration.yaml`** (not the add-on options):
+
+```yaml
+# configuration.yaml
+logger:
+  default: warning
+  logs:
+    ha_mcp.tools.tools_code: debug
+```
+
+Reload the `Logger` integration (or restart HA) to apply.
 
 **ARM platforms require the async sandbox path.** On systems where `Monty.run_async` is unavailable, the tool fails fast with a clear error rather than falling back silently.
 

src/ha_mcp/tools/tools_code.py

@@ -52,11 +52,21 @@
 # Validation for save_as names
 _SAVE_NAME_PATTERN = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]{0,63}$")
 
-# Path-prefix denylist for ``_api_post``. These endpoints either have no
-# legitimate sandbox use case or have a wrapping MCP tool that performs
-# validation/lint/hash-locking that raw ``api_post`` would skip. The
-# prefixes are matched after ``_normalize_endpoint`` strips the leading
-# ``api/`` so they are written as plain HA-relative paths.
+# Path-prefix denylist for ``_api_post``. Two flavours of entry, kept in
+# one list because the matching logic is identical:
+#   1. Endpoints with no legitimate sandbox use case at all — currently
+#      just ``states/`` (raw state writes can conjure ghost entities and
+#      override real ones in the in-memory state machine).
+#   2. Endpoints whose corresponding wrapping MCP tool performs
+#      validation / lint / hash-locking that raw ``api_post`` would skip
+#      — currently ``config/{automation,script}/config/``.
+# Scene config writes (``config/scene/config/*``) are intentionally NOT
+# in this list: there is no ``ha_config_set_scene`` tool to redirect to,
+# and blocking the path without offering a substitute would just remove
+# capability with no validated alternative. Add the block back when a
+# wrapping tool lands.
+# The prefixes are matched after ``_normalize_endpoint`` strips the
+# leading ``api/`` so they are written as plain HA-relative paths.
 _API_POST_BLOCKED_PREFIXES: tuple[tuple[str, str, str], ...] = (
     (
         "states/",
@@ -75,11 +85,6 @@
         "Direct writes to /api/config/script/config/*",
         "use call_tool('ha_config_set_script', ...)",
     ),
-    (
-        "config/scene/config/",
-        "Direct writes to /api/config/scene/config/*",
-        "use call_tool('ha_config_set_scene', ...)",
-    ),
 )
 
 # HA Core internal events. Firing one of these via POST /api/events/<name>
@@ -266,23 +271,46 @@ def _check_api_post_blocked(normalized: str) -> str | None:
             )
     return None
 
-# Cap on the number of saved tools to prevent runaway growth (a buggy LLM
-# loop could otherwise fill disk with unique save_as names).
+# Cap on the number of saved tools to prevent runaway growth. A buggy
+# LLM loop could otherwise fill the on-disk file with unique save_as
+# names. Enforced both at load (truncate-with-warning) and at save
+# (reject the call before mutating the in-memory cache).
 _MAX_SAVED_TOOLS = 256
 
-# Schema version for the on-disk saved-tools file. Bump when the shape
-# changes so _load_saved_tools can migrate or refuse old files cleanly.
+# Schema version for the on-disk saved-tools file. Bumped when the shape
+# changes so _load_saved_tools can refuse old/new files cleanly. The
+# load path checks data["version"] explicitly and refuses anything that
+# isn't this number rather than silently re-interpreting it as v1.
 _SAVED_TOOLS_SCHEMA_VERSION = 1
 
+# Module-level flag set when _load_saved_tools fails for a reason other
+# than "the file doesn't exist yet" (e.g. PermissionError reading an
+# existing file). Persistence is suppressed while this is set so we
+# don't atomically replace a temporarily-unreadable file with empty
+# content and destroy whatever was on disk. Cleared by a successful
+# load at register_code_tools time. The variable is module-level
+# (not closure-captured) so save sites in ha_manage_custom_tool /
+# _delete_saved_tool can read it without parameter plumbing.
+_saved_tools_load_failed = False
+
 
 def _load_saved_tools(path_str: str) -> dict[str, dict[str, str]]:
     """Load saved tools from a JSON file, filtering malformed entries.
 
-    Returns an empty dict if the path is unset, the file doesn't exist
-    yet, or the contents are unreadable / unparseable. A corrupt file is
-    logged at WARNING but does not raise — the user can still save new
-    tools and the bad file will be overwritten on the next persist.
+    Returns an empty dict if the path is unset or the file doesn't exist
+    yet (legitimate "starting empty" cases). A corrupt JSON body or an
+    unexpected schema version is logged at WARNING and returns empty —
+    the file will be overwritten on the next persist.
+
+    A genuine I/O error reading an existing file (OSError that isn't
+    FileNotFoundError) is logged at ERROR and ALSO sets the module-level
+    ``_saved_tools_load_failed`` flag so callers know not to overwrite
+    whatever is on disk while the load condition persists. This prevents
+    a PermissionError at startup from cascading into "next save wipes
+    out the unreadable file" data loss.
     """
+    global _saved_tools_load_failed
+    _saved_tools_load_failed = False
     if not path_str:
         return {}
     path = Path(path_str)
@@ -291,10 +319,31 @@ def _load_saved_tools(path_str: str) -> dict[str, dict[str, str]]:
         return {}
     try:
         raw = path.read_text(encoding="utf-8")
+    except FileNotFoundError:
+        # Race: file disappeared between exists() and read_text().
+        # Treat as legitimate "not yet" rather than an I/O failure.
+        return {}
+    except OSError as exc:
+        # PermissionError / IsADirectoryError / etc. The file exists but
+        # we can't read it. Block subsequent persistence so we don't
+        # overwrite the unreadable original with empty content.
+        logger.error(
+            "Cannot read saved-tools file %s (%s); persistence will be "
+            "suppressed until the load condition clears. Saves and "
+            "deletes will still update the in-memory cache for the "
+            "current session.",
+            path,
+            exc,
+            exc_info=True,
+        )
+        _saved_tools_load_failed = True
+        return {}
+    try:
         data = json.loads(raw)
-    except (OSError, json.JSONDecodeError) as exc:
+    except json.JSONDecodeError as exc:
         logger.warning(
-            "Failed to load saved tools from %s (%s); starting empty",
+            "Saved-tools file %s is not valid JSON (%s); starting empty. "
+            "The corrupt file will be overwritten on the next persist.",
             path,
             exc,
         )
@@ -308,6 +357,25 @@ def _load_saved_tools(path_str: str) -> dict[str, dict[str, str]]:
         )
         return {}
 
+    file_version = data.get("version")
+    if file_version != _SAVED_TOOLS_SCHEMA_VERSION:
+        # Refuse to interpret the file. We don't know whether this is a
+        # newer file produced by a future ha-mcp version (which might
+        # have shape changes we'd silently mangle) or an older file we
+        # don't have a migration for. Setting the failed flag means the
+        # current session won't overwrite it on next save.
+        logger.error(
+            "Saved-tools file %s has schema version %r; this build expects %d. "
+            "Refusing to load. Persistence is suppressed for this session "
+            "to avoid overwriting an unfamiliar file. Move or delete the "
+            "file to recover.",
+            path,
+            file_version,
+            _SAVED_TOOLS_SCHEMA_VERSION,
+        )
+        _saved_tools_load_failed = True
+        return {}
+
     tools_raw = data.get("saved_tools", {})
     if not isinstance(tools_raw, dict):
         logger.warning(
@@ -353,17 +421,32 @@ def _load_saved_tools(path_str: str) -> dict[str, dict[str, str]]:
     return valid
 
 
-def _save_saved_tools(path_str: str, tools: dict[str, dict[str, str]]) -> None:
+def _save_saved_tools(
+    path_str: str, tools: dict[str, dict[str, str]]
+) -> bool:
     """Persist the saved-tools cache to a JSON file atomically.
 
-    Writes to ``path.tmp`` first and uses ``os.replace`` to swap it in,
-    so a crash mid-write cannot corrupt the existing file. Failures are
-    logged at WARNING — a write failure does not raise into the sandbox
-    or the MCP client because the in-memory cache still holds the new
-    entry; persistence is best-effort.
+    Returns ``True`` if persistence succeeded (or was disabled because
+    ``path_str`` is empty — that's the configured-out case, not a
+    failure). Returns ``False`` only when persistence was attempted and
+    the underlying I/O raised. Callers that promised the user durability
+    should surface a ``False`` return as a warning in the response.
+
+    Writes to ``<dir>/.<name>.<rand>.tmp`` first and uses ``os.replace``
+    to swap it in, so a crash mid-write cannot corrupt the existing
+    file. Refuses to write at all when ``_saved_tools_load_failed`` is
+    set — see _load_saved_tools for why we'd rather skip persistence
+    than overwrite an unreadable file with empty content.
     """
     if not path_str:
-        return
+        return True
+    if _saved_tools_load_failed:
+        logger.warning(
+            "Skipping persist to %s because the prior load failed; "
+            "saves and deletes are in-memory only for this session.",
+            path_str,
+        )
+        return False
     path = Path(path_str)
     payload = {
         "version": _SAVED_TOOLS_SCHEMA_VERSION,
@@ -387,11 +470,16 @@ def _save_saved_tools(path_str: str, tools: dict[str, dict[str, str]]) -> None:
             tmp_path = Path(tmp.name)
         tmp_path.replace(path)
     except OSError as exc:
-        logger.warning(
-            "Failed to persist saved tools to %s (%s); cache remains in memory",
+        logger.error(
+            "Failed to persist saved tools to %s (%s); the in-memory "
+            "cache holds the latest change but it will be lost on restart "
+            "unless this resolves before the next save",
             path,
             exc,
+            exc_info=True,
         )
+        return False
+    return True
 
 
 def _extract_tool_result(result: Any) -> Any:
@@ -560,10 +648,15 @@ async def _api_post(endpoint: str, data: dict[str, Any] | None = None) -> Any:
         # State-changing call: DEBUG-level audit trail. Operators can
         # bump the ha_mcp.tools.tools_code logger to DEBUG to see what
         # the sandbox is actually doing on their HA instance.
+        # ``map(str, ...)`` on the keys because Monty allows mixed-type
+        # dict keys (e.g. ``{1: "x", "a": "y"}``); a plain ``sorted``
+        # would raise TypeError on the first invocation and the user
+        # would see a confusing "api_post failed" with no hint that
+        # the audit-log step was the real culprit.
         logger.debug(
             "sandbox.api_post endpoint=%r data_keys=%s",
             endpoint,
-            sorted(data.keys()) if isinstance(data, dict) else None,
+            sorted(map(str, data.keys())) if isinstance(data, dict) else None,
         )
         try:
             post_kwargs: dict[str, Any] = {}
@@ -670,9 +763,11 @@ def _delete_saved_tool(name: Any) -> dict[str, Any]:
         """Remove a previously saved custom tool by name.
 
         Sandbox helper. Returns ``{"deleted": True, "name": name}`` on
-        success, ``{"error": "..."}`` on validation failure or if the
-        named tool does not exist. Persists the change immediately when
-        the saved-tools file path is configured.
+        success, ``{"error": "..."}`` on validation failure, missing
+        name, or persistence failure. When persistence is configured
+        and the on-disk write fails, the in-memory deletion is rolled
+        back so the next save_as / list_saved doesn't show a different
+        view than the next process restart.
         """
         if not isinstance(name, str):
             return {"error": "delete_saved_tool(name) requires a string name"}
@@ -685,9 +780,24 @@ def _delete_saved_tool(name: Any) -> dict[str, Any]:
             }
         if name not in _saved_tools:
             return {"error": f"No saved tool named {name!r}"}
+        # Snapshot the entry before deleting so we can restore it on
+        # persist failure (otherwise the in-memory cache and disk would
+        # disagree, and the on-restart hydration would resurrect the
+        # entry the LLM already saw "deleted").
+        previous = _saved_tools[name]
         del _saved_tools[name]
+        if not _save_saved_tools(
+            settings.code_mode_saved_tools_path, _saved_tools
+        ):
+            _saved_tools[name] = previous
+            return {
+                "error": (
+                    f"Deleted {name!r} from in-memory cache but the "
+                    "persistence write failed; rolled back. Check "
+                    "operator logs for the underlying I/O error."
+                )
+            }
         logger.info("Deleted saved custom tool '%s'", name)
-        _save_saved_tools(settings.code_mode_saved_tools_path, _saved_tools)
         return {"deleted": True, "name": name}
 
     m = Monty(code, script_name="ha_manage_custom_tool.py")
@@ -1021,12 +1131,32 @@ async def ha_manage_custom_tool(
                         ],
                     )
                 )
+            previous = _saved_tools.get(save_as)
             _saved_tools[save_as] = {
                 "code": code,
                 "justification": justification,
             }
             response["data"]["saved_as"] = save_as
             logger.info("Saved custom tool as '%s'", save_as)
-            _save_saved_tools(settings.code_mode_saved_tools_path, _saved_tools)
+            persisted = _save_saved_tools(
+                settings.code_mode_saved_tools_path, _saved_tools
+            )
+            if not persisted:
+                # Roll back the in-memory write so the cache matches
+                # what's on disk (or, on next restart, what's loaded).
+                # Surface a warning in the response so the LLM knows
+                # the save_as didn't actually durable, while still
+                # returning success=True for the code execution itself.
+                if previous is None:
+                    _saved_tools.pop(save_as, None)
+                else:
+                    _saved_tools[save_as] = previous
+                response["data"]["saved_as"] = None
+                response["data"]["save_warning"] = (
+                    f"save_as={save_as!r} was attempted but the persistence "
+                    "write failed; the entry was rolled back from the "
+                    "in-memory cache. Check operator logs for the "
+                    "underlying I/O error."
+                )
 
         return response