fix(code-mode): address Patch76 round-3 review (H1, M2, M3, L1-L6) + clean uv.lock

Rebased on current upstream/master (was 8 commits behind) and
regenerated uv.lock so it adds only ``pydantic-monty==0.0.9`` —
the prior lockfile carried 8 unrelated package downgrades that the
fresh resolver pass clears (M1).

H1 — entity_registry/remove + device_registry/remove_config_entry
-----------------------------------------------------------------

The blocklist was using ``config/entity_registry/delete`` and
``config/device_registry/delete`` — neither is a registered HA Core
WS command. Verified ha-mcp itself emits the actually-registered
names: ``tools_entities.py:1130`` uses ``config/entity_registry/remove``
and ``tools_registry.py:753`` uses
``config/device_registry/remove_config_entry``. A sandbox
``ws_send({"type": "config/entity_registry/remove", ...})`` was
slipping past the blocklist and bypassing ``ha_remove_entity``'s
wrapping checks.

Added both real names to ``_BLOCKED_WS_COMMANDS``. Left the dead
``*_registry/delete`` strings as-is — over-blocking inert names is
harmless, under-blocking real ones is the bug.

M2 — percent-encoded ``..`` traversal
-------------------------------------

The segment-loop check at ``_normalize_endpoint`` rejects literal
``..`` segments but treats ``%2e%2e`` as a normal segment. httpx
itself doesn't decode at the transport level, but reverse proxies
(nginx with config drift, traefik with custom routers) sometimes
decode-then-resolve, which would let ``%2e%2e/auth/providers``
escape ``/api/`` server-side. Each segment is now passed through
``urllib.parse.unquote`` before the ``..`` comparison so encoded
forms can't slip past.

M3 — naked list returns from ``_extract_tool_result``
-----------------------------------------------------

The basic-types tuple at line 566 didn't include ``list``, so a
tool returning a plain ``[{"id": 1}, ...]`` fell into the
ToolResult-extraction branch, found no ``.text`` on its dict
elements, and got string-repr'd. Sandbox code ended up with
``"[{'id': 1}, ...]"`` instead of an iterable list.

Lists are now passed through when they don't look like FastMCP's
content-block shape (heuristic: first element has ``.text`` or
``.type`` — the two attributes content blocks always carry). Plain
data lists pass straight to the sandbox.

L1 — sandbox-side error shape contract documented
--------------------------------------------------

