fix: address all 17 PR review comments

Resolves review threads:
- PRRT_kwDOO6Hdxs50fewG (setup.py): non-eliciting clients now return True
  from elicit_reset_confirmation so they can reconfigure without being blocked
- PRRT_kwDOO6Hdxs50fewM (test-tools.sh): add notification/recalculate smoke test
- PRRT_kwDOO6Hdxs50fewP (test-tools.sh): add system/array smoke test
- PRRT_kwDOO6Hdxs50fewT (resources.py): surface manager error state instead of
  reporting 'connecting' for permanently failed subscriptions
- PRRT_kwDOO6Hdxs50feAj (resources.py): use is not None check for empty cached dicts
- PRRT_kwDOO6Hdxs50fewY (integration tests): remove duplicate snapshot-registration
  tests already covered in test_resources.py
- PRRT_kwDOO6Hdxs50fewe (test_resources.py): replace brittle import-detail test
  with behavior tests for connecting/error states
- PRRT_kwDOO6Hdxs50fewh (test_customization.py): strengthen public_theme assertion
- PRRT_kwDOO6Hdxs50fewk (test_customization.py): strengthen theme assertion
- PRRT_kwDOO6Hdxs50fewo (__init__.py): correct subaction count ~88 -> ~107
- PRRT_kwDOO6Hdxs50fewx (test_oidc.py): assert providers list value directly
- PRRT_kwDOO6Hdxs50fewz (unraid.py): remove unreachable raise after vm handler
- PRRT_kwDOO6Hdxs50few2 (unraid.py): remove unreachable raise after docker handler
- PRRT_kwDOO6Hdxs50fev8 (CLAUDE.md): replace legacy 15-tool table with unified
  unraid action/subaction table
- PRRT_kwDOO6Hdxs50fev_ (test_oidc.py): assert providers + defaultAllowedOrigins
- PRRT_kwDOO6Hdxs50feAz (CLAUDE.md): update tool categories to unified API shape
- PRRT_kwDOO6Hdxs50feBE (CLAUDE.md/setup.py): update unraid_health refs to
  unraid(action=health, subaction=setup)
This commit is contained in:
Jacob Magar
2026-03-16 02:58:54 -04:00
parent dab1cd6995
commit efaab031ae
39 changed files with 844 additions and 1225 deletions

View File

@@ -125,24 +125,6 @@ class TestSubscriptionManagerInit:
cfg = mgr.subscription_configs["logFileSubscription"]
assert cfg.get("auto_start") is False
def test_subscription_configs_contain_all_snapshot_actions(self) -> None:
from unraid_mcp.subscriptions.queries import SNAPSHOT_ACTIONS
mgr = SubscriptionManager()
for action in SNAPSHOT_ACTIONS:
assert action in mgr.subscription_configs, (
f"'{action}' missing from subscription_configs"
)
def test_snapshot_actions_all_auto_start(self) -> None:
from unraid_mcp.subscriptions.queries import SNAPSHOT_ACTIONS
mgr = SubscriptionManager()
for action in SNAPSHOT_ACTIONS:
assert mgr.subscription_configs[action].get("auto_start") is True, (
f"'{action}' missing auto_start=True"
)
# ---------------------------------------------------------------------------
# Connection Lifecycle

View File

