From 884319ab1170dafd65a0075a149a3b059c73220d Mon Sep 17 00:00:00 2001 From: Jacob Magar Date: Mon, 16 Mar 2026 03:10:01 -0400 Subject: [PATCH] fix: address 14 PR review comments from coderabbitai/chatgpt-codex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - guards.py: split confirm bypass into explicit check; use .get() for dict description to prevent KeyError on missing action keys - resources.py: use `is not None` for logs stream cache check; add on-demand subscribe_once fallback when auto_start is disabled so resources return real data instead of a perpetual "connecting" placeholder - setup.py: always prompt before overwriting credentials even on failed probe (transient outage ≠ bad credentials); update elicitation message - unraid.py: always elicit_reset_confirmation before overwriting creds; use asyncio.to_thread() for os.path.realpath() to avoid blocking async - test_health.py: update test for new always-prompt-on-overwrite behavior; add test for declined-reset on failed probe - test_resources.py: add tests for logs-stream None check, auto_start disabled fallback (success and failure), and fallback error recovery - test-tools.sh: add suite_live() covering cpu/memory/cpu_telemetry/ notifications_overview/log_tail; include in sequential and parallel runners - CLAUDE.md: correct unraid_live → live action reference; document that setup always prompts before overwriting; note subscribe_once fallback --- CLAUDE.md | 9 +++-- tests/mcporter/test-tools.sh | 14 +++++++ tests/test_health.py | 34 +++++++++++++++- tests/test_resources.py | 56 +++++++++++++++++++++++++++ unraid_mcp/core/guards.py | 13 ++++++- unraid_mcp/core/setup.py | 6 +-- unraid_mcp/subscriptions/resources.py | 13 ++++++- unraid_mcp/tools/unraid.py | 21 +++++++--- 8 files changed, 149 insertions(+), 17 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 451137d..3d00c21 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -83,10 +83,11 @@ docker compose down - **Data Processing**: Tools return both human-readable summaries and detailed raw data - **Health Monitoring**: Comprehensive health check tool for system monitoring - **Real-time Subscriptions**: WebSocket-based live data streaming -- **Persistent Subscription Manager**: `unraid_live` actions use a shared `SubscriptionManager` +- **Persistent Subscription Manager**: `live` action subactions use a shared `SubscriptionManager` that maintains persistent WebSocket connections. Resources serve cached data via `subscription_manager.get_resource_data(action)`. A "connecting" placeholder is returned - while the subscription starts — callers should retry in a moment. + while the subscription starts — callers should retry in a moment. When + `UNRAID_AUTO_START_SUBSCRIPTIONS=false`, resources fall back to on-demand `subscribe_once`. ### Tool Categories (1 Tool, ~107 Subactions) @@ -202,8 +203,8 @@ When bumping the version, **always update both files** — they must stay in syn ### Credential Storage (`~/.unraid-mcp/.env`) All runtimes (plugin, direct, Docker) load credentials from `~/.unraid-mcp/.env`. - **Plugin/direct:** `unraid action=health subaction=setup` writes this file automatically via elicitation, - **Safe to re-run**: if credentials exist and are working, it asks before overwriting. - If credentials exist but connection fails, it silently re-configures without prompting. + **Safe to re-run**: always prompts for confirmation before overwriting existing credentials, + whether the connection is working or not (failed probe may be a transient outage, not bad creds). or manual: `mkdir -p ~/.unraid-mcp && cp .env.example ~/.unraid-mcp/.env` then edit. - **Docker:** `docker-compose.yml` loads it via `env_file` before container start. - **No symlinks needed.** Version bumps do not affect this path. diff --git a/tests/mcporter/test-tools.sh b/tests/mcporter/test-tools.sh index 9a3ebc4..973b84b 100755 --- a/tests/mcporter/test-tools.sh +++ b/tests/mcporter/test-tools.sh @@ -600,6 +600,18 @@ suite_oidc() { # provider and validate_session require IDs — skipped } +suite_live() { + printf '\n%b== live (snapshot subscriptions) ==%b\n' "${C_BOLD}" "${C_RESET}" | tee -a "${LOG_FILE}" + # Note: these subactions open a transient WebSocket and wait for the first event. + # Event-driven actions (parity_progress, ups_status, notifications_overview, + # owner, server_status) return status=no_recent_events when no events arrive. + run_test "live: cpu" '{"action":"live","subaction":"cpu"}' + run_test "live: memory" '{"action":"live","subaction":"memory"}' + run_test "live: cpu_telemetry" '{"action":"live","subaction":"cpu_telemetry"}' + run_test "live: notifications_overview" '{"action":"live","subaction":"notifications_overview"}' + run_test "live: log_tail" '{"action":"live","subaction":"log_tail"}' +} + # --------------------------------------------------------------------------- # Print final summary # --------------------------------------------------------------------------- @@ -650,6 +662,7 @@ run_parallel() { suite_customization suite_plugin suite_oidc + suite_live ) local pids=() @@ -705,6 +718,7 @@ run_sequential() { suite_customization suite_plugin suite_oidc + suite_live } # --------------------------------------------------------------------------- diff --git a/tests/test_health.py b/tests/test_health.py index e5dbf79..233d6c4 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -309,8 +309,8 @@ async def test_health_setup_already_configured_user_confirms_reset() -> None: @pytest.mark.asyncio -async def test_health_setup_credentials_exist_but_connection_fails() -> None: - """setup proceeds with elicitation when credentials exist but connection fails.""" +async def test_health_setup_credentials_exist_but_connection_fails_user_confirms() -> None: + """setup prompts for confirmation even on failed probe, then reconfigures if confirmed.""" tool_fn = _make_tool() mock_path = MagicMock() @@ -322,6 +322,10 @@ async def test_health_setup_credentials_exist_but_connection_fails() -> None: "unraid_mcp.tools.unraid.make_graphql_request", new=AsyncMock(side_effect=Exception("connection refused")), ), + patch( + "unraid_mcp.core.setup.elicit_reset_confirmation", + new=AsyncMock(return_value=True), + ), patch( "unraid_mcp.core.setup.elicit_and_configure", new=AsyncMock(return_value=True) ) as mock_configure, @@ -332,6 +336,32 @@ async def test_health_setup_credentials_exist_but_connection_fails() -> None: assert "configured" in result.lower() or "success" in result.lower() +@pytest.mark.asyncio +async def test_health_setup_credentials_exist_connection_fails_user_declines() -> None: + """setup returns 'no changes' when credentials exist (even with failed probe) and user declines.""" + tool_fn = _make_tool() + + mock_path = MagicMock() + mock_path.exists.return_value = True + + with ( + patch("unraid_mcp.config.settings.CREDENTIALS_ENV_PATH", mock_path), + patch( + "unraid_mcp.tools.unraid.make_graphql_request", + new=AsyncMock(side_effect=Exception("connection refused")), + ), + patch( + "unraid_mcp.core.setup.elicit_reset_confirmation", + new=AsyncMock(return_value=False), + ), + patch("unraid_mcp.core.setup.elicit_and_configure") as mock_configure, + ): + result = await tool_fn(action="health", subaction="setup", ctx=MagicMock()) + + mock_configure.assert_not_called() + assert "no changes" in result.lower() + + @pytest.mark.asyncio async def test_health_setup_ctx_none_already_configured_returns_no_changes() -> None: """When ctx=None and credentials are working, setup returns 'already configured' gracefully.""" diff --git a/tests/test_resources.py b/tests/test_resources.py index b930c5f..19fc428 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -100,3 +100,59 @@ class TestLogsStreamResource: result = await resource.fn() parsed = json.loads(result) assert "status" in parsed + + async def test_logs_stream_returns_data_with_empty_dict( + self, _mock_ensure_started: AsyncMock + ) -> None: + """Empty dict cache hit must return data, not 'connecting' status.""" + with patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr: + mock_mgr.get_resource_data = AsyncMock(return_value={}) + mcp = _make_resources() + local_provider = mcp.providers[0] + resource = local_provider._components["resource:unraid://logs/stream@"] + result = await resource.fn() + assert json.loads(result) == {} + + +class TestAutoStartDisabledFallback: + """When auto_start is disabled, resources fall back to on-demand subscribe_once.""" + + @pytest.mark.parametrize("action", list(SNAPSHOT_ACTIONS.keys())) + async def test_fallback_returns_subscribe_once_data( + self, action: str, _mock_ensure_started: AsyncMock + ) -> None: + fallback_data = {"systemMetricsCpu": {"percentTotal": 42.0}} + with ( + patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr, + patch( + "unraid_mcp.subscriptions.resources.subscribe_once", + new=AsyncMock(return_value=fallback_data), + ), + ): + mock_mgr.get_resource_data = AsyncMock(return_value=None) + mock_mgr.last_error = {} + mock_mgr.auto_start_enabled = False + mcp = _make_resources() + resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + result = await resource.fn() + assert json.loads(result) == fallback_data + + @pytest.mark.parametrize("action", list(SNAPSHOT_ACTIONS.keys())) + async def test_fallback_failure_returns_connecting( + self, action: str, _mock_ensure_started: AsyncMock + ) -> None: + """When on-demand fallback itself fails, still return 'connecting' status.""" + with ( + patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr, + patch( + "unraid_mcp.subscriptions.resources.subscribe_once", + new=AsyncMock(side_effect=Exception("WebSocket failed")), + ), + ): + mock_mgr.get_resource_data = AsyncMock(return_value=None) + mock_mgr.last_error = {} + mock_mgr.auto_start_enabled = False + mcp = _make_resources() + resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + result = await resource.fn() + assert json.loads(result)["status"] == "connecting" diff --git a/unraid_mcp/core/guards.py b/unraid_mcp/core/guards.py index 97199eb..9b3ec2d 100644 --- a/unraid_mcp/core/guards.py +++ b/unraid_mcp/core/guards.py @@ -90,10 +90,19 @@ async def gate_destructive_action( Pass a str when one description covers all destructive actions. Pass a dict[action_name, description] when descriptions differ. """ - if action not in destructive_actions or confirm: + if action not in destructive_actions: return - desc = description[action] if isinstance(description, dict) else description + if confirm: + logger.info("Destructive action '%s' bypassed via confirm=True.", action) + return + + if isinstance(description, dict): + desc = description.get(action) + if desc is None: + raise ToolError(f"Missing destructive-action description for '{action}'.") + else: + desc = description confirmed = await elicit_destructive_confirmation(ctx, action, desc) if not confirmed: raise ToolError( diff --git a/unraid_mcp/core/setup.py b/unraid_mcp/core/setup.py index fa349ee..20d3661 100644 --- a/unraid_mcp/core/setup.py +++ b/unraid_mcp/core/setup.py @@ -30,11 +30,11 @@ class _UnraidCredentials: async def elicit_reset_confirmation(ctx: Context | None, current_url: str) -> bool: - """Ask the user whether to overwrite already-working credentials. + """Ask the user whether to overwrite existing credentials. Args: ctx: The MCP context for elicitation. If None, returns False immediately. - current_url: The currently configured URL (displayed for context). + current_url: The currently configured URL and status (displayed for context). Returns: True if the user confirmed the reset, False otherwise. @@ -45,7 +45,7 @@ async def elicit_reset_confirmation(ctx: Context | None, current_url: str) -> bo try: result = await ctx.elicit( message=( - "Credentials are already configured and working.\n\n" + "Credentials are already configured.\n\n" f"**Current URL:** `{current_url}`\n\n" "Do you want to reset your API URL and key?" ), diff --git a/unraid_mcp/subscriptions/resources.py b/unraid_mcp/subscriptions/resources.py index fa9cfee..cb25463 100644 --- a/unraid_mcp/subscriptions/resources.py +++ b/unraid_mcp/subscriptions/resources.py @@ -15,6 +15,7 @@ from fastmcp import FastMCP from ..config.logging import logger from .manager import subscription_manager from .queries import SNAPSHOT_ACTIONS +from .snapshot import subscribe_once # Global flag to track subscription startup @@ -94,7 +95,7 @@ def register_subscription_resources(mcp: FastMCP) -> None: """Real-time log stream data from subscription.""" await ensure_subscriptions_started() data = await subscription_manager.get_resource_data("logFileSubscription") - if data: + if data is not None: return json.dumps(data, indent=2) return json.dumps( { @@ -118,6 +119,16 @@ def register_subscription_resources(mcp: FastMCP) -> None: "message": f"Subscription '{action}' failed: {last_error}", } ) + # When auto-start is disabled, fall back to a one-shot fetch so the + # resource returns real data instead of a perpetual "connecting" placeholder. + if not subscription_manager.auto_start_enabled: + try: + query_info = SNAPSHOT_ACTIONS.get(action) + if query_info is not None: + fallback_data = await subscribe_once(query_info) + return json.dumps(fallback_data, indent=2) + except Exception as e: + logger.warning("[RESOURCE] On-demand fallback for '%s' failed: %s", action, e) return json.dumps( { "status": "connecting", diff --git a/unraid_mcp/tools/unraid.py b/unraid_mcp/tools/unraid.py index f0cd270..5e6fecf 100644 --- a/unraid_mcp/tools/unraid.py +++ b/unraid_mcp/tools/unraid.py @@ -21,6 +21,7 @@ Actions: live - Real-time WebSocket subscription snapshots (11 subactions) """ +import asyncio import datetime import os import re @@ -312,10 +313,20 @@ async def _handle_health(subaction: str, ctx: Context | None) -> dict[str, Any] connection_ok = True except Exception: connection_ok = False - if connection_ok: - reset = await elicit_reset_confirmation(ctx, safe_display_url(UNRAID_API_URL) or "") - if not reset: - return f"✅ Credentials already configured and working.\nURL: `{safe_display_url(UNRAID_API_URL)}`\n\nNo changes made." + status_note = ( + "and working" + if connection_ok + else "but the connection test failed — may be a transient outage" + ) + reset = await elicit_reset_confirmation( + ctx, + f"{safe_display_url(UNRAID_API_URL) or ''} ({status_note})", + ) + if not reset: + return ( + f"✅ Credentials already configured ({status_note}).\n" + f"URL: `{safe_display_url(UNRAID_API_URL)}`\n\nNo changes made." + ) configured = await elicit_and_configure(ctx) if configured: return "✅ Credentials configured successfully. You can now use all Unraid MCP tools." @@ -641,7 +652,7 @@ async def _handle_disk( raise ToolError(f"tail_lines must be between 1 and {_MAX_TAIL_LINES}, got {tail_lines}") if not log_path: raise ToolError("log_path is required for disk/logs") - normalized = os.path.realpath(log_path) # noqa: ASYNC240 + normalized = await asyncio.to_thread(os.path.realpath, log_path) if not any(normalized.startswith(p) for p in _ALLOWED_LOG_PREFIXES): raise ToolError(f"log_path must start with one of: {', '.join(_ALLOWED_LOG_PREFIXES)}") log_path = normalized