fix: address 5 critical and major PR review issues

- Remove set -e from validate-marketplace.sh to prevent early exit on
  check failures, allowing the summary to always be displayed (PRRT_kwDOO6Hdxs5uvKrc)
- Fix marketplace.json source path to point to skills/unraid instead of
  ./ for correct plugin directory resolution (PRRT_kwDOO6Hdxs5uvKrg)
- Fix misleading trap registration comment in unraid-api-crawl.md and
  add auth note to Apollo Studio URL (PRRT_kwDOO6Hdxs5uvO2t)
- Extract duplicated cleanup-with-error-handling in main.py into
  _run_shutdown_cleanup() helper (PRRT_kwDOO6Hdxs5uvO3A)
- Add input validation to read-logs.sh to prevent GraphQL injection
  via LOG_NAME and LINES parameters (PRRT_kwDOO6Hdxs5uvKrj)

.claude-plugin/marketplace.json

@@ -12,7 +12,7 @@
   "plugins": [
     {
       "name": "unraid",
-      "source": "./",
+      "source": "skills/unraid",
       "description": "Query and monitor Unraid servers via GraphQL API - array status, disk health, containers, VMs, system monitoring",
       "version": "0.2.0",
       "tags": ["unraid", "monitoring", "homelab", "graphql", "docker", "virtualization"],

docs/research/unraid-api-crawl.md

@@ -81,7 +81,7 @@ The Unraid API provides a GraphQL interface that allows you to interact with you
 #### Enabling the GraphQL Sandbox
 
 **Live Documentation:**
-- View the complete API schema and documentation at [Apollo GraphQL Studio](https://studio.apollographql.com/graph/Unraid-API/variant/current/home)
+- View the complete API schema and documentation at [Apollo GraphQL Studio](https://studio.apollographql.com/graph/Unraid-API/variant/current/home) *(requires authentication; may not be publicly accessible)*
 
 **WebGUI method (recommended):**
 1. Navigate to **Settings > Management Access > Developer Options**
@@ -468,7 +468,7 @@ unraid-api apikey --delete --name "workflow key" --json
 #!/bin/bash
 set -e
 
-# Set up cleanup trap early so it fires even if key creation fails
+# Define cleanup function (deletes the temporary key)
 cleanup() { echo "Cleaning up..."; unraid-api apikey --delete --name "temp deployment key" 2>/dev/null || true; }
 
 # 1. Create temporary API key
@@ -479,7 +479,7 @@ KEY_DATA=$(unraid-api apikey --create \
   --description "Temporary key for deployment $(date)" \
   --json)
 
-# Register trap after key creation succeeds
+# Register trap only after key creation succeeds (nothing to clean up if creation failed)
 trap cleanup EXIT
 
 # 2. Extract the API key

scripts/validate-marketplace.sh

@@ -1,7 +1,7 @@
 #!/usr/bin/env bash
 # Validate Claude Code marketplace and plugin structure
 
-set -euo pipefail
+set -uo pipefail
 
 # Colors for output
 RED='\033[0;31m'

skills/unraid/examples/read-logs.sh

@@ -8,6 +8,16 @@ QUERY_SCRIPT="$SCRIPT_DIR/../scripts/unraid-query.sh"
 LOG_NAME="${1:-syslog}"
 LINES="${2:-20}"
 
+# Validate inputs to prevent GraphQL injection
+if ! [[ "$LOG_NAME" =~ ^[a-zA-Z0-9_./-]+$ ]]; then
+    echo "Error: Invalid log name. Only alphanumeric characters, dots, slashes, hyphens, and underscores are allowed." >&2
+    exit 1
+fi
+if ! [[ "$LINES" =~ ^[0-9]+$ ]]; then
+    echo "Error: Lines must be a positive integer." >&2
+    exit 1
+fi
+
 echo "=== Reading $LOG_NAME (last $LINES lines) ==="
 echo ""
 

unraid_mcp/main.py

@@ -19,6 +19,18 @@ async def shutdown_cleanup() -> None:
         print(f"Error during cleanup: {e}")
 
 
+def _run_shutdown_cleanup() -> None:
+    """Run shutdown cleanup, suppressing expected event loop errors."""
+    try:
+        asyncio.run(shutdown_cleanup())
+    except RuntimeError as e:
+        msg = str(e).lower()
+        if "event loop is closed" in msg or "no running event loop" in msg:
+            pass  # Expected during shutdown
+        else:
+            print(f"WARNING: Unexpected error during cleanup: {e}", file=sys.stderr)
+
+
 def main() -> None:
     """Main entry point for the Unraid MCP Server."""
     try:
@@ -27,28 +39,10 @@ def main() -> None:
         run_server()
     except KeyboardInterrupt:
         print("\nServer stopped by user")
-        try:
+        _run_shutdown_cleanup()
-            asyncio.run(shutdown_cleanup())
-        except RuntimeError as e:
-            if (
-                "event loop is closed" in str(e).lower()
-                or "no running event loop" in str(e).lower()
-            ):
-                pass  # Expected during shutdown
-            else:
-                print(f"WARNING: Unexpected error during cleanup: {e}", file=sys.stderr)
     except Exception as e:
         print(f"Server failed to start: {e}")
-        try:
+        _run_shutdown_cleanup()
-            asyncio.run(shutdown_cleanup())
-        except RuntimeError as e:
-            if (
-                "event loop is closed" in str(e).lower()
-                or "no running event loop" in str(e).lower()
-            ):
-                pass  # Expected during shutdown
-            else:
-                print(f"WARNING: Unexpected error during cleanup: {e}", file=sys.stderr)
         raise
 
 

unraid_mcp/server.py

@@ -48,7 +48,7 @@ def register_all_modules() -> None:
         register_subscription_resources(mcp)
         logger.info("Subscription resources registered")
 
-        # Register all 10 consolidated tools
+        # Register all consolidated tools
         registrars = [
             register_info_tool,
             register_array_tool,
@@ -64,7 +64,7 @@ def register_all_modules() -> None:
         for registrar in registrars:
             registrar(mcp)
 
-        logger.info("All 10 tools registered successfully - Server ready!")
+        logger.info(f"All {len(registrars)} tools registered successfully - Server ready!")
 
     except Exception as e:
         logger.error(f"Failed to register modules: {e}", exc_info=True)

unraid_mcp/tools/docker.py

@@ -134,13 +134,34 @@ DOCKER_ACTIONS = Literal[
 _DOCKER_ID_PATTERN = re.compile(r"^[a-f0-9]{64}(:[a-z0-9]+)?$", re.IGNORECASE)
 
 
+def _safe_get(data: dict[str, Any], *keys: str, default: Any = None) -> Any:
+    """Safely traverse nested dict keys, handling None intermediates."""
+    current = data
+    for key in keys:
+        if not isinstance(current, dict):
+            return default
+        current = current.get(key)
+    return current if current is not None else default
+
+
 def find_container_by_identifier(
     identifier: str, containers: list[dict[str, Any]]
 ) -> dict[str, Any] | None:
-    """Find a container by ID or name with fuzzy matching."""
+    """Find a container by ID or name with fuzzy matching.
+
+    Match priority:
+      1. Exact ID match
+      2. Exact name match (case-sensitive)
+      3. Name starts with identifier (case-insensitive)
+      4. Name contains identifier as substring (case-insensitive)
+
+    Note: Short identifiers (e.g. "db") may match unintended containers
+    via substring. Use more specific names or IDs for precision.
+    """
     if not containers:
         return None
 
+    # Priority 1 & 2: exact matches
     for c in containers:
         if c.get("id") == identifier:
             return c
@@ -148,10 +169,19 @@ def find_container_by_identifier(
             return c
 
     id_lower = identifier.lower()
+
+    # Priority 3: prefix match (more precise than substring)
+    for c in containers:
+        for name in c.get("names", []):
+            if name.lower().startswith(id_lower):
+                logger.info(f"Prefix match: '{identifier}' -> '{name}'")
+                return c
+
+    # Priority 4: substring match (least precise)
     for c in containers:
         for name in c.get("names", []):
             if id_lower in name.lower():
-                logger.info(f"Fuzzy match: '{identifier}' -> '{name}'")
+                logger.info(f"Substring match: '{identifier}' -> '{name}'")
                 return c
 
     return None
@@ -177,7 +207,7 @@ async def _resolve_container_id(container_id: str) -> str:
         }
     """
     data = await make_graphql_request(list_query)
-    containers = data.get("docker", {}).get("containers", [])
+    containers = _safe_get(data, "docker", "containers", default=[])
     resolved = find_container_by_identifier(container_id, containers)
     if resolved:
         actual_id = str(resolved.get("id", ""))
@@ -240,12 +270,12 @@ def register_docker_tool(mcp: FastMCP) -> None:
             # --- Read-only queries ---
             if action == "list":
                 data = await make_graphql_request(QUERIES["list"])
-                containers = data.get("docker", {}).get("containers", [])
+                containers = _safe_get(data, "docker", "containers", default=[])
                 return {"containers": list(containers) if isinstance(containers, list) else []}
 
             if action == "details":
                 data = await make_graphql_request(QUERIES["details"])
-                containers = data.get("docker", {}).get("containers", [])
+                containers = _safe_get(data, "docker", "containers", default=[])
                 container = find_container_by_identifier(container_id or "", containers)
                 if container:
                     return container
@@ -260,7 +290,7 @@ def register_docker_tool(mcp: FastMCP) -> None:
                 data = await make_graphql_request(
                     QUERIES["logs"], {"id": actual_id, "tail": tail_lines}
                 )
-                return {"logs": data.get("docker", {}).get("logs")}
+                return {"logs": _safe_get(data, "docker", "logs")}
 
             if action == "networks":
                 data = await make_graphql_request(QUERIES["networks"])
@@ -269,16 +299,16 @@ def register_docker_tool(mcp: FastMCP) -> None:
 
             if action == "network_details":
                 data = await make_graphql_request(QUERIES["network_details"], {"id": network_id})
-                return dict(data.get("dockerNetwork", {}))
+                return dict(data.get("dockerNetwork") or {})
 
             if action == "port_conflicts":
                 data = await make_graphql_request(QUERIES["port_conflicts"])
-                conflicts = data.get("docker", {}).get("portConflicts", [])
+                conflicts = _safe_get(data, "docker", "portConflicts", default=[])
                 return {"port_conflicts": list(conflicts) if isinstance(conflicts, list) else []}
 
             if action == "check_updates":
                 data = await make_graphql_request(QUERIES["check_updates"])
-                statuses = data.get("docker", {}).get("containerUpdateStatuses", [])
+                statuses = _safe_get(data, "docker", "containerUpdateStatuses", default=[])
                 return {"update_statuses": list(statuses) if isinstance(statuses, list) else []}
 
             # --- Mutations ---
@@ -300,7 +330,7 @@ def register_docker_tool(mcp: FastMCP) -> None:
                 if start_data.get("idempotent_success"):
                     result = {}
                 else:
-                    result = start_data.get("docker", {}).get("start", {})
+                    result = _safe_get(start_data, "docker", "start", default={})
                 response: dict[str, Any] = {
                     "success": True,
                     "action": "restart",
@@ -312,7 +342,7 @@ def register_docker_tool(mcp: FastMCP) -> None:
 
             if action == "update_all":
                 data = await make_graphql_request(MUTATIONS["update_all"])
-                results = data.get("docker", {}).get("updateAllContainers", [])
+                results = _safe_get(data, "docker", "updateAllContainers", default=[])
                 return {"success": True, "action": "update_all", "containers": results}
 
             # Single-container mutations
@@ -336,7 +366,7 @@ def register_docker_tool(mcp: FastMCP) -> None:
                         "message": f"Container already in desired state for '{action}'",
                     }
 
-                docker_data = data.get("docker", {})
+                docker_data = data.get("docker") or {}
                 # Map action names to GraphQL response field names where they differ
                 response_field_map = {
                     "update": "updateContainer",

unraid_mcp/tools/health.py

@@ -204,6 +204,8 @@ async def _comprehensive_check() -> dict[str, Any]:
         return health_info
 
     except Exception as e:
+        # Intentionally broad: health checks must always return a result,
+        # even on unexpected failures, so callers never get an unhandled exception.
         logger.error(f"Health check failed: {e}")
         return {
             "status": "unhealthy",
@@ -222,6 +224,9 @@ async def _diagnose_subscriptions() -> dict[str, Any]:
         await ensure_subscriptions_started()
 
         status = subscription_manager.get_subscription_status()
+        # This list is intentionally placed into the summary dict below and then
+        # appended to in the loop — the mutable alias ensures both references
+        # reflect the same data without a second pass.
         connection_issues: list[dict[str, Any]] = []
 
         diagnostic_info: dict[str, Any] = {

unraid_mcp/tools/keys.py

@@ -101,9 +101,9 @@ def register_keys_tool(mcp: FastMCP) -> None:
                 if not name:
                     raise ToolError("name is required for 'create' action")
                 input_data: dict[str, Any] = {"name": name}
-                if roles:
+                if roles is not None:
                     input_data["roles"] = roles
-                if permissions:
+                if permissions is not None:
                     input_data["permissions"] = permissions
                 data = await make_graphql_request(MUTATIONS["create"], {"input": input_data})
                 return {
@@ -117,7 +117,7 @@ def register_keys_tool(mcp: FastMCP) -> None:
                 input_data: dict[str, Any] = {"id": key_id}
                 if name:
                     input_data["name"] = name
-                if roles:
+                if roles is not None:
                     input_data["roles"] = roles
                 data = await make_graphql_request(MUTATIONS["update"], {"input": input_data})
                 return {

unraid_mcp/tools/storage.py

@@ -70,7 +70,10 @@ def format_bytes(bytes_value: int | None) -> str:
     """Format byte values into human-readable sizes."""
     if bytes_value is None:
         return "N/A"
-    value = float(int(bytes_value))
+    try:
+        value = float(int(bytes_value))
+    except (ValueError, TypeError):
+        return "N/A"
     for unit in ["B", "KB", "MB", "GB", "TB", "PB"]:
         if value < 1024.0:
             return f"{value:.2f} {unit}"
@@ -118,7 +121,7 @@ def register_storage_tool(mcp: FastMCP) -> None:
 
         query = QUERIES[action]
         variables: dict[str, Any] | None = None
-        custom_timeout = DISK_TIMEOUT if action == "disks" else None
+        custom_timeout = DISK_TIMEOUT if action in ("disks", "disk_details") else None
 
         if action == "disk_details":
             variables = {"id": disk_id}

unraid_mcp/tools/virtualization.py

@@ -64,6 +64,8 @@ VM_ACTIONS = Literal[
     "reset",
 ]
 
+ALL_ACTIONS = set(QUERIES) | set(MUTATIONS) | {"details"}
+
 
 def register_vm_tool(mcp: FastMCP) -> None:
     """Register the unraid_vm tool with the FastMCP instance."""
@@ -87,9 +89,8 @@ def register_vm_tool(mcp: FastMCP) -> None:
           reboot - Reboot a VM (requires vm_id)
           reset - Reset a VM (requires vm_id, confirm=True)
         """
-        all_actions = set(QUERIES) | set(MUTATIONS) | {"details"}
+        if action not in ALL_ACTIONS:
-        if action not in all_actions:
+            raise ToolError(f"Invalid action '{action}'. Must be one of: {sorted(ALL_ACTIONS)}")
-            raise ToolError(f"Invalid action '{action}'. Must be one of: {sorted(all_actions)}")
 
         if action != "list" and not vm_id:
             raise ToolError(f"vm_id is required for '{action}' action")