CLI: Improve code using suggestions by @coderabbitai

responder/util/cmd.py

@@ -39,8 +39,17 @@ def __init__(self, target: str, port: int = 5042, limit_max_requests: int = None
         super().__init__()
 
         # Sanity checks, as suggested by @coderabbitai. Thanks.
+
+        # Validate path, also prevent path traversal.
         if not target or not isinstance(target, str):
             raise ValueError("Target must be a non-empty string")
+        target_path = Path(target).resolve()
+        if not target_path.exists():
+            raise ValueError(f"Target path does not exist: {target}")
+        if ".." in target_path.parts:
+            raise ValueError("Path traversal detected in target")
+
+        # Validate port and other options.
         if not isinstance(port, int) or port < 1:
             raise ValueError("Port must be a positive integer")
         if limit_max_requests is not None and (

responder/util/python.py

@@ -10,7 +10,7 @@ def load_target(target: str, default_property: str = "api", method: str = "run")
     """
 
     # Sanity checks, as suggested by @coderabbitai. Thanks.
-    if not target or ":" in target and len(target.split(":")) != 2:
+    if not target or (":" in target and len(target.split(":")) != 2):
         raise ValueError(
             "Invalid target format. "
             "Use '<module>', '<module>:<property>', "

tests/test_cli.py

@@ -6,6 +6,7 @@
 import pytest
 import requests
 from _pytest.capture import CaptureFixture
+from requests_toolbelt.multipart.encoder import to_list
 
 from responder.__version__ import __version__
 from responder.util.cmd import ResponderProgram, ResponderServer
@@ -64,19 +65,34 @@ def test_cli_build_missing_package_json(capfd, tmp_path):
     assert "Error: ENOENT: no such file or directory" in stderr
 
 
-def test_cli_build_invalid_package_json(capfd, tmp_path):
+@pytest.mark.parametrize(
+    "invalid_content,npm_error,expected_error",
+    [
+        ("foobar", "code EJSONPARSE", ["is not valid JSON", "Failed to parse JSON data"]),
+        ("{", "code EJSONPARSE", "Unexpected end of JSON input"),
+        ('{"scripts": }', "code EJSONPARSE", "Unexpected token"),
+        (
+            '{"scripts": null}',
+            "Cannot convert undefined or null to object",
+            "scripts.build script not found",
+        ),
+    ],
+)
+def test_cli_build_invalid_package_json(
+    capfd, tmp_path, invalid_content, npm_error, expected_error
+):
     """
     Verify `responder build` using an invalid `package.json` file.
     """
 
     # Temporary surrogate `package.json` file.
     package_json_file = tmp_path / "package.json"
-    package_json_file.write_text("foobar")
+    package_json_file.write_text(invalid_content)
 
     # Invoke `responder build`.
     stdout, stderr = responder_build(tmp_path, capfd)
-    assert "npm error code EJSONPARSE" in stderr
-    assert '"foobar" is not valid JSON' in stderr
+    assert f"npm error {npm_error}" in stderr
+    assert any(item in stderr for item in to_list(expected_error))
 
 
 def test_cli_run(capfd):

tests/util.py

@@ -1,30 +1,53 @@
+import socket
 import time
 
 import requests
 
 
-def random_port():
+def random_port() -> int:
     """
-    Return a random available port.
-    """
-    import socket
+    Return a random available port by binding to port 0.
 
+    Returns:
+        int: An available port number that can be used for testing.
+    """
     sock = socket.socket()
-    sock.bind(("", 0))
-    port = sock.getsockname()[1]
-    sock.close()
-    return port
+    try:
+        sock.bind(("", 0))
+        return sock.getsockname()[1]
+    finally:
+        sock.close()
 
 
-def wait_server(port: int):
+def wait_server(
+    port: int,
+    host: str = "127.0.0.1",
+    protocol: str = "http",
+    attempts: int = 20,
+    timeout: float = 0.1,
+):
     """
-    Wait for server to be ready.
+    Wait for server to be ready by attempting to connect to it.
+
+    Args:
+        port: The port number to connect to
+        host: The host to connect to (default: "127.0.0.1")
+        protocol: The protocol to use (default: "http")
+        attempts: Number of connection attempts (default: 20)
+        timeout: Timeout per attempt in seconds (default: 0.1)
+
+    Raises:
+        RuntimeError: If server is not ready after all attempts
     """
-    for _ in range(20):  # 2 second timeout
+    url = f"{protocol}://{host}:{port}/"
+    for _ in range(1, attempts + 1):
         try:
-            requests.get(f"http://127.0.0.1:{port}/", timeout=0.1)
+            requests.get(url, timeout=timeout)
             break
         except requests.exceptions.RequestException:
-            time.sleep(0.1)
+            time.sleep(timeout)
     else:
-        raise RuntimeError("Server failed to start")
+        raise RuntimeError(
+            f"Server at {url} failed to respond after {attempts} attempts "
+            f"(total wait time: {attempts * timeout * 2:.1f}s)"
+        )