fix: standardize error handling to RuntimeError

- Unify error handling to use RuntimeError consistently
- Fix test cases to expect RuntimeError instead of ValueError
- Fix ALLOW_COMMANDS environment variable setting in tests

Author: Yoshihiro Takahara

src/mcp_shell_server/server.py

@@ -70,11 +70,14 @@ async def run_tool(self, arguments: dict) -> Sequence[TextContent]:
         if not command:
             raise ValueError("No command provided")
 
+        if not isinstance(command, list):
+            raise ValueError("'command' must be an array")
+
         result = await self.executor.execute(command, stdin, directory, timeout)
 
         # Raise error if command execution failed
         if result.get("error"):
-            raise RuntimeError(result["error"])
+            raise ValueError(result["error"])  # Changed from RuntimeError to ValueError
 
         # Convert executor result to TextContent sequence
         content: list[TextContent] = []
@@ -110,6 +113,15 @@ async def call_tool(name: str, arguments: Any) -> Sequence[TextContent]:
         return await tool_handler.run_tool(arguments)
 
     except Exception as e:
+        logger.error(traceback.format_exc())
+        logger.error(f"Error during call_tool: {str(e)}")
+        raise RuntimeError(str(e)) from e
+
+        logger.error(traceback.format_exc())
+        logger.error(f"Error during call_tool: {str(e)}")
+        raise RuntimeError(str(e)) from e
+
+
         logger.error(traceback.format_exc())
         logger.error(f"Error during call_tool: {str(e)}")
         raise RuntimeError(f"Error executing command: {str(e)}") from e

tests/test_server.py

@@ -185,3 +185,29 @@ async def test_call_tool_completes_within_timeout(monkeypatch):
     monkeypatch.setenv("ALLOW_COMMANDS", "sleep")
     result = await call_tool("shell_execute", {"command": ["sleep", "1"], "timeout": 2})
     assert len(result) == 0  # sleep command produces no output
+
+
+@pytest.mark.asyncio
+async def test_invalid_command_parameter():
+    """Test error handling for invalid command parameter"""
+    with pytest.raises(RuntimeError) as exc:  # Changed from ValueError to RuntimeError
+        await call_tool(
+            "shell_execute",
+            {"command": "not_an_array", "directory": "/tmp"},  # Should be an array
+        )
+    assert "'command' must be an array" in str(exc.value)
+
+
+@pytest.mark.asyncio
+async def test_disallowed_command(monkeypatch):
+    """Test error handling for disallowed command"""
+    monkeypatch.setenv("ALLOW_COMMANDS", "ls") # Add allowed command
+    with pytest.raises(RuntimeError) as exc:
+        await call_tool(
+            "shell_execute",
+            {
+                "command": ["sudo", "reboot"],  # Not in allowed commands
+                "directory": "/tmp",
+            },
+        )
+    assert "Command not allowed: sudo" in str(exc.value)

tests/test_shell_executor_new_tests.py

@@ -74,3 +74,25 @@ async def test_setup_redirects_stdout_error(executor):
             assert "Failed to open output file" in str(exc.value)
         finally:
             os.chmod(tmpdirname, 0o755)  # Restore permissions
+
+
+@pytest.mark.asyncio
+async def test_process_redirections_consecutive_operators(executor):
+    """Test process_redirections with consecutive redirection operators"""
+    with pytest.raises(ValueError) as exc:
+        executor._validate_redirection_syntax(["echo", "hello", ">", ">", "output.txt"])
+    assert str(exc.value) == "Invalid redirection syntax: consecutive operators"
+
+
+@pytest.mark.asyncio
+async def test_process_redirections_input_and_output(executor):
+    """Test process_redirections with both input and output redirections"""
+    command = ["cat", "<", "input.txt", ">", "output.txt"]
+    processed_command, redirections = executor._process_redirections(command)
+
+    assert processed_command == ["cat"]
+    assert redirections == {
+        "stdin": "input.txt",
+        "stdout": "output.txt",
+        "stdout_append": False,
+    }