From 183db70d9709c6e661541652508600055e5a184e Mon Sep 17 00:00:00 2001 From: Jacob Magar Date: Tue, 24 Mar 2026 22:50:40 -0400 Subject: [PATCH] fix: address 17 remaining PR review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves review threads: - PRRT_kwDOO6Hdxs50mcYz: oidc/validate_session now documents required `token` - PRRT_kwDOO6Hdxs50mcY8: setting/update corrected to require `settings_input` - PRRT_kwDOO6Hdxs50mcZE: rclone/create_remote corrected to `provider_type`+`config_data` - PRRT_kwDOO6Hdxs50mcZL: disk/logs corrected to `log_path`+`tail_lines` - PRRT_kwDOO6Hdxs50mcZe: parity_progress added to event-driven subscriptions list - PRRT_kwDOO6Hdxs50mcZh: log_tail README example now includes required `path` - PRRT_kwDOO6Hdxs50mcaR: parity_start quick-reference now includes required `correct=False` - PRRT_kwDOO6Hdxs50mcaq: array_state documented as "may show" not "will always show" - PRRT_kwDOO6Hdxs50mnR8: key/create roles is optional; add_role/remove_role use `roles` (plural) - PRRT_kwDOO6Hdxs50mnRd: endpoints.md heading moved before blockquote (MD041) - PRRT_kwDOO6Hdxs50mnTB: test_resources.py uses _get_resource() helper instead of raw internals - PRRT_kwDOO6Hdxs50mYkZ: N/A — _build_google_auth removed in prior refactor commit - PRRT_kwDOO6Hdxs50mnQf: N/A — plugin.json already at 1.1.2, matches pyproject.toml - PRRT_kwDOO6Hdxs50mnQ7: N/A — blank line already present in CLAUDE.md - PRRT_kwDOO6Hdxs50mnRD: N/A — fastmcp.http.json removed in prior refactor commit - PRRT_kwDOO6Hdxs50mnRH: N/A — blank line already present in README.md - PRRT_kwDOO6Hdxs50mnSW: N/A — test_auth_builder.py removed in prior refactor commit --- README.md | 2 +- skills/unraid/SKILL.md | 16 ++++----- skills/unraid/references/endpoints.md | 4 +-- skills/unraid/references/quick-reference.md | 2 +- skills/unraid/references/troubleshooting.md | 4 +-- tests/test_resources.py | 40 +++++++++------------ 6 files changed, 31 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index 4547bb0..d1df295 100644 --- a/README.md +++ b/README.md @@ -252,7 +252,7 @@ The server exposes two classes of MCP resources backed by persistent WebSocket c > **Note**: Resources return cached data from persistent WebSocket subscriptions. A `{"status": "connecting"}` placeholder is returned while the subscription initializes — retry in a moment. > -> **`log_tail` and `notification_feed`** are accessible as tool subactions (`unraid(action="live", subaction="log_tail")`) but are not registered as MCP resources — they use transient one-shot subscriptions and require parameters. +> **`log_tail`** is accessible as a tool subaction (`unraid(action="live", subaction="log_tail", path="/var/log/syslog")`) and requires a `path`; **`notification_feed`** is also available as a tool subaction but uses a transient one-shot subscription and accepts optional parameters. Neither is registered as an MCP resource. > **Security note**: The `disk/logs` and `live/log_tail` subactions allow reading files under `/var/log/` and `/boot/logs/` on the Unraid server. Authenticated MCP clients can stream any log file within these directories. diff --git a/skills/unraid/SKILL.md b/skills/unraid/SKILL.md index 7e84b03..ca9e051 100644 --- a/skills/unraid/SKILL.md +++ b/skills/unraid/SKILL.md @@ -93,7 +93,7 @@ unraid(action="live", subaction="cpu") | `disks` | All physical disks with health and temperatures | | `disk_details` | Detailed info for a specific disk (requires `disk_id`) | | `log_files` | List available log files | -| `logs` | Read log content (requires `path`; optional `lines`) | +| `logs` | Read log content (requires `log_path`; optional `tail_lines`) | | `flash_backup` | ⚠️ Trigger a flash backup (requires `confirm=True`) | ### `docker` — Containers @@ -143,11 +143,11 @@ unraid(action="live", subaction="cpu") |-----------|-------------| | `list` | All API keys | | `get` | Single key details (requires `key_id`) | -| `create` | Create a new key (requires `name`, `roles`) | +| `create` | Create a new key (requires `name`; optional `roles`, `permissions`) | | `update` | Update a key (requires `key_id`) | | `delete` | ⚠️ Delete a key (requires `key_id`, `confirm=True`) | -| `add_role` | Add a role to a key (requires `key_id`, `role`) | -| `remove_role` | Remove a role from a key (requires `key_id`, `role`) | +| `add_role` | Add a role to a key (requires `key_id`, `roles`) | +| `remove_role` | Remove a role from a key (requires `key_id`, `roles`) | ### `plugin` — Plugins | Subaction | Description | @@ -161,13 +161,13 @@ unraid(action="live", subaction="cpu") |-----------|-------------| | `list_remotes` | List configured rclone remotes | | `config_form` | Get configuration form for a remote type | -| `create_remote` | Create a new remote (requires `name`, `type`, `fields`) | +| `create_remote` | Create a new remote (requires `name`, `provider_type`, `config_data`) | | `delete_remote` | ⚠️ Delete a remote (requires `name`, `confirm=True`) | ### `setting` — System Settings | Subaction | Description | |-----------|-------------| -| `update` | Update system settings (requires `settings` object) | +| `update` | Update system settings (requires `settings_input` object) | | `configure_ups` | ⚠️ Configure UPS settings (requires `confirm=True`) | ### `customization` — Theme & Appearance @@ -186,7 +186,7 @@ unraid(action="live", subaction="cpu") | `provider` | Single provider details (requires `provider_id`) | | `configuration` | OIDC configuration | | `public_providers` | Public-facing provider list | -| `validate_session` | Validate current SSO session | +| `validate_session` | Validate current SSO session (requires `token`) | ### `user` — Current User | Subaction | Description | @@ -264,7 +264,7 @@ unraid(action="system", subaction="array") ### Read logs ``` unraid(action="disk", subaction="log_files") -unraid(action="disk", subaction="logs", path="syslog", lines=50) +unraid(action="disk", subaction="logs", log_path="/var/log/syslog", tail_lines=50) ``` ### Live monitoring diff --git a/skills/unraid/references/endpoints.md b/skills/unraid/references/endpoints.md index 5094297..f4fe1c0 100644 --- a/skills/unraid/references/endpoints.md +++ b/skills/unraid/references/endpoints.md @@ -1,7 +1,7 @@ -> **⚠️ DEVELOPER REFERENCE ONLY** — This file documents raw GraphQL endpoints for development purposes. For MCP tool usage, use `unraid(action=..., subaction=...)` calls as documented in `SKILL.md`. - # Unraid API Endpoints Reference +> **⚠️ DEVELOPER REFERENCE ONLY** — This file documents raw GraphQL endpoints for development purposes. For MCP tool usage, use `unraid(action=..., subaction=...)` calls as documented in `SKILL.md`. + Complete list of available GraphQL read-only endpoints in Unraid 7.2+. ## System & Metrics (8) diff --git a/skills/unraid/references/quick-reference.md b/skills/unraid/references/quick-reference.md index eb102e7..64bdbe8 100644 --- a/skills/unraid/references/quick-reference.md +++ b/skills/unraid/references/quick-reference.md @@ -22,7 +22,7 @@ unraid(action="system", subaction="array") # Array status overview unraid(action="disk", subaction="disks") # All disks with temps & health unraid(action="array", subaction="parity_status") # Current parity check unraid(action="array", subaction="parity_history") # Past parity results -unraid(action="array", subaction="parity_start") # Start parity check +unraid(action="array", subaction="parity_start", correct=False) # Start parity check unraid(action="array", subaction="stop_array", confirm=True) # ⚠️ Stop array ``` diff --git a/skills/unraid/references/troubleshooting.md b/skills/unraid/references/troubleshooting.md index d0b830e..74e6fe2 100644 --- a/skills/unraid/references/troubleshooting.md +++ b/skills/unraid/references/troubleshooting.md @@ -76,9 +76,9 @@ See the Destructive Actions table in `SKILL.md` for the full list. **Explanation:** The persistent WebSocket subscription has not yet received its first event. Retry in a moment. -**Known issue:** `live/array_state` uses `arraySubscription` which has a known Unraid API bug (returns null for a non-nullable field). This subscription will always show "connecting." +**Known issue:** `live/array_state` uses `arraySubscription` which has a known Unraid API bug (returns null for a non-nullable field). This subscription may show "connecting" indefinitely. -**Event-driven subscriptions** (`live/notifications_overview`, `live/owner`, `live/server_status`, `live/ups_status`) only populate when the server emits a change event. If the server is idle, these may never populate during a session. +**Event-driven subscriptions** (`live/parity_progress`, `live/notifications_overview`, `live/owner`, `live/server_status`, `live/ups_status`) only populate when the server emits a change event. If the server is idle, these may never populate during a session. **Workaround for array state:** Use `unraid(action="system", subaction="array")` for a synchronous snapshot instead. diff --git a/tests/test_resources.py b/tests/test_resources.py index a0c725d..ef8e864 100644 --- a/tests/test_resources.py +++ b/tests/test_resources.py @@ -17,6 +17,16 @@ def _make_resources(): return test_mcp +def _get_resource(mcp: FastMCP, uri: str): + """Look up a registered resource by URI. + + Accesses FastMCP provider internals. If this breaks after a FastMCP upgrade, + check whether a public resource-lookup API has been added upstream. + """ + key = f"resource:{uri}@" + return mcp.providers[0]._components[key] + + @pytest.fixture def _mock_ensure_started(): with patch( @@ -36,9 +46,7 @@ class TestLiveResourcesUseManagerCache: with patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr: mock_mgr.get_resource_data = AsyncMock(return_value=cached) mcp = _make_resources() - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + resource = _get_resource(mcp, f"unraid://live/{action}") result = await resource.fn() assert json.loads(result) == cached @@ -51,9 +59,7 @@ class TestLiveResourcesUseManagerCache: mock_mgr.get_resource_data = AsyncMock(return_value=None) mock_mgr.last_error = {} mcp = _make_resources() - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + resource = _get_resource(mcp, f"unraid://live/{action}") result = await resource.fn() parsed = json.loads(result) assert parsed["status"] == "connecting" @@ -67,9 +73,7 @@ class TestLiveResourcesUseManagerCache: mock_mgr.connection_states = {action: "auth_failed"} mock_mgr.auto_start_enabled = True mcp = _make_resources() - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + resource = _get_resource(mcp, f"unraid://live/{action}") result = await resource.fn() parsed = json.loads(result) assert parsed["status"] == "error" @@ -103,10 +107,7 @@ class TestLogsStreamResource: with patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr: mock_mgr.get_resource_data = AsyncMock(return_value=None) mcp = _make_resources() - local_provider = mcp.providers[0] - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = local_provider._components["resource:unraid://logs/stream@"] + resource = _get_resource(mcp, "unraid://logs/stream") result = await resource.fn() parsed = json.loads(result) assert "status" in parsed @@ -117,10 +118,7 @@ class TestLogsStreamResource: 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] - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = local_provider._components["resource:unraid://logs/stream@"] + resource = _get_resource(mcp, "unraid://logs/stream") result = await resource.fn() assert json.loads(result) == {} @@ -143,9 +141,7 @@ class TestAutoStartDisabledFallback: mock_mgr.last_error = {} mock_mgr.auto_start_enabled = False mcp = _make_resources() - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + resource = _get_resource(mcp, f"unraid://live/{action}") result = await resource.fn() assert json.loads(result) == fallback_data @@ -164,8 +160,6 @@ class TestAutoStartDisabledFallback: mock_mgr.last_error = {} mock_mgr.auto_start_enabled = False mcp = _make_resources() - # Accessing FastMCP internals intentionally for unit test isolation. - # This may break on FastMCP upgrades — consider a make_resource_fn() helper if it does. - resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"] + resource = _get_resource(mcp, f"unraid://live/{action}") result = await resource.fn() assert json.loads(result)["status"] == "connecting"