From 94850333e8f508dee5bc086d21bb8d589e6f3695 Mon Sep 17 00:00:00 2001 From: Jacob Magar Date: Sun, 15 Mar 2026 20:02:33 -0400 Subject: [PATCH] fix(safety): add stop_array to DESTRUCTIVE_ACTIONS, add error propagation test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stop_array can cause data loss for running containers/VMs that depend on array shares — requires confirm=True like other destructive mutations. - Add stop_array to DESTRUCTIVE_ACTIONS and desc_map in array.py - Update safety audit KNOWN_DESTRUCTIVE[array] to include stop_array - Add stop_array negative/positive tests (test_array.py, safety tests) - Add test_snapshot_wraps_bare_exception to test_live.py (bare Exception from subscribe_once is wrapped by tool_error_handler into ToolError) 748 tests passing --- tests/safety/test_destructive_guards.py | 9 ++++++++- tests/test_array.py | 10 ++++++++-- tests/test_live.py | 11 +++++++++++ unraid_mcp/tools/array.py | 3 ++- 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/tests/safety/test_destructive_guards.py b/tests/safety/test_destructive_guards.py index 25f1a13..d95b2aa 100644 --- a/tests/safety/test_destructive_guards.py +++ b/tests/safety/test_destructive_guards.py @@ -47,7 +47,7 @@ KNOWN_DESTRUCTIVE: dict[str, dict[str, set[str] | str]] = { "module": "unraid_mcp.tools.array", "register_fn": "register_array_tool", "tool_name": "unraid_array", - "actions": {"remove_disk", "clear_disk_stats"}, + "actions": {"remove_disk", "clear_disk_stats", "stop_array"}, "runtime_set": ARRAY_DESTRUCTIVE, }, "vm": { @@ -198,6 +198,7 @@ _DESTRUCTIVE_TEST_CASES: list[tuple[str, str, dict]] = [ # Array ("array", "remove_disk", {"disk_id": "abc123:local"}), ("array", "clear_disk_stats", {"disk_id": "abc123:local"}), + ("array", "stop_array", {}), # VM ("vm", "force_stop", {"vm_id": "test-vm-uuid"}), ("vm", "reset", {"vm_id": "test-vm-uuid"}), @@ -468,6 +469,12 @@ class TestConfirmAllowsExecution: result = await tool_fn(action="clear_disk_stats", disk_id="abc:local", confirm=True) assert result["success"] is True + async def test_array_stop_array_with_confirm(self, _mock_array_graphql: AsyncMock) -> None: + _mock_array_graphql.return_value = {"array": {"setState": {"state": "STOPPED"}}} + tool_fn = make_tool_fn("unraid_mcp.tools.array", "register_array_tool", "unraid_array") + result = await tool_fn(action="stop_array", confirm=True) + assert result["success"] is True + async def test_plugins_remove_with_confirm(self, _mock_plugins_graphql: AsyncMock) -> None: _mock_plugins_graphql.return_value = {"removePlugin": True} tool_fn = make_tool_fn( diff --git a/tests/test_array.py b/tests/test_array.py index 041ab5f..0042355 100644 --- a/tests/test_array.py +++ b/tests/test_array.py @@ -172,9 +172,15 @@ async def test_start_array(_mock_graphql): @pytest.mark.asyncio -async def test_stop_array(_mock_graphql): +async def test_stop_array_requires_confirm(_mock_graphql): + with pytest.raises(ToolError, match="not confirmed"): + await _make_tool()(action="stop_array", confirm=False) + + +@pytest.mark.asyncio +async def test_stop_array_with_confirm(_mock_graphql): _mock_graphql.return_value = {"array": {"setState": {"state": "STOPPED"}}} - result = await _make_tool()(action="stop_array") + result = await _make_tool()(action="stop_array", confirm=True) assert result["success"] is True diff --git a/tests/test_live.py b/tests/test_live.py index e303c2f..4b4f64f 100644 --- a/tests/test_live.py +++ b/tests/test_live.py @@ -112,3 +112,14 @@ async def test_log_tail_rejects_invalid_path(mcp, _mock_subscribe_collect): tool_fn = _make_live_tool(mcp) with pytest.raises(ToolError, match="must start with"): await tool_fn(action="log_tail", path="/etc/shadow") + + +@pytest.mark.asyncio +async def test_snapshot_wraps_bare_exception(mcp, _mock_subscribe_once): + """Bare exceptions from subscribe_once are wrapped in ToolError by tool_error_handler.""" + from unraid_mcp.core.exceptions import ToolError + + _mock_subscribe_once.side_effect = RuntimeError("WebSocket connection refused") + tool_fn = _make_live_tool(mcp) + with pytest.raises(ToolError): + await tool_fn(action="cpu") diff --git a/unraid_mcp/tools/array.py b/unraid_mcp/tools/array.py index c569b8f..8d70841 100644 --- a/unraid_mcp/tools/array.py +++ b/unraid_mcp/tools/array.py @@ -95,7 +95,7 @@ MUTATIONS: dict[str, str] = { """, } -DESTRUCTIVE_ACTIONS = {"remove_disk", "clear_disk_stats"} +DESTRUCTIVE_ACTIONS = {"remove_disk", "clear_disk_stats", "stop_array"} ALL_ACTIONS = set(QUERIES) | set(MUTATIONS) ARRAY_ACTIONS = Literal[ @@ -163,6 +163,7 @@ def register_array_tool(mcp: FastMCP) -> None: desc_map = { "remove_disk": f"Remove disk **{disk_id}** from the array. The array must be stopped first.", "clear_disk_stats": f"Clear all I/O statistics for disk **{disk_id}**. This cannot be undone.", + "stop_array": "Stop the Unraid array. Running containers and VMs may lose access to array shares.", } confirmed = await elicit_destructive_confirmation(ctx, action, desc_map[action]) if not confirmed: