fix: enable e2e filesystem tests and fix ha_mcp_tools integration (#868)

Three root causes prevented filesystem e2e tests from running:

1. ha_mcp_tools component was never installed in the Docker test
   container. Unified the existing _install_mcp_proxy_component into
   a generic _install_custom_component helper and used it to install
   ha_mcp_tools alongside mcp_proxy.

2. async_add_executor_job does not pass kwargs — mkdir calls with
   parents=True, exist_ok=True were silently called without arguments,
   causing 500 errors. Fixed with lambda wrappers (3 occurrences).

3. HA's call_service(return_response=True) wraps results in
   {"changed_states": [], "service_response": {...}} but
   parse_mcp_result returned this as-is. Tests couldn't find
   the success/error fields. Added _unwrap_service_response to
   extract the inner dict.

Also:
- Removed add_timezone_metadata from filesystem/yaml_config tools
  (timezone info is irrelevant for file operations)
- Fixed tests that used call_tool_success for expected-failure cases
  (should use safe_call_tool which handles ToolError)
- Removed dead data.get("data", {}).get("success") branch and
  unsafe eval() fallback in assert_mcp_success / parse_mcp_result
- Added tests/AGENTS.md with e2e test infrastructure notes

Results: 22 file_operations + 18 yaml_config e2e tests now pass
(previously all skipped). 901 unit tests pass. Full e2e suite:
525 passed, 3 failed (pre-existing HACS), 11 skipped.

Author: sergeykad

custom_components/ha_mcp_tools/__init__.py

@@ -402,7 +402,7 @@ async def handle_write_file(call: ServiceCall) -> ServiceResponse:
             # Create parent directories if needed
             if create_dirs:
                 await hass.async_add_executor_job(
-                    target_file.parent.mkdir, parents=True, exist_ok=True
+                    lambda: target_file.parent.mkdir(parents=True, exist_ok=True)
                 )
 
             # Check parent directory exists
@@ -597,7 +597,7 @@ async def handle_edit_yaml_config(call: ServiceCall) -> ServiceResponse:
             if do_backup and raw_content:
                 backup_dir = config_dir / "www" / "yaml_backups"
                 await hass.async_add_executor_job(
-                    backup_dir.mkdir, parents=True, exist_ok=True
+                    lambda: backup_dir.mkdir(parents=True, exist_ok=True)
                 )
                 timestamp = datetime.now().strftime("%Y%m%d_%H%M%S")
                 safe_name = normalized.replace(os.sep, "_")
@@ -661,7 +661,7 @@ async def handle_edit_yaml_config(call: ServiceCall) -> ServiceResponse:
             # Create parent directories if needed (for new package files)
             if not target_file.parent.exists():
                 await hass.async_add_executor_job(
-                    target_file.parent.mkdir, parents=True, exist_ok=True
+                    lambda: target_file.parent.mkdir(parents=True, exist_ok=True)
                 )
 
             # Atomic write: write to temp file, then rename into place

src/ha_mcp/tools/tools_filesystem.py

@@ -13,6 +13,7 @@
 Feature Flag: Set HAMCP_ENABLE_FILESYSTEM_TOOLS=true to enable these tools.
 """
 
+import json
 import logging
 import os
 from typing import Annotated, Any
@@ -22,7 +23,7 @@
 
 from ..errors import ErrorCode, create_error_response
 from .helpers import exception_to_structured_error, log_tool_usage, raise_tool_error
-from .util_helpers import add_timezone_metadata, coerce_bool_param, coerce_int_param
+from .util_helpers import coerce_bool_param, coerce_int_param, unwrap_service_response
 
 logger = logging.getLogger(__name__)
 
@@ -174,18 +175,14 @@ async def ha_list_files(
 
             # The service returns the response directly
             if isinstance(result, dict):
-                return await add_timezone_metadata(client, result)
-
-            return await add_timezone_metadata(
-                client,
-                {
-                    "success": True,
-                    "path": path,
-                    "pattern": pattern,
-                    "files": [],
-                    "count": 0,
-                    "note": "Unexpected response format from service",
-                },
+                return unwrap_service_response(result)
+
+            raise_tool_error(
+                create_error_response(
+                    ErrorCode.SERVICE_CALL_FAILED,
+                    "Unexpected response format from list_files service",
+                    context={"path": path},
+                )
             )
 
         except ToolError:
@@ -289,15 +286,14 @@ async def ha_read_file(
             )
 
             if isinstance(result, dict):
-                return await add_timezone_metadata(client, result)
-
-            return await add_timezone_metadata(
-                client,
-                {
-                    "success": False,
-                    "error": "Unexpected response format from service",
-                    "path": path,
-                },
+                return unwrap_service_response(result)
+
+            raise_tool_error(
+                create_error_response(
+                    ErrorCode.SERVICE_CALL_FAILED,
+                    "Unexpected response format from read_file service",
+                    context={"path": path},
+                )
             )
 
         except ToolError:
@@ -419,15 +415,14 @@ async def ha_write_file(
             )
 
             if isinstance(result, dict):
-                return await add_timezone_metadata(client, result)
-
-            return await add_timezone_metadata(
-                client,
-                {
-                    "success": False,
-                    "error": "Unexpected response format from service",
-                    "path": path,
-                },
+                return unwrap_service_response(result)
+
+            raise_tool_error(
+                create_error_response(
+                    ErrorCode.SERVICE_CALL_FAILED,
+                    "Unexpected response format from write_file service",
+                    context={"path": path},
+                )
             )
 
         except ToolError:
@@ -503,20 +498,15 @@ async def ha_delete_file(
             confirm_bool = coerce_bool_param(confirm, "confirm", default=False)
 
             if not confirm_bool:
-                return await add_timezone_metadata(
-                    client,
-                    {
-                        "success": False,
-                        "error": "Deletion not confirmed",
-                        "message": (
-                            "You must set confirm=True to delete a file. "
-                            "This is a safety measure to prevent accidental deletions."
-                        ),
-                        "path": path,
-                        "suggestions": [
-                            f"Call ha_delete_file(path='{path}', confirm=True) to proceed",
+                raise_tool_error(
+                    create_error_response(
+                        ErrorCode.VALIDATION_INVALID_PARAMETER,
+                        "Deletion not confirmed. Set confirm=True to delete a file.",
+                        suggestions=[
+                            f"Call ha_delete_file(path={json.dumps(path)}, confirm=True) to proceed",
                         ],
-                    },
+                        context={"path": path},
+                    )
                 )
 
             # Check if custom component is available
@@ -534,15 +524,14 @@ async def ha_delete_file(
             )
 
             if isinstance(result, dict):
-                return await add_timezone_metadata(client, result)
-
-            return await add_timezone_metadata(
-                client,
-                {
-                    "success": False,
-                    "error": "Unexpected response format from service",
-                    "path": path,
-                },
+                return unwrap_service_response(result)
+
+            raise_tool_error(
+                create_error_response(
+                    ErrorCode.SERVICE_CALL_FAILED,
+                    "Unexpected response format from delete_file service",
+                    context={"path": path},
+                )
             )
 
         except ToolError:

src/ha_mcp/tools/tools_yaml_config.py

@@ -24,7 +24,7 @@
     MCP_TOOLS_DOMAIN,
     _assert_mcp_tools_available,
 )
-from .util_helpers import add_timezone_metadata, coerce_bool_param
+from .util_helpers import coerce_bool_param, unwrap_service_response
 
 logger = logging.getLogger(__name__)
 
@@ -179,9 +179,10 @@ async def ha_config_set_yaml(
             )
 
             if isinstance(result, dict):
+                result = unwrap_service_response(result)
                 if not result.get("success", True):
                     raise_tool_error(result)
-                return await add_timezone_metadata(client, result)
+                return result
 
             raise_tool_error(
                 create_error_response(

src/ha_mcp/tools/util_helpers.py

@@ -232,6 +232,17 @@ def parse_string_list_param(
     raise ValueError(f"{param_name} must be string, list, or None")
 
 
+def unwrap_service_response(result: dict[str, Any]) -> dict[str, Any]:
+    """Extract service_response from HA call_service result.
+
+    HA's call_service with return_response wraps results in
+    {"changed_states": [...], "service_response": {...}}.
+    Returns service_response if present and is a dict, otherwise the original result.
+    """
+    sr = result.get("service_response")
+    return sr if isinstance(sr, dict) else result
+
+
 async def add_timezone_metadata(client: Any, data: dict[str, Any]) -> dict[str, Any]:
     """Add Home Assistant timezone to tool responses for local time context."""
     try:

tests/AGENTS.md

@@ -0,0 +1,15 @@
+# E2E Test Infrastructure
+
+## Custom Component (ha_mcp_tools)
+
+- Component is installed into the Docker container by `_install_custom_component` in `src/e2e/conftest.py`
+- HA's `call_service(return_response=True)` wraps results in `{"changed_states": [], "service_response": {...}}` — tools unwrap this with `result.get("service_response", result)` before returning
+- `hass.async_add_executor_job` only passes positional args — use `lambda:` wrappers for calls needing kwargs (e.g., `mkdir(parents=True, exist_ok=True)`)
+- HA Docker image uses `annotatedyaml` (PyYAML wrapper), NOT `ruamel.yaml` — custom components needing ruamel must declare it in `manifest.json` requirements
+- Feature flags (`ENABLE_YAML_CONFIG_EDITING`, `HAMCP_ENABLE_FILESYSTEM_TOOLS`) are set in `ha_container_with_fresh_config` fixture
+
+## Test Patterns
+
+- Tests expecting tool **success**: use `mcp.call_tool_success()` inside `MCPAssertions` context
+- Tests expecting tool **failure**: use `safe_call_tool()` directly (catches `ToolError`, returns parsed dict)
+- Service availability checks should use `safe_call_tool` to probe, not `call_tool_success`

tests/CLAUDE.md

@@ -0,0 +1 @@
+AGENTS.md

tests/src/e2e/conftest.py

@@ -125,63 +125,55 @@ def _ensure_hacs_frontend(initial_state_path: Path) -> None:
             logger.warning("HACS tests may be skipped without the frontend")
 
 
-def _install_mcp_proxy_component(config_path: Path) -> None:
-    """Dynamically install mcp_proxy from the webhook proxy addon source.
-
-    Copies the integration source, writes the test config file, and injects
-    a config entry into HA storage. This avoids duplicating source files in
-    initial_test_state and survives test environment rebuilds.
+def _install_custom_component(
+    config_path: Path,
+    component_src: Path,
+    domain: str,
+    title: str,
+) -> bool:
+    """Install a custom component into the test HA config.
+
+    Copies component source into custom_components/<domain> and injects a
+    config entry so HA loads it on startup. Returns True if installed.
     """
-    import json
-
-    repo_root = Path(__file__).parent.parent.parent.parent
-    addon_mcp_proxy = repo_root / "homeassistant-addon-webhook-proxy" / "mcp_proxy"
-
-    if not addon_mcp_proxy.exists():
-        logger.info("mcp_proxy addon source not found — skipping installation")
-        return
+    if not component_src.exists():
+        logger.info("%s source not found — skipping installation", domain)
+        return False
 
-    # Copy component source
-    dest = config_path / "custom_components" / "mcp_proxy"
+    dest = config_path / "custom_components" / domain
     dest.mkdir(parents=True, exist_ok=True)
-    shutil.copytree(addon_mcp_proxy, dest, dirs_exist_ok=True)
-
-    # Write config file (target_url points at HA's own API for testing)
-    proxy_config = {
-        "target_url": "http://localhost:8123/api/",
-        "webhook_id": "mcp_e2e_test_webhook_proxy",
-    }
-    (config_path / ".mcp_proxy_config.json").write_text(json.dumps(proxy_config))
+    shutil.copytree(component_src, dest, dirs_exist_ok=True)
 
     # Inject config entry if not already present
     storage_file = config_path / ".storage" / "core.config_entries"
     if storage_file.exists():
         data = json.loads(storage_file.read_text())
         entries = data.get("data", {}).get("entries", [])
-        if not any(e.get("domain") == "mcp_proxy" for e in entries):
+        if not any(e.get("domain") == domain for e in entries):
             entries.append(
                 {
                     "created_at": "2025-09-07T23:56:28.040744+00:00",
                     "data": {},
                     "disabled_by": None,
                     "discovery_keys": {},
-                    "domain": "mcp_proxy",
-                    "entry_id": "e2e_test_mcp_proxy_entry",
+                    "domain": domain,
+                    "entry_id": f"e2e_test_{domain}_entry",
                     "minor_version": 1,
                     "modified_at": "2025-09-07T23:56:28.040747+00:00",
                     "options": {},
                     "pref_disable_new_entities": False,
                     "pref_disable_polling": False,
                     "source": "import",
                     "subentries": [],
-                    "title": "MCP Webhook Proxy",
-                    "unique_id": "mcp_proxy",
+                    "title": title,
+                    "unique_id": domain,
                     "version": 1,
                 }
             )
             storage_file.write_text(json.dumps(data, indent=2))
 
-    logger.info("Installed mcp_proxy component from addon source")
+    logger.info("Installed %s component", domain)
+    return True
 
 
 @pytest.fixture(scope="session")
@@ -247,8 +239,28 @@ def ha_container_with_fresh_config():
                     break
             storage_file.write_text(json.dumps(ce_data, indent=2))
 
-    # Install mcp_proxy from addon source (avoids duplicating files in test state)
-    _install_mcp_proxy_component(config_path)
+    # Install custom components from repo source
+    repo_root = Path(__file__).parent.parent.parent.parent
+    if _install_custom_component(
+        config_path,
+        repo_root / "homeassistant-addon-webhook-proxy" / "mcp_proxy",
+        "mcp_proxy",
+        "MCP Webhook Proxy",
+    ):
+        # mcp_proxy needs a config file pointing at HA's own API
+        proxy_config = {
+            "target_url": "http://localhost:8123/api/",
+            "webhook_id": "mcp_e2e_test_webhook_proxy",
+        }
+        (config_path / ".mcp_proxy_config.json").write_text(
+            json.dumps(proxy_config)
+        )
+    _install_custom_component(
+        config_path,
+        repo_root / "custom_components" / "ha_mcp_tools",
+        "ha_mcp_tools",
+        "HA MCP Tools",
+    )
 
     # Ensure proper permissions for Home Assistant
     _setup_config_permissions(config_path)
@@ -295,6 +307,9 @@ def ha_container_with_fresh_config():
         # Set environment variables for the dynamic URL so WebSocket client uses correct port
         os.environ["HOMEASSISTANT_URL"] = base_url
         os.environ["HOMEASSISTANT_TOKEN"] = TEST_TOKEN
+        # Enable feature flags for e2e tests
+        os.environ["ENABLE_YAML_CONFIG_EDITING"] = "true"
+        os.environ["HAMCP_ENABLE_FILESYSTEM_TOOLS"] = "true"
 
         # Reset cached settings so WebSocket client picks up the dynamic URL
         import ha_mcp.config

tests/src/e2e/utilities/assertions.py

@@ -47,12 +47,10 @@ def parse_mcp_result(result) -> dict[str, Any]:
         if hasattr(result.content[0], "text"):
             response_text = result.content[0].text
             try:
-                return json.loads(response_text)
+                parsed = json.loads(response_text)
+                return parsed
             except json.JSONDecodeError:
-                try:
-                    return eval(response_text)
-                except Exception:
-                    return {"raw_response": response_text}
+                return {"raw_response": response_text}
         return {"content": str(result.content[0])}
     return {"error": "No content in result"}
 
@@ -112,10 +110,8 @@ def assert_mcp_success(result, operation_name: str = "operation"):
     data = parse_mcp_result(result)
 
     # Handle different success indicators
-    # Some tools return success in the top level, others in data.success
     success_indicators = [
         data.get("success") is True,
-        data.get("data", {}).get("success") is True,
         # If no explicit success field but has data and no error, consider success
         ("data" in data and data.get("error") is None and data.get("success") is None),
         # Bulk operations success: has operational data without explicit success field
@@ -135,8 +131,7 @@ def assert_mcp_success(result, operation_name: str = "operation"):
     ]
 
     if not any(success_indicators):
-        error_msg = data.get("error") or data.get("data", {}).get(
-            "error", "Unknown error"
+        error_msg = data.get("error", "Unknown error"
         )
         suggestions = data.get("suggestions", [])