From a07dbd229464b3724c311b4686019ec17728e911 Mon Sep 17 00:00:00 2001 From: Jacob Magar Date: Fri, 13 Mar 2026 15:23:12 -0400 Subject: [PATCH] fix: address PR review critical and high findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove duplicate _cap_log_content definition (dead code merge artifact) from manager.py; keep byte-count version that correctly handles multibyte UTF-8 - Fix storage.py unassigned handler reading wrong key (unassignedDevices → disks) — query already fetched `disks {}` but handler returned empty list every call - Add null checks to all 8 Docker organizer object mutations; raise ToolError instead of silently returning success=True with organizer=None - Raise ToolError in docker logs when server returns no log data - Extract notification object from create response (was returning raw GraphQL wrapper dict instead of the notification itself) - Raise ToolError in test_subscription_query on connection failure and unexpected exceptions (was returning error dicts, bypassing error handling) - Remove stale "Bug N fix" inline comments from diagnostics.py - Update docker.py module docstring to reflect 26 actions (was 15) - Bump version 0.4.1 → 0.4.2 Co-authored-by: Claude --- pyproject.toml | 2 +- unraid_mcp/subscriptions/diagnostics.py | 6 +- unraid_mcp/subscriptions/manager.py | 50 ------------- unraid_mcp/tools/docker.py | 97 ++++++++++++++----------- unraid_mcp/tools/notifications.py | 5 +- unraid_mcp/tools/storage.py | 2 +- 6 files changed, 61 insertions(+), 101 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index e2e2fd9..6bf9485 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -10,7 +10,7 @@ build-backend = "hatchling.build" # ============================================================================ [project] name = "unraid-mcp" -version = "0.4.1" +version = "0.4.2" description = "MCP Server for Unraid API - provides tools to interact with an Unraid server's GraphQL API" readme = "README.md" license = {file = "LICENSE"} diff --git a/unraid_mcp/subscriptions/diagnostics.py b/unraid_mcp/subscriptions/diagnostics.py index 4207aa4..da5c2e5 100644 --- a/unraid_mcp/subscriptions/diagnostics.py +++ b/unraid_mcp/subscriptions/diagnostics.py @@ -101,13 +101,11 @@ def register_diagnostic_tools(mcp: FastMCP) -> None: Returns: Dict containing test results and response data """ - # Validate before any network I/O (Bug 1 fix) field_name = _validate_subscription_query(subscription_query) try: logger.info(f"[TEST_SUBSCRIPTION] Testing validated subscription field '{field_name}'") - # Build WebSocket URL — raises ValueError on invalid/missing scheme (Bug 4 fix) try: ws_url = build_ws_url() except ValueError as e: @@ -139,7 +137,7 @@ def register_diagnostic_tools(mcp: FastMCP) -> None: init_response = json.loads(response) if init_response.get("type") != "connection_ack": - return {"error": f"Connection failed: {init_response}"} + raise ToolError(f"Connection failed: {init_response}") # Send subscription await websocket.send( @@ -168,7 +166,7 @@ def register_diagnostic_tools(mcp: FastMCP) -> None: raise except Exception as e: logger.error(f"[TEST_SUBSCRIPTION] Error: {e}", exc_info=True) - return {"error": str(e), "query_tested": subscription_query} + raise ToolError(f"Subscription test failed: {e!s}") from e @mcp.tool() async def diagnose_subscriptions() -> dict[str, Any]: diff --git a/unraid_mcp/subscriptions/manager.py b/unraid_mcp/subscriptions/manager.py index fef4309..cd749da 100644 --- a/unraid_mcp/subscriptions/manager.py +++ b/unraid_mcp/subscriptions/manager.py @@ -29,56 +29,6 @@ _MAX_RESOURCE_DATA_LINES = 5_000 _STABLE_CONNECTION_SECONDS = 30 -def _cap_log_content(data: dict[str, Any]) -> dict[str, Any]: - """Cap log content in subscription data to prevent unbounded memory growth. - - Returns a new dict — does NOT mutate the input. If any nested 'content' - field (from log subscriptions) exceeds the byte limit, truncate it to the - most recent _MAX_RESOURCE_DATA_LINES lines. - - The final content is guaranteed to be <= _MAX_RESOURCE_DATA_BYTES. - """ - result: dict[str, Any] = {} - for key, value in data.items(): - if isinstance(value, dict): - result[key] = _cap_log_content(value) - elif ( - key == "content" - and isinstance(value, str) - and len(value) > _MAX_RESOURCE_DATA_BYTES # fast pre-check on char count - ): - lines = value.splitlines() - original_line_count = len(lines) - - # Keep most recent lines first. - if len(lines) > _MAX_RESOURCE_DATA_LINES: - lines = lines[-_MAX_RESOURCE_DATA_LINES:] - - truncated = "\n".join(lines) - # Encode once and slice bytes instead of O(n²) line-trim loop - encoded = truncated.encode("utf-8", errors="replace") - if len(encoded) > _MAX_RESOURCE_DATA_BYTES: - truncated = encoded[-_MAX_RESOURCE_DATA_BYTES:].decode("utf-8", errors="ignore") - # Strip partial first line that may have been cut mid-character - nl_pos = truncated.find("\n") - if nl_pos != -1: - truncated = truncated[nl_pos + 1 :] - - logger.warning( - f"[RESOURCE] Capped log content from {original_line_count} to " - f"{len(lines)} lines ({len(value)} -> {len(truncated)} chars)" - ) - result[key] = truncated - else: - result[key] = value - return result - - -# Resource data size limits to prevent unbounded memory growth -_MAX_RESOURCE_DATA_BYTES = 1_048_576 # 1 MB -_MAX_RESOURCE_DATA_LINES = 5_000 - - def _cap_log_content(data: dict[str, Any]) -> dict[str, Any]: """Cap log content in subscription data to prevent unbounded memory growth. diff --git a/unraid_mcp/tools/docker.py b/unraid_mcp/tools/docker.py index f7e1ddc..7084ec6 100644 --- a/unraid_mcp/tools/docker.py +++ b/unraid_mcp/tools/docker.py @@ -1,7 +1,7 @@ """Docker container management. -Provides the `unraid_docker` tool with 15 actions for container lifecycle, -logs, networks, and update management. +Provides the `unraid_docker` tool with 26 actions for container lifecycle, +logs, networks, update management, and Docker organizer operations. """ import re @@ -395,6 +395,17 @@ def register_docker_tool(mcp: FastMCP) -> None: network_details - Details of a network (requires network_id) port_conflicts - Check for port conflicts check_updates - Check which containers have updates available + create_folder - Create Docker organizer folder (requires folder_name) + set_folder_children - Set children of a folder (requires children_ids) + delete_entries - Delete organizer entries (requires entry_ids, confirm=True) + move_to_folder - Move entries to a folder (requires source_entry_ids, destination_folder_id) + move_to_position - Move entries to position in folder (requires source_entry_ids, destination_folder_id, position) + rename_folder - Rename a folder (requires folder_id, new_folder_name) + create_folder_with_items - Create folder with items (requires folder_name) + update_view_prefs - Update organizer view preferences (requires view_prefs) + sync_templates - Sync Docker template paths + reset_template_mappings - Reset template mappings (confirm=True) + refresh_digests - Refresh container image digests """ if action not in ALL_ACTIONS: raise ToolError(f"Invalid action '{action}'. Must be one of: {sorted(ALL_ACTIONS)}") @@ -436,7 +447,10 @@ def register_docker_tool(mcp: FastMCP) -> None: data = await make_graphql_request( QUERIES["logs"], {"id": actual_id, "tail": tail_lines} ) - return {"logs": safe_get(data, "docker", "logs")} + logs = safe_get(data, "docker", "logs") + if logs is None: + raise ToolError(f"No logs returned for container '{container_id}'") + return {"logs": logs} if action == "networks": data = await make_graphql_request(QUERIES["networks"]) @@ -506,11 +520,10 @@ def register_docker_tool(mcp: FastMCP) -> None: if children_ids is not None: _vars["childrenIds"] = children_ids data = await make_graphql_request(MUTATIONS["create_folder"], _vars) - return { - "success": True, - "action": "create_folder", - "organizer": data.get("createDockerFolder"), - } + organizer = data.get("createDockerFolder") + if organizer is None: + raise ToolError("create_folder failed: server returned no data") + return {"success": True, "action": "create_folder", "organizer": organizer} if action == "set_folder_children": if children_ids is None: @@ -519,11 +532,10 @@ def register_docker_tool(mcp: FastMCP) -> None: if folder_id is not None: _vars["folderId"] = folder_id data = await make_graphql_request(MUTATIONS["set_folder_children"], _vars) - return { - "success": True, - "action": "set_folder_children", - "organizer": data.get("setDockerFolderChildren"), - } + organizer = data.get("setDockerFolderChildren") + if organizer is None: + raise ToolError("set_folder_children failed: server returned no data") + return {"success": True, "action": "set_folder_children", "organizer": organizer} if action == "delete_entries": if not entry_ids: @@ -531,11 +543,10 @@ def register_docker_tool(mcp: FastMCP) -> None: data = await make_graphql_request( MUTATIONS["delete_entries"], {"entryIds": entry_ids} ) - return { - "success": True, - "action": "delete_entries", - "organizer": data.get("deleteDockerEntries"), - } + organizer = data.get("deleteDockerEntries") + if organizer is None: + raise ToolError("delete_entries failed: server returned no data") + return {"success": True, "action": "delete_entries", "organizer": organizer} if action == "move_to_folder": if not source_entry_ids: @@ -549,11 +560,10 @@ def register_docker_tool(mcp: FastMCP) -> None: "destinationFolderId": destination_folder_id, }, ) - return { - "success": True, - "action": "move_to_folder", - "organizer": data.get("moveDockerEntriesToFolder"), - } + organizer = data.get("moveDockerEntriesToFolder") + if organizer is None: + raise ToolError("move_to_folder failed: server returned no data") + return {"success": True, "action": "move_to_folder", "organizer": organizer} if action == "move_to_position": if not source_entry_ids: @@ -572,11 +582,10 @@ def register_docker_tool(mcp: FastMCP) -> None: "position": position, }, ) - return { - "success": True, - "action": "move_to_position", - "organizer": data.get("moveDockerItemsToPosition"), - } + organizer = data.get("moveDockerItemsToPosition") + if organizer is None: + raise ToolError("move_to_position failed: server returned no data") + return {"success": True, "action": "move_to_position", "organizer": organizer} if action == "rename_folder": if not folder_id: @@ -586,11 +595,10 @@ def register_docker_tool(mcp: FastMCP) -> None: data = await make_graphql_request( MUTATIONS["rename_folder"], {"folderId": folder_id, "newName": new_folder_name} ) - return { - "success": True, - "action": "rename_folder", - "organizer": data.get("renameDockerFolder"), - } + organizer = data.get("renameDockerFolder") + if organizer is None: + raise ToolError("rename_folder failed: server returned no data") + return {"success": True, "action": "rename_folder", "organizer": organizer} if action == "create_folder_with_items": if not folder_name: @@ -603,10 +611,13 @@ def register_docker_tool(mcp: FastMCP) -> None: if position is not None: _vars["position"] = position data = await make_graphql_request(MUTATIONS["create_folder_with_items"], _vars) + organizer = data.get("createDockerFolderWithItems") + if organizer is None: + raise ToolError("create_folder_with_items failed: server returned no data") return { "success": True, "action": "create_folder_with_items", - "organizer": data.get("createDockerFolderWithItems"), + "organizer": organizer, } if action == "update_view_prefs": @@ -615,19 +626,17 @@ def register_docker_tool(mcp: FastMCP) -> None: data = await make_graphql_request( MUTATIONS["update_view_prefs"], {"viewId": view_id, "prefs": view_prefs} ) - return { - "success": True, - "action": "update_view_prefs", - "organizer": data.get("updateDockerViewPreferences"), - } + organizer = data.get("updateDockerViewPreferences") + if organizer is None: + raise ToolError("update_view_prefs failed: server returned no data") + return {"success": True, "action": "update_view_prefs", "organizer": organizer} if action == "sync_templates": data = await make_graphql_request(MUTATIONS["sync_templates"]) - return { - "success": True, - "action": "sync_templates", - "result": data.get("syncDockerTemplatePaths"), - } + result = data.get("syncDockerTemplatePaths") + if result is None: + raise ToolError("sync_templates failed: server returned no data") + return {"success": True, "action": "sync_templates", "result": result} if action == "reset_template_mappings": data = await make_graphql_request(MUTATIONS["reset_template_mappings"]) diff --git a/unraid_mcp/tools/notifications.py b/unraid_mcp/tools/notifications.py index e4f4790..9582ee7 100644 --- a/unraid_mcp/tools/notifications.py +++ b/unraid_mcp/tools/notifications.py @@ -261,7 +261,10 @@ def register_notifications_tool(mcp: FastMCP) -> None: "importance": importance.upper(), } data = await make_graphql_request(MUTATIONS["create"], {"input": input_data}) - return {"success": True, "data": data} + notification = data.get("createNotification") + if notification is None: + raise ToolError("Notification creation failed: server returned no data") + return {"success": True, "notification": notification} if action in ("archive", "unread"): if not notification_id: diff --git a/unraid_mcp/tools/storage.py b/unraid_mcp/tools/storage.py index 47fcd8a..1391e50 100644 --- a/unraid_mcp/tools/storage.py +++ b/unraid_mcp/tools/storage.py @@ -204,7 +204,7 @@ def register_storage_tool(mcp: FastMCP) -> None: return {"summary": summary, "details": raw} if action == "unassigned": - return {"devices": data.get("unassignedDevices", [])} + return {"devices": data.get("disks", [])} if action == "log_files": return {"log_files": data.get("logFiles", [])}