fix: address Gemini review feedback

- Use create_error_response/ErrorCode for all REST endpoint errors
  (settings save, restart, validation)
- Validate states dict from client (string keys, allowed state values)
  before persisting to disk
- Type hint settings parameter as Settings (TYPE_CHECKING import)
- Initialize _user_pinned_tools in __init__ instead of getattr fallback
- Add clearer docstring explaining why _list_tools() (private API)
  is used: public list_tools() filters disabled tools, and the
  settings UI specifically needs the unfiltered list

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

Author: kingpanther13

src/ha_mcp/server.py

@@ -70,6 +70,8 @@ def __init__(
         self._device_tools: Any = None
         self._tools_registry: ToolsRegistry | None = None
         self._skill_tool_names: list[str] = []
+        # Populated by _apply_settings_ui from tool_config.json on startup
+        self._user_pinned_tools: list[str] = []
 
         # Get server name/version from settings if no client provided
         if not self._client_provided:
@@ -442,7 +444,7 @@ def _apply_tool_search(self) -> None:
 
         # Build the always_visible list: defaults + user-configured pins
         pinned = list(self._PINNED_TOOLS)
-        pinned.extend(getattr(self, "_user_pinned_tools", []))
+        pinned.extend(self._user_pinned_tools)
 
         # Pin ResourcesAsTools and skill guidance tools if skills-as-tools is enabled
         if self.settings.enable_skills_as_tools:

src/ha_mcp/settings_ui.py

@@ -19,13 +19,17 @@
 from starlette.requests import Request
 from starlette.responses import HTMLResponse, JSONResponse
 
+from .errors import ErrorCode, create_error_response
 from .transforms import DEFAULT_PINNED_TOOLS
 
 if TYPE_CHECKING:
     from fastmcp import FastMCP
 
+    from .config import Settings
     from .server import HomeAssistantSmartMCPServer
 
+_VALID_STATES = frozenset({"enabled", "disabled", "pinned"})
+
 logger = logging.getLogger(__name__)
 
 MANDATORY_TOOLS: set[str] = {
@@ -88,7 +92,7 @@ def _get_config_path() -> Path:
     return home_dir / "tool_config.json"
 
 
-def load_tool_config(settings: Any = None) -> dict[str, Any]:
+def load_tool_config(settings: "Settings | None" = None) -> dict[str, Any]:
     """Load persisted tool config, seeding from env vars if no file exists."""
     path = _get_config_path()
     if path.exists():
@@ -134,12 +138,15 @@ def save_tool_config(config: dict[str, Any]) -> None:
         logger.exception("Failed to save tool config to %s", path)
 
 
-async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[str, Any]]:
+async def _get_tool_metadata(server: "HomeAssistantSmartMCPServer") -> list[dict[str, Any]]:
     """Extract metadata for all registered tools from the server.
 
-    Reads from the local provider's unfiltered tool list so that disabled
-    tools are still shown in the settings UI (users need to be able to
-    re-enable them).
+    Uses FastMCP's internal ``local_provider._list_tools()`` because the
+    public ``mcp.list_tools()`` filters out tools marked as disabled via
+    ``mcp.disable()``. The settings UI specifically needs the UNFILTERED
+    list so that users can see and re-enable tools they previously
+    disabled. There is no public FastMCP API that returns the unfiltered
+    list as of v3.2.0.
     """
     tools: list[dict[str, Any]] = []
     # Groups not considered "primary" when choosing a tool's canonical group —
@@ -195,9 +202,9 @@ async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[s
 
 
 def apply_tool_visibility(
-    mcp: FastMCP,
+    mcp: "FastMCP",
     config: dict[str, Any],
-    settings: Any,
+    settings: "Settings",
 ) -> set[str]:
     """Apply tool visibility from config, respecting safety toggles.
 
@@ -648,11 +655,32 @@ async def _save_tools(request: Request) -> JSONResponse:
             body = await request.json()
         except (ValueError, TypeError):
             return JSONResponse(
-                {"success": False, "error": {"code": "VALIDATION_ERROR", "message": "Invalid JSON body"}},
+                create_error_response(
+                    ErrorCode.VALIDATION_INVALID_JSON,
+                    "Invalid JSON body",
+                    suggestions=["Ensure the request body is valid JSON"],
+                ),
+                status_code=400,
+            )
+
+        raw_states = body.get("states", {})
+        if not isinstance(raw_states, dict):
+            return JSONResponse(
+                create_error_response(
+                    ErrorCode.VALIDATION_INVALID_PARAMETER,
+                    "'states' must be an object mapping tool names to state values",
+                ),
                 status_code=400,
             )
+        # Validate: keys must be strings, values must be one of the valid states
+        states: dict[str, str] = {}
+        for name, state in raw_states.items():
+            if not isinstance(name, str) or not isinstance(state, str):
+                continue
+            if state not in _VALID_STATES:
+                continue
+            states[name] = state
 
-        states = body.get("states", {})
         config = load_tool_config()
         config["tools"] = states
         save_tool_config(config)
@@ -676,7 +704,11 @@ async def _restart_addon(_: Request) -> JSONResponse:
         token = os.environ.get("SUPERVISOR_TOKEN")
         if not token:
             return JSONResponse(
-                {"success": False, "error": {"code": "NOT_IN_ADDON", "message": "Restart only available in add-on mode"}},
+                create_error_response(
+                    ErrorCode.CONFIG_VALIDATION_FAILED,
+                    "Restart only available when running as an add-on",
+                    details="SUPERVISOR_TOKEN environment variable is not set",
+                ),
                 status_code=400,
             )
         # Short timeout — the supervisor kills our process during restart so
@@ -694,21 +726,21 @@ async def _restart_addon(_: Request) -> JSONResponse:
         except httpx.HTTPError as e:
             logger.exception("Failed to reach Supervisor for restart")
             return JSONResponse(
-                {"success": False, "error": {"code": "SUPERVISOR_UNREACHABLE", "message": str(e)}},
+                create_error_response(
+                    ErrorCode.CONNECTION_FAILED,
+                    f"Failed to reach Supervisor: {e}",
+                ),
                 status_code=502,
             )
 
         if resp.status_code >= 400:
             body = resp.text
             logger.error("Supervisor restart failed: %d %s", resp.status_code, body)
             return JSONResponse(
-                {
-                    "success": False,
-                    "error": {
-                        "code": "SUPERVISOR_ERROR",
-                        "message": f"Supervisor returned {resp.status_code}: {body[:500]}",
-                    },
-                },
+                create_error_response(
+                    ErrorCode.INTERNAL_ERROR,
+                    f"Supervisor returned {resp.status_code}: {body[:500]}",
+                ),
                 status_code=502,
             )
         return JSONResponse({"success": True, "message": "Restart initiated"})