Patch76 noted that sandbox code sees two error shapes
(``{"error": str}`` from the bridge helpers, ``{"success": False,
"error": {"code", "message"}}`` from ``_sandbox_error``) and
suggested standardizing on the structured shape. Standardizing
that direction would have changed ~15 existing tests that do
``result.get("error", "").lower()`` blindly. The simpler
``{"error": str}`` shape is what most helpers and tests already
assume, and the structured form has real value for ``call_tool``
(propagates the underlying tool's ``ErrorCode``).

Resolved instead by making the contract explicit in the
``_run_sandboxed_code`` docstring: both shapes are documented,
consumers should always probe with ``if "error" in result:``
before indexing. Same UX outcome (consumers know what to
expect) without churning the tests.

L2 — switched ``%r`` for the justification log line
---------------------------------------------------

Removed the ``_log_safe`` helper and inlined ``%r`` (repr())
for the LLM-supplied ``justification`` log line, matching the
``%r`` already used for endpoint/type fields in the audit-log
lines. ``%r`` escapes ``\r`` / ``\n`` / ``\t`` as literal
``\r`` / ``\n`` / ``\t`` sequences in formatted output — same
log-injection prevention as ``_log_safe`` but consistent with
the rest of the file.

L4 / L5 / L6 — clarifying comments
----------------------------------

* L4: Note next to the ``_saved_tools_load_failed`` read in
  ``_save_saved_tools`` that the read is intentionally without
  ``global`` — Python doesn't require it for read-only access.
* L5: One-line clarification in ``_normalize_endpoint``'s
  docstring that ``@`` later in the path (``events/foo@bar``)
  is acceptable — only userinfo position (before the first
  ``/``) is the credential-leaking shape.
* L6: Block comment on the ``_saved_tools`` module-level
  declaration explaining *why* it's at module level
  (cross-call persistence is the documented contract;
  per-request scope wouldn't allow ``run_saved`` to see prior
  ``save_as`` writes), and pointing at
  ``code_mode_saved_tools_path`` as the persistence boundary.

Test expansion — full ``_BLOCKED_WS_COMMANDS`` parametrize
----------------------------------------------------------

``test_ws_send_blocks_command`` now parametrizes over all 25
entries in ``_BLOCKED_WS_COMMANDS`` (was: 3 hand-picked cases).
The "blocklist names a command HA Core doesn't accept" class
of bug surfaces in CI now: if the blocklist drops an entry,
the corresponding parametrize row fails; if HA Core renames a
command, the test fails with the now-stale name still in the
parametrize list and a maintainer notices.

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

Author: kingpanther13

src/ha_mcp/tools/tools_code.py

@@ -25,6 +25,7 @@
 import logging
 import re
 import tempfile
+import urllib.parse
 from datetime import UTC, datetime
 from pathlib import Path
 from typing import Any
@@ -39,34 +40,22 @@
 logger = logging.getLogger(__name__)
 
 # In-memory cache for saved custom tools, optionally persisted to disk.
+#
+# Lives at module level deliberately: ``ha_manage_custom_tool`` is a
+# stateless MCP tool that needs ``run_saved`` / ``list_saved`` to see
+# entries written by earlier ``save_as`` calls in the same process.
+# Hydrated from ``settings.code_mode_saved_tools_path`` on
+# ``register_code_tools`` startup (one-time) and persisted on every
+# subsequent ``save_as`` / ``delete_saved_tool``. Per-call request
+# scope wouldn't work because ``run_saved`` would never see prior
+# saves; ``code_mode_saved_tools_path`` is the documented persistence
+# boundary.
+#
 # WARNING: This is shared across all clients in the same server process.
 # In multi-user modes (OAuth, HTTP), one user's saved tools are visible to
 # all other users. Scope to per-session/user before multi-user support.
-# When ``settings.code_mode_saved_tools_path`` is set, the cache is hydrated
-# from disk on registration and persisted on every save_as / delete.
 _saved_tools: dict[str, dict[str, str]] = {}
 
-# Translation table for ``_log_safe`` — replaces CR / LF / TAB with a single
-# space so an LLM-controlled string interpolated into a log line via ``%s``
-# can't manufacture extra log records.
-_LOG_CONTROL_CHARS_MAP = str.maketrans({"\r": " ", "\n": " ", "\t": " "})
-
-
-def _log_safe(value: Any, max_len: int = 200) -> str:
-    """Return ``value`` flattened to a single line, safe for ``%s`` log interpolation.
-
-    Replaces ``\\r`` / ``\\n`` / ``\\t`` with spaces and truncates to
-    ``max_len`` characters. Used on user-controlled strings (``justification``,
-    saved-tool ``name``, etc.) before they reach ``logger.info(..., %s, ...)``
-    so a crafted input like ``"real reason\\nFAKE_CRITICAL: thing crashed"``
-    cannot inject a second log line.
-    """
-    text = str(value)
-    if len(text) > max_len:
-        text = text[:max_len]
-    return text.translate(_LOG_CONTROL_CHARS_MAP)
-
-
 # Tools that sandbox code must not call. Includes ``ha_manage_custom_tool``
 # itself (prevents recursive self-invocation) plus the four synthetics that
 # the categorized-search transform exposes when ``ENABLE_TOOL_SEARCH=true``
@@ -179,9 +168,18 @@ def _log_safe(value: Any, max_len: int = 200) -> str:
     "config/device_registry/delete",
     "config/device_registry/disable",
     "config/device_registry/update",
+    # Device registry deletion is registered as ``remove_config_entry`` on
+    # HA Core, not ``delete`` — see ``tools_registry.py:753`` for the
+    # actually-emitted command. ``ha_remove_device`` wraps it; raw
+    # ``ws_send`` would skip those checks.
+    "config/device_registry/remove_config_entry",
     "config/entity_registry/delete",
     "config/entity_registry/disable",
     "config/entity_registry/update",
+    # Entity registry deletion is registered as ``remove`` on HA Core,
+    # not ``delete`` — see ``tools_entities.py:1130`` for the
+    # actually-emitted command. ``ha_remove_entity`` wraps it.
+    "config/entity_registry/remove",
     # Floor / label / category registries follow the same rationale as
     # area / device / entity above: each has a wrapping MCP tool
     # (``ha_config_set_floor``, ``ha_config_set_label``,
@@ -497,6 +495,8 @@ def _save_saved_tools(
     """
     if not path_str:
         return True
+    # Read-only access to the module-level flag; ``global`` declaration
+    # only needed at the set sites in ``_load_saved_tools``.
     if _saved_tools_load_failed:
         logger.warning(
             "Skipping persist to %s because the prior load failed; "
@@ -556,6 +556,18 @@ def _extract_tool_result(result: Any) -> Any:
     if isinstance(result, (str, int, float, bool, type(None), dict)):
         return result
 
+    # ``list`` is also a basic type Monty handles, but a list might also
+    # be FastMCP's "list of content objects" shape (each element having
+    # a ``.text`` or being a content-block ``dict``). Distinguish: if the
+    # first element looks like a content object, treat as a ToolResult
+    # payload; otherwise pass through as a normal list of basic values.
+    if isinstance(result, list):
+        looks_like_content = bool(result) and (
+            hasattr(result[0], "text") or hasattr(result[0], "type")
+        )
+        if not looks_like_content:
+            return result
+
     # ToolResult or similar: extract content list
     content = None
     if hasattr(result, "content"):
@@ -615,6 +627,18 @@ async def _run_sandboxed_code(
     - api_post(endpoint, data) — POST request to HA REST API
     - ws_send(message) — send a HA WebSocket command and return its result
     - call_tool(name, args) — call a registered MCP tool (for existing tools)
+
+    **Error shape contract.** All bridge functions return a dict with an
+    ``"error"`` key on failure; the value may be either a plain string
+    (``api_get`` / ``api_post`` / ``ws_send`` / ``delete_saved_tool`` —
+    transport-level failures, validation rejections) or a structured
+    sub-dict ``{"code": "<ErrorCode>", "message": "<text>"}`` from
+    ``_sandbox_error`` (``call_tool`` — propagates the underlying tool's
+    ``ErrorCode`` so sandbox code can branch on category). The structured
+    form also includes ``"success": False`` at the top level. **Consumers
+    should always probe with ``if "error" in result:``** — never
+    ``result["error"].lower()`` blindly, because the value isn't always a
+    string.
     """
     call_count = 0
 
@@ -641,13 +665,21 @@ def _normalize_endpoint(endpoint: Any) -> str:
           or ``@`` before the first ``/`` (userinfo). Without this httpx
           will dispatch the request to the absolute host *with the HA
           bearer token still attached*, leaking credentials.
+
+          Note: ``@`` later in the path (``events/foo@bar``) is fine —
+          only userinfo position (before the first ``/``) is the
+          credential-leaking shape that http URL parsers will treat as
+          ``user@host``.
         * ``..`` path segments — httpx happily resolves
           ``base_url='http://ha:8123/api'`` + endpoint ``'../auth/providers'``
           to ``http://ha:8123/auth/providers``, escaping the ``/api/``
           prefix entirely. HA exposes other bearer-authenticated routes
           at root (``/auth/...``, ``/profile``, etc.) — every one of
           those becomes reachable from the sandbox unless we reject
-          ``..`` here.
+          ``..`` here. Each segment is also percent-decoded once before
+          the comparison so ``%2e%2e`` (and other percent-encoded
+          variants of ``..``) can't slip past on reverse-proxy setups
+          that decode-then-resolve.
         """
         if not isinstance(endpoint, str):
             raise ValueError("endpoint must be a string path (e.g. '/states')")
@@ -666,12 +698,14 @@ def _normalize_endpoint(endpoint: Any) -> str:
             ep = ep[4:]
         # ``..`` segments would let the sandbox escape the ``/api/`` prefix
         # via httpx URL resolution. Check after stripping so the comparison
-        # is against actual path segments, and return the same error for
-        # leading-, mid-, or trailing-position cases.
+        # is against actual path segments, and percent-decode each segment
+        # so encoded forms (``%2e%2e`` and similar) don't slip past on
+        # reverse-proxy setups that decode-then-resolve.
         for segment in ep.split("/"):
-            if segment == "..":
+            if urllib.parse.unquote(segment) == "..":
                 raise ValueError(
-                    "endpoint must not contain '..' path segments; "
+                    "endpoint must not contain '..' path segments "
+                    "(including percent-encoded forms like %2e%2e); "
                     "the sandbox is restricted to /api/ routes"
                 )
         return ep
@@ -1169,9 +1203,15 @@ async def ha_manage_custom_tool(
                 )
             )
 
+        # ``%r`` (repr) defends against log-line injection by escaping
+        # ``\r`` / ``\n`` / ``\t`` as literal ``\r``/``\n``/``\t``
+        # sequences in the formatted output — same primitive used by
+        # the audit-log endpoint/type fields below. Truncate to keep
+        # log lines bounded; an LLM can supply a multi-KB
+        # justification.
         logger.info(
-            "ha_manage_custom_tool invoked — justification: %s",
-            _log_safe(justification),
+            "ha_manage_custom_tool invoked — justification: %r",
+            justification[:200],
         )
         # Code is logged at DEBUG and the multi-line shape is intentional
         # (the LLM-authored snippet is the operator's primary forensic

tests/src/e2e/tools/test_create_custom_tool.py

@@ -1410,39 +1410,74 @@ async def test_ws_send_blocks_lovelace_config_save(
         )
         logger.info("ws_send correctly blocked lovelace/config/save")
 
-    async def test_ws_send_blocks_registry_mutations(
-        self, mcp_client_with_code_mode
+    @pytest.mark.parametrize(
+        "ws_type",
+        [
+            # Each member of _BLOCKED_WS_COMMANDS must be rejected by
+            # ws_send. Listed explicitly so a regression that drops an
+            # entry from the frozenset surfaces as a missing parametrize
+            # row in CI rather than passing silently.
+            "config/core/update",
+            "lovelace/config/save",
+            "lovelace/dashboards/create",
+            "lovelace/dashboards/delete",
+            "lovelace/dashboards/update",
+            "config/area_registry/delete",
+            "config/area_registry/disable",
+            "config/area_registry/update",
+            "config/device_registry/delete",
+            "config/device_registry/disable",
+            "config/device_registry/update",
+            "config/device_registry/remove_config_entry",
+            "config/entity_registry/delete",
+            "config/entity_registry/disable",
+            "config/entity_registry/update",
+            "config/entity_registry/remove",
+            "config/floor_registry/create",
+            "config/floor_registry/delete",
+            "config/floor_registry/update",
+            "config/label_registry/create",
+            "config/label_registry/delete",
+            "config/label_registry/update",
+            "config/category_registry/create",
+            "config/category_registry/delete",
+            "config/category_registry/update",
+        ],
+    )
+    async def test_ws_send_blocks_command(
+        self, mcp_client_with_code_mode, ws_type
     ):
-        """Registry mutation commands are blocked — must go through their
-        wrapping tools (ha_config_set_area, ha_update_device, ha_set_entity).
+        """Every entry in _BLOCKED_WS_COMMANDS must be rejected by ws_send.
+
+        Parametrizing over the full set catches the "blocklist names a
+        command HA Core doesn't actually accept" class of bug — if a
+        future refactor drops an entry, the corresponding row fails;
+        if HA Core renames a command and we forget to update the
+        blocklist, the test fails with the old name still in the
+        parametrize list.
         """
         check = await _check_tool_available(mcp_client_with_code_mode)
-        _skip_if_unavailable(check, "ws_send registry mutation blocklist")
+        _skip_if_unavailable(check, f"ws_send blocks {ws_type}")
 
-        for ws_type in (
-            "config/area_registry/update",
-            "config/device_registry/update",
-            "config/entity_registry/update",
-        ):
-            code = (
-                f'result = await ws_send({{"type": "{ws_type}", "id": "x"}})\n'
-                '{"has_error": "error" in result if isinstance(result, dict) else False,'
-                ' "error": result.get("error", "") if isinstance(result, dict) else ""}'
-            )
-            data = await safe_call_tool(
-                mcp_client_with_code_mode,
-                TOOL_NAME,
-                {"code": code, "justification": f"E2E test: blocked {ws_type}"},
-            )
-            assert data.get("success") is True, f"Sandbox should succeed: {data}"
-            result = data["data"]["result"]
-            assert result["has_error"] is True, (
-                f"ws_send must block {ws_type}: {data}"
-            )
-            assert ws_type in result["error"], (
-                f"Error should mention the blocked command: {result}"
-            )
-            logger.info("ws_send correctly blocked %s", ws_type)
+        code = (
+            f'result = await ws_send({{"type": "{ws_type}", "id": "x"}})\n'
+            '{"has_error": "error" in result if isinstance(result, dict) else False,'
+            ' "error": result.get("error", "") if isinstance(result, dict) else ""}'
+        )
+        data = await safe_call_tool(
+            mcp_client_with_code_mode,
+            TOOL_NAME,
+            {"code": code, "justification": f"E2E test: blocked {ws_type}"},
+        )
+        assert data.get("success") is True, f"Sandbox should succeed: {data}"
+        result = data["data"]["result"]
+        assert result["has_error"] is True, (
+            f"ws_send must block {ws_type}: {data}"
+        )
+        assert ws_type in result["error"], (
+            f"Error should mention the blocked command: {result}"
+        )
+        logger.info("ws_send correctly blocked %s", ws_type)
 
     async def test_ws_send_allows_registry_list(self, mcp_client_with_code_mode):
         """Registry LIST queries stay allowed — only mutations are blocked."""