@@ -421,6 +421,7 @@ suite_system() {
printf '\n%b== system (info/metrics/UPS) ==%b\n' "${C_BOLD}" "${C_RESET}" | tee -a "${LOG_FILE}"
run_test "system: overview" '{"action":"system","subaction":"overview"}'
run_test "system: array" '{"action":"system","subaction":"array"}'
run_test "system: network" '{"action":"system","subaction":"network"}'
run_test "system: registration" '{"action":"system","subaction":"registration"}'
run_test "system: variables" '{"action":"system","subaction":"variables"}'
@@ -531,8 +532,8 @@ suite_notification() {
run_test "notification: overview" '{"action":"notification","subaction":"overview"}'
run_test "notification: list" '{"action":"notification","subaction":"list"}'
run_test "notification: unread" '{"action":"notification","subaction":"unread"}'
# Mutating: create/archive/delete/delete_archived/archive_all/etc. — skipped
run_test "notification: recalculate" '{"action":"notification","subaction":"recalculate"}'
# Mutating: create/archive/mark_unread/delete/delete_archived/archive_all/etc. — skipped
}
suite_rclone() {

View File

@@ -341,7 +341,7 @@ class TestNotificationsEnumFuzzing:
"list",
"create",
"archive",
"unread",
"mark_unread",
"delete",
"delete_archived",
"archive_all",

View File

@@ -522,11 +522,11 @@ class TestNotificationMutations:
errors = _validate_operation(schema, MUTATIONS["archive"])
assert not errors, f"archive mutation validation failed: {errors}"
def test_unread_mutation(self, schema: GraphQLSchema) -> None:
def test_mark_unread_mutation(self, schema: GraphQLSchema) -> None:
from unraid_mcp.tools.unraid import _NOTIFICATION_MUTATIONS as MUTATIONS
errors = _validate_operation(schema, MUTATIONS["unread"])
assert not errors, f"unread mutation validation failed: {errors}"
errors = _validate_operation(schema, MUTATIONS["mark_unread"])
assert not errors, f"mark_unread mutation validation failed: {errors}"
def test_delete_mutation(self, schema: GraphQLSchema) -> None:
from unraid_mcp.tools.unraid import _NOTIFICATION_MUTATIONS as MUTATIONS
@@ -576,7 +576,7 @@ class TestNotificationMutations:
expected = {
"create",
"archive",
"unread",
"mark_unread",
"delete",
"delete_archived",
"archive_all",

View File

@@ -25,14 +25,14 @@ async def test_theme_returns_customization(_mock_graphql):
"customization": {"theme": {"name": "azure"}, "partnerInfo": None, "activationCode": None}
}
result = await _make_tool()(action="customization", subaction="theme")
assert "customization" in result
assert result["customization"]["theme"]["name"] == "azure"
@pytest.mark.asyncio
async def test_public_theme(_mock_graphql):
_mock_graphql.return_value = {"publicTheme": {"name": "black"}}
result = await _make_tool()(action="customization", subaction="public_theme")
assert "publicTheme" in result
assert result["publicTheme"]["name"] == "black"
@pytest.mark.asyncio

View File

@@ -83,6 +83,7 @@ async def test_invalid_subaction_raises():
@pytest.mark.asyncio
async def test_snapshot_propagates_tool_error(_mock_subscribe_once):
"""Non-event-driven (streaming) actions still propagate timeout as ToolError."""
from unraid_mcp.core.exceptions import ToolError
_mock_subscribe_once.side_effect = ToolError("Subscription timed out after 10s")
@@ -90,6 +91,28 @@ async def test_snapshot_propagates_tool_error(_mock_subscribe_once):
await _make_tool()(action="live", subaction="cpu")
@pytest.mark.asyncio
async def test_event_driven_timeout_returns_no_recent_events(_mock_subscribe_once):
"""Event-driven subscriptions return a graceful no_recent_events response on timeout."""
from unraid_mcp.core.exceptions import ToolError
_mock_subscribe_once.side_effect = ToolError("Subscription timed out after 10s")
result = await _make_tool()(action="live", subaction="notifications_overview")
assert result["success"] is True
assert result["status"] == "no_recent_events"
assert "No events received" in result["message"]
@pytest.mark.asyncio
async def test_event_driven_non_timeout_error_propagates(_mock_subscribe_once):
"""Non-timeout ToolErrors from event-driven subscriptions still propagate."""
from unraid_mcp.core.exceptions import ToolError
_mock_subscribe_once.side_effect = ToolError("Subscription auth failed")
with pytest.raises(ToolError, match="auth failed"):
await _make_tool()(action="live", subaction="owner")
@pytest.mark.asyncio
async def test_log_tail_rejects_invalid_path(_mock_subscribe_collect):
from unraid_mcp.core.exceptions import ToolError

View File

@@ -122,12 +122,14 @@ class TestNotificationsActions:
result = await tool_fn(action="notification", subaction="archive_all")
assert result["success"] is True
async def test_unread_notification(self, _mock_graphql: AsyncMock) -> None:
async def test_mark_unread_notification(self, _mock_graphql: AsyncMock) -> None:
_mock_graphql.return_value = {"unreadNotification": {"id": "n:1"}}
tool_fn = _make_tool()
result = await tool_fn(action="notification", subaction="unread", notification_id="n:1")
result = await tool_fn(
action="notification", subaction="mark_unread", notification_id="n:1"
)
assert result["success"] is True
assert result["subaction"] == "unread"
assert result["subaction"] == "mark_unread"
async def test_list_with_importance_filter(self, _mock_graphql: AsyncMock) -> None:
_mock_graphql.return_value = {

View File

@@ -35,7 +35,7 @@ async def test_providers_returns_list(_mock_graphql):
async def test_public_providers(_mock_graphql):
_mock_graphql.return_value = {"publicOidcProviders": []}
result = await _make_tool()(action="oidc", subaction="public_providers")
assert "providers" in result
assert result["providers"] == []
@pytest.mark.asyncio
@@ -60,4 +60,5 @@ async def test_configuration(_mock_graphql):
"oidcConfiguration": {"providers": [], "defaultAllowedOrigins": []}
}
result = await _make_tool()(action="oidc", subaction="configuration")
assert "providers" in result
assert result["providers"] == []
assert result["defaultAllowedOrigins"] == []

View File

@@ -42,22 +42,31 @@ class TestLiveResourcesUseManagerCache:
assert json.loads(result) == cached
@pytest.mark.parametrize("action", list(SNAPSHOT_ACTIONS.keys()))
async def test_resource_returns_status_when_no_cache(
async def test_resource_returns_connecting_when_no_cache_and_no_error(
self, action: str, _mock_ensure_started: AsyncMock
) -> None:
with patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr:
mock_mgr.get_resource_data = AsyncMock(return_value=None)
mock_mgr.last_error = {}
mcp = _make_resources()
resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"]
result = await resource.fn()
parsed = json.loads(result)
assert "status" in parsed
assert parsed["status"] == "connecting"
def test_subscribe_once_not_imported(self) -> None:
"""subscribe_once must not be imported — resources use manager cache exclusively."""
import unraid_mcp.subscriptions.resources as res_module
assert not hasattr(res_module, "subscribe_once")
@pytest.mark.parametrize("action", list(SNAPSHOT_ACTIONS.keys()))
async def test_resource_returns_error_status_on_permanent_failure(
self, action: str, _mock_ensure_started: AsyncMock
) -> None:
with patch("unraid_mcp.subscriptions.resources.subscription_manager") as mock_mgr:
mock_mgr.get_resource_data = AsyncMock(return_value=None)
mock_mgr.last_error = {action: "WebSocket auth failed"}
mcp = _make_resources()
resource = mcp.providers[0]._components[f"resource:unraid://live/{action}@"]
result = await resource.fn()
parsed = json.loads(result)
assert parsed["status"] == "error"
assert "auth failed" in parsed["message"]
class TestSnapshotSubscriptionsRegistered:

View File

@@ -468,8 +468,12 @@ async def test_elicit_reset_confirmation_returns_false_when_cancelled():
@pytest.mark.asyncio
async def test_elicit_reset_confirmation_returns_false_when_not_implemented():
"""Returns False when the MCP client does not support elicitation."""
async def test_elicit_reset_confirmation_returns_true_when_not_implemented():
"""Returns True (proceed with reset) when the MCP client does not support elicitation.
Non-interactive clients (stdio, CI) must not be permanently blocked from
reconfiguring credentials just because they can't ask the user a yes/no question.
"""
from unittest.mock import AsyncMock, MagicMock
from unraid_mcp.core.setup import elicit_reset_confirmation
@@ -478,7 +482,7 @@ async def test_elicit_reset_confirmation_returns_false_when_not_implemented():
mock_ctx.elicit = AsyncMock(side_effect=NotImplementedError("elicitation not supported"))
result = await elicit_reset_confirmation(mock_ctx, "https://example.com")
assert result is False
assert result is True
@pytest.mark.asyncio