mirror of
https://github.com/jmagar/unraid-mcp.git
synced 2026-03-23 12:39:24 -07:00
fix: address PR review threads (test-actions, diagnostics, docker, health, storage, plugin)
Resolves review threads:
- PRRT_kwDOO6Hdxs50R8VI (test-actions.sh: remove || echo "000" curl fallback)
- PRRT_kwDOO6Hdxs50R8VJ (test-actions.sh: JSON parse failures → FAIL not silent)
- PRRT_kwDOO6Hdxs50QdKd (diagnostics.py: sanitize raw exception text from ToolError)
- PRRT_kwDOO6Hdxs50QdKs (storage.py: unassigned uses unassignedDevices query)
- PRRT_kwDOO6Hdxs50Mwlk (docker.py: port_conflicts returns flat merged list)
- PRRT_kwDOO6Hdxs50Mwlo (docker.py: logs returns plain string not dict)
- PRRT_kwDOO6Hdxs50Mt5K (docker.py: unraid_docker logs format compatibility)
- PRRT_kwDOO6Hdxs50Mt5L (health.py: or {} null guards throughout)
- PRRT_kwDOO6Hdxs50Mt5r (docker.py: port_conflicts flat list backward compat)
- plugin.json: version synced to 0.4.4 to match pyproject.toml
Changes:
- test-actions.sh: curl exit code captured directly; JSON failures surface as FAIL
- diagnostics.py: 4 ToolError sites log exc_info=True, raise sanitized messages
- storage.py: unassigned action queries unassignedDevices instead of disks
- docker.py: logs action returns newline-joined string; port_conflicts merges
containerPorts + lanPorts into a flat list for backward compatibility
- health.py: all nested dict lookups use `or {}` instead of `.get(k, {})` to
handle explicit GraphQL null values
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -109,7 +109,10 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
|
||||
try:
|
||||
ws_url = build_ws_url()
|
||||
except ValueError as e:
|
||||
raise ToolError(str(e)) from e
|
||||
logger.error("[TEST_SUBSCRIPTION] Invalid WebSocket URL configuration: %s", e)
|
||||
raise ToolError(
|
||||
"Subscription test failed: invalid WebSocket URL configuration."
|
||||
) from e
|
||||
|
||||
ssl_context = build_ws_ssl_context(ws_url)
|
||||
|
||||
@@ -137,7 +140,13 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
|
||||
init_response = json.loads(response)
|
||||
|
||||
if init_response.get("type") != "connection_ack":
|
||||
raise ToolError(f"Connection failed: {init_response}")
|
||||
logger.error(
|
||||
"[TEST_SUBSCRIPTION] Connection not acknowledged: %s",
|
||||
init_response,
|
||||
)
|
||||
raise ToolError(
|
||||
"Subscription test failed: WebSocket connection was not acknowledged."
|
||||
)
|
||||
|
||||
# Send subscription
|
||||
await websocket.send(
|
||||
@@ -165,8 +174,10 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
|
||||
except ToolError:
|
||||
raise
|
||||
except Exception as e:
|
||||
logger.error(f"[TEST_SUBSCRIPTION] Error: {e}", exc_info=True)
|
||||
raise ToolError(f"Subscription test failed: {e!s}") from e
|
||||
logger.error("[TEST_SUBSCRIPTION] Error: %s", e, exc_info=True)
|
||||
raise ToolError(
|
||||
"Subscription test failed: an unexpected error occurred. Check server logs for details."
|
||||
) from e
|
||||
|
||||
@mcp.tool()
|
||||
async def diagnose_subscriptions() -> dict[str, Any]:
|
||||
@@ -269,7 +280,9 @@ def register_diagnostic_tools(mcp: FastMCP) -> None:
|
||||
return diagnostic_info
|
||||
|
||||
except Exception as e:
|
||||
logger.error(f"[DIAGNOSTIC] Failed to generate diagnostics: {e}")
|
||||
raise ToolError(f"Failed to generate diagnostics: {e!s}") from e
|
||||
logger.error("[DIAGNOSTIC] Failed to generate diagnostics: %s", e, exc_info=True)
|
||||
raise ToolError(
|
||||
"Failed to generate diagnostics: an unexpected error occurred. Check server logs for details."
|
||||
) from e
|
||||
|
||||
logger.info("Subscription diagnostic tools registered successfully")
|
||||
|
||||
@@ -447,10 +447,21 @@ def register_docker_tool(mcp: FastMCP) -> None:
|
||||
data = await make_graphql_request(
|
||||
QUERIES["logs"], {"id": actual_id, "tail": tail_lines}
|
||||
)
|
||||
logs = safe_get(data, "docker", "logs")
|
||||
if logs is None:
|
||||
logs_data = safe_get(data, "docker", "logs")
|
||||
if logs_data is None:
|
||||
raise ToolError(f"No logs returned for container '{container_id}'")
|
||||
return {"logs": logs}
|
||||
# Extract log lines into a plain text string for backward compatibility.
|
||||
# The GraphQL response is { containerId, lines: [{ timestamp, message }], cursor }
|
||||
# but callers expect result["logs"] to be a string of log text.
|
||||
lines = logs_data.get("lines", []) if isinstance(logs_data, dict) else []
|
||||
log_text = "\n".join(
|
||||
f"{line.get('timestamp', '')} {line.get('message', '')}".strip()
|
||||
for line in lines
|
||||
)
|
||||
return {
|
||||
"logs": log_text,
|
||||
"cursor": logs_data.get("cursor") if isinstance(logs_data, dict) else None,
|
||||
}
|
||||
|
||||
if action == "networks":
|
||||
data = await make_graphql_request(QUERIES["networks"])
|
||||
@@ -468,7 +479,16 @@ def register_docker_tool(mcp: FastMCP) -> None:
|
||||
|
||||
if action == "port_conflicts":
|
||||
data = await make_graphql_request(QUERIES["port_conflicts"])
|
||||
conflicts = safe_get(data, "docker", "portConflicts", default=[])
|
||||
conflicts_data = safe_get(data, "docker", "portConflicts", default={})
|
||||
# The GraphQL response is { containerPorts: [...], lanPorts: [...] }
|
||||
# but callers expect result["port_conflicts"] to be a flat list.
|
||||
# Merge both conflict lists for backward compatibility.
|
||||
if isinstance(conflicts_data, dict):
|
||||
conflicts: list[Any] = []
|
||||
conflicts.extend(conflicts_data.get("containerPorts", []))
|
||||
conflicts.extend(conflicts_data.get("lanPorts", []))
|
||||
else:
|
||||
conflicts = list(conflicts_data) if conflicts_data else []
|
||||
return {"port_conflicts": conflicts}
|
||||
|
||||
if action == "check_updates":
|
||||
|
||||
@@ -135,21 +135,21 @@ async def _comprehensive_check() -> dict[str, Any]:
|
||||
return health_info
|
||||
|
||||
# System info
|
||||
info = data.get("info", {})
|
||||
info = data.get("info") or {}
|
||||
if info:
|
||||
health_info["unraid_system"] = {
|
||||
"status": "connected",
|
||||
"url": safe_display_url(UNRAID_API_URL),
|
||||
"machine_id": info.get("machineId"),
|
||||
"version": (info.get("versions") or {}).get("core", {}).get("unraid"),
|
||||
"uptime": info.get("os", {}).get("uptime"),
|
||||
"version": ((info.get("versions") or {}).get("core") or {}).get("unraid"),
|
||||
"uptime": (info.get("os") or {}).get("uptime"),
|
||||
}
|
||||
else:
|
||||
_escalate("degraded")
|
||||
issues.append("Unable to retrieve system info")
|
||||
|
||||
# Array
|
||||
array_info = data.get("array", {})
|
||||
array_info = data.get("array") or {}
|
||||
if array_info:
|
||||
state = array_info.get("state", "unknown")
|
||||
health_info["array_status"] = {
|
||||
@@ -164,9 +164,9 @@ async def _comprehensive_check() -> dict[str, Any]:
|
||||
issues.append("Unable to retrieve array status")
|
||||
|
||||
# Notifications
|
||||
notifications = data.get("notifications", {})
|
||||
notifications = data.get("notifications") or {}
|
||||
if notifications and notifications.get("overview"):
|
||||
unread = notifications["overview"].get("unread", {})
|
||||
unread = notifications["overview"].get("unread") or {}
|
||||
alerts = unread.get("alert", 0)
|
||||
health_info["notifications"] = {
|
||||
"unread_total": unread.get("total", 0),
|
||||
@@ -178,7 +178,7 @@ async def _comprehensive_check() -> dict[str, Any]:
|
||||
issues.append(f"{alerts} unread alert(s)")
|
||||
|
||||
# Docker
|
||||
docker = data.get("docker", {})
|
||||
docker = data.get("docker") or {}
|
||||
if docker and docker.get("containers"):
|
||||
containers = docker["containers"]
|
||||
health_info["docker_services"] = {
|
||||
|
||||
@@ -41,7 +41,7 @@ QUERIES: dict[str, str] = {
|
||||
""",
|
||||
"unassigned": """
|
||||
query GetUnassignedDevices {
|
||||
disks { id device name vendor size type interfaceType smartStatus }
|
||||
unassignedDevices { id device name size type }
|
||||
}
|
||||
""",
|
||||
"log_files": """
|
||||
@@ -204,7 +204,7 @@ def register_storage_tool(mcp: FastMCP) -> None:
|
||||
return {"summary": summary, "details": raw}
|
||||
|
||||
if action == "unassigned":
|
||||
return {"devices": data.get("disks", [])}
|
||||
return {"devices": data.get("unassignedDevices", [])}
|
||||
|
||||
if action == "log_files":
|
||||
return {"log_files": data.get("logFiles", [])}
|
||||
|
||||
Reference in New Issue
Block a user