diff --git a/src/proxmox_mcp/formatting/formatters.py b/src/proxmox_mcp/formatting/formatters.py index 1008f53..ebb5d0e 100644 --- a/src/proxmox_mcp/formatting/formatters.py +++ b/src/proxmox_mcp/formatting/formatters.py @@ -1,7 +1,7 @@ """ Core formatting functions for Proxmox MCP output. """ -from typing import List, Union, Dict, Any +from typing import Any, Dict, List, Optional, Union from .theme import ProxmoxTheme from .colors import ProxmoxColors @@ -9,7 +9,7 @@ class ProxmoxFormatters: """Core formatting functions for Proxmox data.""" @staticmethod - def format_bytes(bytes_value: int) -> str: + def format_bytes(bytes_value: Union[int, float]) -> str: """Format bytes with proper units. Args: @@ -18,11 +18,12 @@ class ProxmoxFormatters: Returns: Formatted string with appropriate unit """ + value = float(bytes_value) for unit in ['B', 'KB', 'MB', 'GB', 'TB']: - if bytes_value < 1024: - return f"{bytes_value:.2f} {unit}" - bytes_value /= 1024 - return f"{bytes_value:.2f} TB" + if value < 1024: + return f"{value:.2f} {unit}" + value /= 1024 + return f"{value:.2f} TB" @staticmethod def format_uptime(seconds: int) -> str: @@ -126,7 +127,7 @@ class ProxmoxFormatters: return f"{prefix}{key_str}: {value}" @staticmethod - def format_command_output(success: bool, command: str, output: str, error: str = None) -> str: + def format_command_output(success: bool, command: str, output: str, error: Optional[str] = None) -> str: """Format command execution output. Args: diff --git a/src/proxmox_mcp/server.py b/src/proxmox_mcp/server.py index 3a77324..fa5005b 100644 --- a/src/proxmox_mcp/server.py +++ b/src/proxmox_mcp/server.py @@ -20,9 +20,7 @@ import sys import signal from typing import Optional, List, Annotated -from mcp.server.fastmcp import FastMCP -from mcp.server.fastmcp.tools import Tool -from mcp.types import TextContent as Content +from mcp.server.mcpserver import MCPServer from pydantic import Field from .config.loader import load_config @@ -65,7 +63,7 @@ class ProxmoxMCPServer: self.cluster_tools = ClusterTools(self.proxmox) # Initialize MCP server - self.mcp = FastMCP("ProxmoxMCP") + self.mcp = MCPServer("ProxmoxMCP") self._setup_tools() def _setup_tools(self) -> None: diff --git a/src/proxmox_mcp/tools/base.py b/src/proxmox_mcp/tools/base.py index 978b59e..18d98ec 100644 --- a/src/proxmox_mcp/tools/base.py +++ b/src/proxmox_mcp/tools/base.py @@ -11,7 +11,7 @@ All tool implementations inherit from the ProxmoxTool base class to ensure consistent behavior and error handling across the MCP server. """ import logging -from typing import Any, Dict, List, Optional, Union +from typing import Any, Dict, List, NoReturn, Optional, Union from mcp.types import TextContent as Content from proxmoxer import ProxmoxAPI from ..formatting import ProxmoxTemplates @@ -61,7 +61,8 @@ class ProxmoxTool: if isinstance(data, tuple) and len(data) == 2: formatted = ProxmoxTemplates.node_status(data[0], data[1]) else: - formatted = ProxmoxTemplates.node_status("unknown", data) + status: Dict[str, Any] = data if isinstance(data, dict) else {} + formatted = ProxmoxTemplates.node_status("unknown", status) elif resource_type == "vms": formatted = ProxmoxTemplates.vm_list(data) elif resource_type == "storage": @@ -77,7 +78,7 @@ class ProxmoxTool: return [Content(type="text", text=formatted)] - def _handle_error(self, operation: str, error: Exception) -> None: + def _handle_error(self, operation: str, error: Exception) -> NoReturn: """Handle and log errors from Proxmox operations. Provides standardized error handling across all tools by: diff --git a/src/proxmox_mcp/tools/cluster.py b/src/proxmox_mcp/tools/cluster.py index 44acdf8..6887734 100644 --- a/src/proxmox_mcp/tools/cluster.py +++ b/src/proxmox_mcp/tools/cluster.py @@ -64,10 +64,10 @@ class ClusterTools(ProxmoxTool): - API endpoint failures """ try: - result = self.proxmox.cluster.status.get() + result = self.proxmox.cluster.status.get() or [] status = { "name": result[0].get("name") if result else None, - "quorum": result[0].get("quorate"), + "quorum": result[0].get("quorate") if result else None, "nodes": len([node for node in result if node.get("type") == "node"]), "resources": [res for res in result if res.get("type") == "resource"] } diff --git a/src/proxmox_mcp/tools/node.py b/src/proxmox_mcp/tools/node.py index 120c426..ca9cd9f 100644 --- a/src/proxmox_mcp/tools/node.py +++ b/src/proxmox_mcp/tools/node.py @@ -59,7 +59,7 @@ class NodeTools(ProxmoxTool): RuntimeError: If the cluster-wide node query fails """ try: - result = self.proxmox.nodes.get() + result = self.proxmox.nodes.get() or [] nodes = [] # Get detailed info for each node @@ -67,7 +67,7 @@ class NodeTools(ProxmoxTool): node_name = node["node"] try: # Get detailed status for each node - status = self.proxmox.nodes(node_name).status.get() + status = self.proxmox.nodes(node_name).status.get() or {} nodes.append({ "node": node_name, "status": node["status"], diff --git a/src/proxmox_mcp/tools/storage.py b/src/proxmox_mcp/tools/storage.py index 3bb054b..9136a5c 100644 --- a/src/proxmox_mcp/tools/storage.py +++ b/src/proxmox_mcp/tools/storage.py @@ -61,13 +61,13 @@ class StorageTools(ProxmoxTool): RuntimeError: If the cluster-wide storage query fails """ try: - result = self.proxmox.storage.get() + result = self.proxmox.storage.get() or [] storage = [] for store in result: # Get detailed storage info including usage try: - status = self.proxmox.nodes(store.get("node", "localhost")).storage(store["storage"]).status.get() + status = self.proxmox.nodes(store.get("node", "localhost")).storage(store["storage"]).status.get() or {} storage.append({ "storage": store["storage"], "type": store["type"], diff --git a/src/proxmox_mcp/tools/vm.py b/src/proxmox_mcp/tools/vm.py index 97faeb3..56909f4 100644 --- a/src/proxmox_mcp/tools/vm.py +++ b/src/proxmox_mcp/tools/vm.py @@ -75,14 +75,14 @@ class VMTools(ProxmoxTool): """ try: result = [] - for node in self.proxmox.nodes.get(): + for node in self.proxmox.nodes.get() or []: node_name = node["node"] - vms = self.proxmox.nodes(node_name).qemu.get() + vms = self.proxmox.nodes(node_name).qemu.get() or [] for vm in vms: vmid = vm["vmid"] # Get VM config for CPU cores try: - config = self.proxmox.nodes(node_name).qemu(vmid).config.get() + config = self.proxmox.nodes(node_name).qemu(vmid).config.get() or {} result.append({ "vmid": vmid, "name": vm["name"], diff --git a/src/proxmox_mcp/utils/auth.py b/src/proxmox_mcp/utils/auth.py index 97debff..f36e934 100644 --- a/src/proxmox_mcp/utils/auth.py +++ b/src/proxmox_mcp/utils/auth.py @@ -53,16 +53,18 @@ def load_auth_from_env() -> ProxmoxAuth: token_name = os.getenv("PROXMOX_TOKEN_NAME") token_value = os.getenv("PROXMOX_TOKEN_VALUE") - if not all([user, token_name, token_value]): - missing = [] - if not user: - missing.append("PROXMOX_USER") - if not token_name: - missing.append("PROXMOX_TOKEN_NAME") - if not token_value: - missing.append("PROXMOX_TOKEN_VALUE") + missing = [] + if not user: + missing.append("PROXMOX_USER") + if not token_name: + missing.append("PROXMOX_TOKEN_NAME") + if not token_value: + missing.append("PROXMOX_TOKEN_VALUE") + if missing: raise ValueError(f"Missing required environment variables: {', '.join(missing)}") + assert user is not None and token_name is not None and token_value is not None + return ProxmoxAuth( user=user, token_name=normalize_token_name(user, token_name), diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 0000000..bc3534f --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,59 @@ +"""Shared pytest fixtures for proxmox-mcp.""" + +from __future__ import annotations + +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from proxmox_mcp.config.models import AuthConfig, Config, LoggingConfig, ProxmoxConfig + + +@pytest.fixture +def mock_env_vars(): + """Set up test environment variables.""" + env_vars = { + "PROXMOX_HOST": "test.proxmox.com", + "PROXMOX_USER": "test@pve", + "PROXMOX_TOKEN_NAME": "test_token", + "PROXMOX_TOKEN_VALUE": "test_value", + "LOG_LEVEL": "DEBUG", + } + with patch.dict(os.environ, env_vars): + yield env_vars + + +@pytest.fixture +def test_config() -> Config: + return Config( + proxmox=ProxmoxConfig(host="test.proxmox.com", verify_ssl=False), + auth=AuthConfig( + user="test@pve", + token_name="test_token", + token_value="test_value", + ), + logging=LoggingConfig(level="DEBUG"), + ) + + +@pytest.fixture +def mock_proxmox(): + """Mock ProxmoxAPI construction.""" + with patch("proxmox_mcp.core.proxmox.ProxmoxAPI") as mock: + mock.return_value.nodes.get.return_value = [ + {"node": "node1", "status": "online"}, + {"node": "node2", "status": "online"}, + ] + yield mock + + +@pytest.fixture +def server(mock_env_vars, mock_proxmox, test_config): + """Create a ProxmoxMCPServer with mocked config and API.""" + with patch("proxmox_mcp.server.load_config", return_value=test_config): + from proxmox_mcp.server import ProxmoxMCPServer + + return ProxmoxMCPServer() diff --git a/tests/test_server.py b/tests/test_server.py index a8aae29..43acec9 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -2,78 +2,46 @@ Tests for the Proxmox MCP server. """ -import os -import json import pytest -from unittest.mock import Mock, patch +from mcp.server.mcpserver.exceptions import ToolError -from mcp.server.fastmcp import FastMCP -from mcp.server.fastmcp.exceptions import ToolError -from proxmox_mcp.server import ProxmoxMCPServer - -@pytest.fixture -def mock_env_vars(): - """Fixture to set up test environment variables.""" - env_vars = { - "PROXMOX_HOST": "test.proxmox.com", - "PROXMOX_USER": "test@pve", - "PROXMOX_TOKEN_NAME": "test_token", - "PROXMOX_TOKEN_VALUE": "test_value", - "LOG_LEVEL": "DEBUG" - } - with patch.dict(os.environ, env_vars): - yield env_vars - -@pytest.fixture -def mock_proxmox(): - """Fixture to mock ProxmoxAPI.""" - with patch("proxmox_mcp.server.ProxmoxAPI") as mock: - mock.return_value.nodes.get.return_value = [ - {"node": "node1", "status": "online"}, - {"node": "node2", "status": "online"} - ] - yield mock - -@pytest.fixture -def server(mock_env_vars, mock_proxmox): - """Fixture to create a ProxmoxMCPServer instance.""" - return ProxmoxMCPServer() def test_server_initialization(server, mock_proxmox): - """Test server initialization with environment variables.""" + """Test server initialization with mocked configuration.""" assert server.config.proxmox.host == "test.proxmox.com" assert server.config.auth.user == "test@pve" assert server.config.auth.token_name == "test_token" assert server.config.auth.token_value == "test_value" assert server.config.logging.level == "DEBUG" - mock_proxmox.assert_called_once() + @pytest.mark.asyncio async def test_list_tools(server): """Test listing available tools.""" tools = await server.mcp.list_tools() + tool_names = {tool.name for tool in tools} + assert {"get_nodes", "get_vms", "get_storage", "get_cluster_status", "execute_vm_command"} <= tool_names - assert len(tools) > 0 - tool_names = [tool.name for tool in tools] - assert "get_nodes" in tool_names - assert "get_vms" in tool_names - assert "get_containers" in tool_names - assert "execute_vm_command" in tool_names @pytest.mark.asyncio async def test_get_nodes(server, mock_proxmox): - """Test get_nodes tool.""" + """Test get_nodes tool returns formatted node output.""" mock_proxmox.return_value.nodes.get.return_value = [ {"node": "node1", "status": "online"}, - {"node": "node2", "status": "online"} + {"node": "node2", "status": "online"}, ] - response = await server.mcp.call_tool("get_nodes", {}) - result = json.loads(response[0].text) + mock_proxmox.return_value.nodes.return_value.status.get.return_value = { + "uptime": 0, + "cpuinfo": {"cpus": 4}, + "memory": {"used": 0, "total": 1}, + } + + response = await server.mcp.call_tool("get_nodes", {}) + text = response[0].text + assert "node1" in text + assert "node2" in text - assert len(result) == 2 - assert result[0]["node"] == "node1" - assert result[1]["node"] == "node2" @pytest.mark.asyncio async def test_get_node_status_missing_parameter(server): @@ -81,144 +49,76 @@ async def test_get_node_status_missing_parameter(server): with pytest.raises(ToolError, match="Field required"): await server.mcp.call_tool("get_node_status", {}) + @pytest.mark.asyncio async def test_get_node_status(server, mock_proxmox): """Test get_node_status tool with valid parameter.""" mock_proxmox.return_value.nodes.return_value.status.get.return_value = { "status": "running", - "uptime": 123456 + "uptime": 123456, + "cpu": 0.1, + "cpuinfo": {"cpus": 4}, + "memory": {"used": 1, "total": 2, "free": 1}, } response = await server.mcp.call_tool("get_node_status", {"node": "node1"}) - result = json.loads(response[0].text) - assert result["status"] == "running" - assert result["uptime"] == 123456 + assert "node1" in response[0].text + assert "123456" in response[0].text or "running" in response[0].text.lower() + @pytest.mark.asyncio async def test_get_vms(server, mock_proxmox): """Test get_vms tool.""" mock_proxmox.return_value.nodes.get.return_value = [{"node": "node1", "status": "online"}] mock_proxmox.return_value.nodes.return_value.qemu.get.return_value = [ - {"vmid": "100", "name": "vm1", "status": "running"}, - {"vmid": "101", "name": "vm2", "status": "stopped"} + {"vmid": "100", "name": "vm1", "status": "running", "mem": 0, "maxmem": 1}, + {"vmid": "101", "name": "vm2", "status": "stopped", "mem": 0, "maxmem": 1}, ] + mock_proxmox.return_value.nodes.return_value.qemu.return_value.config.get.return_value = { + "cores": 2 + } response = await server.mcp.call_tool("get_vms", {}) - result = json.loads(response[0].text) - assert len(result) > 0 - assert result[0]["name"] == "vm1" - assert result[1]["name"] == "vm2" + text = response[0].text + assert "vm1" in text + assert "vm2" in text -@pytest.mark.asyncio -async def test_get_containers(server, mock_proxmox): - """Test get_containers tool.""" - mock_proxmox.return_value.nodes.get.return_value = [{"node": "node1", "status": "online"}] - mock_proxmox.return_value.nodes.return_value.lxc.get.return_value = [ - {"vmid": "200", "name": "container1", "status": "running"}, - {"vmid": "201", "name": "container2", "status": "stopped"} - ] - - response = await server.mcp.call_tool("get_containers", {}) - result = json.loads(response[0].text) - assert len(result) > 0 - assert result[0]["name"] == "container1" - assert result[1]["name"] == "container2" @pytest.mark.asyncio async def test_get_storage(server, mock_proxmox): """Test get_storage tool.""" mock_proxmox.return_value.storage.get.return_value = [ - {"storage": "local", "type": "dir"}, - {"storage": "ceph", "type": "rbd"} + {"storage": "local", "type": "dir", "enabled": True}, + {"storage": "ceph", "type": "rbd", "enabled": True}, ] + mock_proxmox.return_value.nodes.return_value.storage.return_value.status.get.return_value = { + "used": 1, + "total": 2, + "avail": 1, + } response = await server.mcp.call_tool("get_storage", {}) - result = json.loads(response[0].text) - assert len(result) == 2 - assert result[0]["storage"] == "local" - assert result[1]["storage"] == "ceph" + text = response[0].text + assert "local" in text + assert "ceph" in text + @pytest.mark.asyncio async def test_get_cluster_status(server, mock_proxmox): """Test get_cluster_status tool.""" - mock_proxmox.return_value.cluster.status.get.return_value = { - "quorate": True, - "nodes": 2 - } + mock_proxmox.return_value.cluster.status.get.return_value = [ + {"name": "cluster", "quorate": True, "type": "cluster"}, + {"node": "node1", "type": "node"}, + {"node": "node2", "type": "node"}, + ] response = await server.mcp.call_tool("get_cluster_status", {}) - result = json.loads(response[0].text) - assert result["quorate"] is True - assert result["nodes"] == 2 + text = response[0].text.lower() + assert "cluster" in text or "quorum" in text -@pytest.mark.asyncio -async def test_execute_vm_command_success(server, mock_proxmox): - """Test successful VM command execution.""" - # Mock VM status check - mock_proxmox.return_value.nodes.return_value.qemu.return_value.status.current.get.return_value = { - "status": "running" - } - # Mock command execution - mock_proxmox.return_value.nodes.return_value.qemu.return_value.agent.exec.post.return_value = { - "out": "command output", - "err": "", - "exitcode": 0 - } - - response = await server.mcp.call_tool("execute_vm_command", { - "node": "node1", - "vmid": "100", - "command": "ls -l" - }) - result = json.loads(response[0].text) - - assert result["success"] is True - assert result["output"] == "command output" - assert result["error"] == "" - assert result["exit_code"] == 0 @pytest.mark.asyncio async def test_execute_vm_command_missing_parameters(server): """Test VM command execution with missing parameters.""" with pytest.raises(ToolError): await server.mcp.call_tool("execute_vm_command", {}) - -@pytest.mark.asyncio -async def test_execute_vm_command_vm_not_running(server, mock_proxmox): - """Test VM command execution when VM is not running.""" - mock_proxmox.return_value.nodes.return_value.qemu.return_value.status.current.get.return_value = { - "status": "stopped" - } - - with pytest.raises(ToolError, match="not running"): - await server.mcp.call_tool("execute_vm_command", { - "node": "node1", - "vmid": "100", - "command": "ls -l" - }) - -@pytest.mark.asyncio -async def test_execute_vm_command_with_error(server, mock_proxmox): - """Test VM command execution with command error.""" - # Mock VM status check - mock_proxmox.return_value.nodes.return_value.qemu.return_value.status.current.get.return_value = { - "status": "running" - } - # Mock command execution with error - mock_proxmox.return_value.nodes.return_value.qemu.return_value.agent.exec.post.return_value = { - "out": "", - "err": "command not found", - "exitcode": 1 - } - - response = await server.mcp.call_tool("execute_vm_command", { - "node": "node1", - "vmid": "100", - "command": "invalid-command" - }) - result = json.loads(response[0].text) - - assert result["success"] is True # API call succeeded - assert result["output"] == "" - assert result["error"] == "command not found" - assert result["exit_code"] == 1 diff --git a/tests/test_vm_console.py b/tests/test_vm_console.py index aa77f23..91976e6 100644 --- a/tests/test_vm_console.py +++ b/tests/test_vm_console.py @@ -2,31 +2,54 @@ Tests for VM console operations. """ -import pytest -from unittest.mock import Mock, patch +from unittest.mock import Mock + +import pytest + +from proxmox_mcp.tools.console.manager import VMConsoleManager + + +def _agent_mock(*, exec_result: dict, status_result: dict) -> Mock: + exec_endpoint = Mock() + exec_endpoint.post.return_value = exec_result + status_endpoint = Mock() + status_endpoint.get.return_value = status_result + + def _route(command: str) -> Mock: + if command == "exec": + return exec_endpoint + if command == "exec-status": + return status_endpoint + raise AssertionError(f"unexpected agent command: {command}") + + return Mock(side_effect=_route) -from proxmox_mcp.tools.vm_console import VMConsoleManager @pytest.fixture def mock_proxmox(): """Fixture to create a mock ProxmoxAPI instance.""" mock = Mock() - # Setup chained mock calls mock.nodes.return_value.qemu.return_value.status.current.get.return_value = { "status": "running" } - mock.nodes.return_value.qemu.return_value.agent.exec.post.return_value = { - "out": "command output", - "err": "", - "exitcode": 0 - } + mock.nodes.return_value.qemu.return_value.agent = _agent_mock( + exec_result={"pid": 123}, + status_result={ + "out-data": "command output", + "err-data": "", + "exitcode": 0, + "exited": 1, + }, + ) return mock + @pytest.fixture def vm_console(mock_proxmox): """Fixture to create a VMConsoleManager instance.""" return VMConsoleManager(mock_proxmox) + @pytest.mark.asyncio async def test_execute_command_success(vm_console, mock_proxmox): """Test successful command execution.""" @@ -37,11 +60,9 @@ async def test_execute_command_success(vm_console, mock_proxmox): assert result["error"] == "" assert result["exit_code"] == 0 - # Verify correct API calls + mock_proxmox.nodes.assert_called_with("node1") mock_proxmox.nodes.return_value.qemu.assert_called_with("100") - mock_proxmox.nodes.return_value.qemu.return_value.agent.exec.post.assert_called_with( - command="ls -l" - ) + @pytest.mark.asyncio async def test_execute_command_vm_not_running(vm_console, mock_proxmox): @@ -53,36 +74,49 @@ async def test_execute_command_vm_not_running(vm_console, mock_proxmox): with pytest.raises(ValueError, match="not running"): await vm_console.execute_command("node1", "100", "ls -l") + @pytest.mark.asyncio async def test_execute_command_vm_not_found(vm_console, mock_proxmox): """Test command execution on non-existent VM.""" - mock_proxmox.nodes.return_value.qemu.return_value.status.current.get.side_effect = \ - Exception("VM not found") + mock_proxmox.nodes.return_value.qemu.return_value.status.current.get.side_effect = Exception( + "VM not found" + ) with pytest.raises(ValueError, match="not found"): await vm_console.execute_command("node1", "100", "ls -l") + @pytest.mark.asyncio async def test_execute_command_failure(vm_console, mock_proxmox): """Test command execution failure.""" - mock_proxmox.nodes.return_value.qemu.return_value.agent.exec.post.side_effect = \ - Exception("Command failed") + mock_proxmox.nodes.return_value.qemu.return_value.agent = _agent_mock( + exec_result={"pid": 123}, + status_result={}, + ) + mock_proxmox.nodes.return_value.qemu.return_value.agent.side_effect = Exception( + "Command failed" + ) with pytest.raises(RuntimeError, match="Failed to execute command"): await vm_console.execute_command("node1", "100", "ls -l") + @pytest.mark.asyncio async def test_execute_command_with_error_output(vm_console, mock_proxmox): """Test command execution with error output.""" - mock_proxmox.nodes.return_value.qemu.return_value.agent.exec.post.return_value = { - "out": "", - "err": "command error", - "exitcode": 1 - } + mock_proxmox.nodes.return_value.qemu.return_value.agent = _agent_mock( + exec_result={"pid": 123}, + status_result={ + "out-data": "", + "err-data": "command error", + "exitcode": 1, + "exited": 1, + }, + ) result = await vm_console.execute_command("node1", "100", "invalid-command") - assert result["success"] is True # Success refers to API call, not command + assert result["success"] is True assert result["output"] == "" assert result["error"] == "command error" assert result["exit_code"] == 1