Merge pull request #73 from adamamyl/feature/quality-tooling

feat(quality): add ruff/mypy/bandit tooling and fix all issues

Author: adamamyl

.github/workflows/quality.yml

@@ -0,0 +1,9 @@
+name: Quality
+on: [push, pull_request]
+jobs:
+  quality:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: astral-sh/setup-uv@v5
+      - run: uv run pre-commit run --all-files

.pre-commit-config.yaml

@@ -0,0 +1,34 @@
+repos:
+  - repo: https://github.com/astral-sh/ruff-pre-commit
+    rev: v0.11.13
+    hooks:
+      - id: ruff
+        args: [--fix]
+      - id: ruff-format
+
+  - repo: https://github.com/PyCQA/bandit
+    rev: 1.8.5
+    hooks:
+      - id: bandit
+        args: ["-c", "pyproject.toml"]
+
+  - repo: https://github.com/pre-commit/mirrors-mypy
+    rev: v1.16.0
+    hooks:
+      - id: mypy
+        additional_dependencies: ["types-requests"]
+
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v5.0.0
+    hooks:
+      - id: check-json
+      - id: check-yaml
+      - id: check-added-large-files
+      - id: detect-private-key
+      - id: no-commit-to-branch
+        args: [--branch, main]
+
+  - repo: https://github.com/betterleaks/betterleaks
+    rev: v1.1.2
+    hooks:
+      - id: betterleaks

lib/executor.py

@@ -1,7 +1,7 @@
 import subprocess
 import os
 import sys
-from typing import List, Optional, Union, Any, Dict
+from typing import List, Optional, Union, Dict
 from .logger import log
 from . import constants
 
@@ -11,7 +11,9 @@ class Executor:
     Handles DRY_RUN, SUDO elevation, logging, and error checking.
     """
 
-    def __init__(self, dry_run: bool = False, quiet: bool = False, verbose: bool = False, force: bool = False):
+    def __init__(
+        self, dry_run: bool = False, quiet: bool = False, verbose: bool = False, force: bool = False
+    ):
         self.dry_run = dry_run
         self.quiet = quiet
         self.verbose = verbose
@@ -31,9 +33,10 @@ def run(self,
             env: Optional[Dict[str, str]] = None,
             check: bool = True,
             run_quiet: bool = False, 
-            interactive: bool = False) -> subprocess.CompletedProcess:
+            interactive: bool = False) -> subprocess.CompletedProcess[str]:
         """
-        Executes a shell command. If interactive=True, it allows direct terminal I/O (no pipe capture).
+        Executes a shell command.
+        If interactive=True, allows direct terminal I/O (no pipe capture).
         """
 
         if isinstance(command, str):
@@ -44,8 +47,7 @@ def run(self,
             log_cmd = " ".join(command)
 
         if user:
-            # Note: We are running the command as root, but providing user context via sudo -u 
-            # (although in the recursive call, the 'user' is handled by the recursive script's logic)
+            # Running as root but delegating via sudo -u; recursive calls handle user context.
             if os.geteuid() != 0:
                 log_cmd = f"sudo -H -u {user} {log_cmd}"
                 cmd_list = ['sudo', '-H', '-u', user] + cmd_list
@@ -63,7 +65,7 @@ def run(self,
         if self.dry_run:
             if not suppress_logging:
                 log.info(f"[DRY-RUN] {log_cmd}")
-            return subprocess.CompletedProcess(args=cmd_list, returncode=0, stdout=b"", stderr=b"")
+            return subprocess.CompletedProcess(args=cmd_list, returncode=0, stdout="", stderr="")
 
         # --- 4. I/O Stream Determination ---
         if interactive:
@@ -123,10 +125,10 @@ def run(self,
 EXEC = Executor()
 
 
-def run_function_as_user(executor: Executor, 
-                         user: str, 
-                         function_name: str, 
-                         *func_args: str) -> subprocess.CompletedProcess:
+def run_function_as_user(executor: Executor,
+                         user: str,
+                         function_name: str,
+                         *func_args: str) -> subprocess.CompletedProcess[str]:
     """
     Executes a specific Python function (by name) from the main script as another user
     by recursively calling the setup script.

lib/installer_utils/apt_tools.py

@@ -7,7 +7,9 @@ def _is_package_installed(pkg: str) -> bool:
     """Checks if a package is installed using dpkg -s."""
     try:
         # Use subprocess.run directly as we don't need Executor for a simple check
-        subprocess.run(['dpkg', '-s', pkg], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
+        subprocess.run(
+            ['dpkg', '-s', pkg], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
+        )
         return True
     except subprocess.CalledProcessError:
         return False
@@ -61,7 +63,7 @@ def apt_autoremove(exec_obj: Executor) -> None:
     exec_obj.run(autoremove_cmd, force_sudo=True)
 
 def ensure_apt_repo(exec_obj: Executor, list_file: str, repo_line: str) -> None:
-    """Adds an apt repository line to a file if it is not already present, and deduplicates the file."""
+    """Adds an apt repository line to a file if not already present, and deduplicates."""
 
     existing_lines = []
     try:

lib/installer_utils/git_tools.py

@@ -1,6 +1,6 @@
 import os
 import subprocess
-from typing import Optional, List
+from typing import Optional
 from ..executor import Executor
 from ..logger import log
 from ..constants import GIT_BIN_PATH
@@ -53,7 +53,9 @@ def clone_or_update_repo(exec_obj: Executor,
     if os.path.isdir(os.path.join(dest_dir, ".git")):
         try:
             # INTEGRITY CHECK: Use the resolved path constant (GIT_BIN_PATH)
-            exec_obj.run([GIT_BIN_PATH, '-C', dest_dir, 'rev-parse', '--is-inside-work-tree'], user=user) 
+            exec_obj.run(
+                [GIT_BIN_PATH, '-C', dest_dir, 'rev-parse', '--is-inside-work-tree'], user=user
+            )
 
             log.info(f"Updating existing repository: {dest_dir}")
 
@@ -64,7 +66,9 @@ def clone_or_update_repo(exec_obj: Executor,
 
         except Exception:
             # Integrity check or fetch failed -> Remove and re-clone
-            log.warning(f"Repo integrity check or fetch failed at {dest_dir}; removing and recloning.")
+            log.warning(
+                f"Repo integrity check or fetch failed at {dest_dir}; removing and recloning."
+            )
             exec_obj.run(f"rm -rf {dest_dir}", force_sudo=True)
 
     # 4. Clone if missing or just removed
@@ -99,7 +103,10 @@ def clone_or_update_private_repo_with_key_check(exec_obj: Executor,
 
     for attempt in range(MAX_CLONE_ATTEMPTS):
         try:
-            log.info(f"Attempting to clone/update repository (Attempt {attempt + 1}/{MAX_CLONE_ATTEMPTS})...")
+            log.info(
+                f"Attempting to clone/update repository "
+                f"(Attempt {attempt + 1}/{MAX_CLONE_ATTEMPTS})..."
+            )
             clone_or_update_repo(
                 exec_obj, 
                 repo_url, 
@@ -151,7 +158,7 @@ def _configure_repo_ssh_key(exec_obj: Executor, user: str, repo_dir: str, key_pa
     log.info(f"Configuring Git SSH command for {repo_dir}")
 
     # Git command to set the core.sshCommand locally
-    # We use single quotes around the key_path in the ssh_command_value to protect it in the git config file
+    # Single quotes around key_path protect spaces in the git config value.
     ssh_command_value = f"ssh -i '{key_path}' -o IdentitiesOnly=yes"
 
     # --- FIX: Pass command as a single string and remove --local ---
@@ -167,12 +174,14 @@ def _configure_repo_ssh_key(exec_obj: Executor, user: str, repo_dir: str, key_pa
 
         log.success(f"Set core.sshCommand to use {key_path} in {repo_dir}")
     except Exception as e:
-        log.error(f"Failed to set local Git SSH config in {repo_dir}. Manual Git operations may fail.")
+        log.error(
+            f"Failed to set local Git SSH config in {repo_dir}. Manual Git operations may fail."
+        )
         log.debug(f"Git config error: {e}")
 
 
 def set_homedir_perms_recursively(exec_obj: Executor, user: str, dir_path: str) -> None:
-    """Sets sane read/write permissions while preserving executable bits on files and directories."""
+    """Sets sane read/write permissions, preserving executable bits on files and directories."""
     log.info(f"Setting recursive permissions for {dir_path} owned by {user}")
 
     # 1. Set ownership recursively
@@ -187,7 +196,7 @@ def set_homedir_perms_recursively(exec_obj: Executor, user: str, dir_path: str)
     exec_obj.run(f"chmod -R go-w {dir_path}", force_sudo=True)
 
     # c) Crucial: Grant execute permission selectively (+X). 
-    # '+X' only grants execute if the item is a directory OR if it already has execute permissions set for any user.
+    # '+X' grants execute only for directories or items already executable by any user.
     exec_obj.run(f"chmod -R a+X {dir_path}", force_sudo=True)
 
 
@@ -199,8 +208,7 @@ def set_ssh_perms(exec_obj: Executor, user: str, ssh_dir: str) -> None:
     exec_obj.run(f"chmod 700 {ssh_dir}", force_sudo=True)
     exec_obj.run(f"chown {user}:{user} {ssh_dir}", force_sudo=True)
 
-    # Note: We rely on _create_if_needed_ssh_key to enforce 600/644 on private/public keys.
-    # We still ensure all files inside have correct ownership (if the keys were newly generated by root).
+    # Re-fix ownership in case root generated the keys; _create_if_needed_ssh_key handles 600/644.
     exec_obj.run(f"chown {user}:{user} {ssh_dir}/* || true", force_sudo=True)
 
     # We explicitly relax permissions on known_hosts and public keys to 644/400, just in case

lib/installer_utils/module_docker.py

@@ -1,7 +1,7 @@
 import shutil
 import platform
 import os
-from typing import List, Optional, Union, Dict
+from typing import List, Dict
 import subprocess
 
 from ..executor import Executor
@@ -36,7 +36,10 @@ def _remove_old_docker(exec_obj: Executor) -> None:
     log.info("Checking for and removing old/conflicting Docker packages...")
 
     # Comprehensive query command to find packages that need removal
-    query_cmd = "dpkg --get-selections docker.io docker-compose docker-compose-v2 docker-doc podman-docker containerd runc | cut -f1"
+    query_cmd = (
+        "dpkg --get-selections docker.io docker-compose docker-compose-v2 docker-doc "
+        "podman-docker containerd runc | cut -f1"
+    )
 
     try:
         # Execute the query command as root to get the list of packages to remove
@@ -51,15 +54,17 @@ def _remove_old_docker(exec_obj: Executor) -> None:
         # 2. Construct the removal command
         remove_cmd = ["apt", "remove", "-y"] + packages_to_remove
 
-        log.warning(f"Removing the following old/conflicting packages: {', '.join(packages_to_remove)}")
+        log.warning(
+            f"Removing the following old/conflicting packages: {', '.join(packages_to_remove)}"
+        )
 
         # Execute the removal command
-        # We allow check=False in case some packages are listed by dpkg but apt fails to find them, though unlikely here.
+        # check=False: dpkg may list packages that apt can't find; we continue regardless.
         exec_obj.run(remove_cmd, force_sudo=True, check=True)
         log.success("Old Docker packages successfully removed.")
 
     except Exception as e:
-        log.warning(f"Failed to execute package removal query or removal. Continuing installation.")
+        log.warning("Failed to execute package removal query or removal. Continuing installation.")
         log.debug(f"Removal error: {e}")
         # We don't halt here, as the subsequent installation step will fail if necessary.
 
@@ -91,21 +96,25 @@ def install_docker_and_add_users(exec_obj: Executor, *users_to_add: str) -> None
         codename = os_info.get("VERSION_CODENAME")  # e.g., 'noble' or 'bookworm'
 
         if not os_id or not codename:
-            log.critical("Could not detect OS ID or Codename from /etc/os-release. Aborting Docker setup.")
+            log.critical(
+                "Could not detect OS ID or Codename from /etc/os-release. Aborting Docker setup."
+            )
             raise RuntimeError("Cannot proceed without distribution details.")
 
         log.info(f"Detected OS: {os_id}, Codename: {codename}")
 
         keyrings_dir = "/etc/apt/keyrings"
-        docker_gpg_path = os.path.join(keyrings_dir, "docker.gpg") # Modern standard uses .gpg binary
+        docker_gpg_path = os.path.join(keyrings_dir, "docker.gpg")
         list_file = "/etc/apt/sources.list.d/docker.list"
 
         exec_obj.run(f"mkdir -p {keyrings_dir}", force_sudo=True)
 
         if not os.path.exists(docker_gpg_path):
             log.info(f"Downloading and adding Docker GPG key for {os_id}.")
-            # Note: We use gpg --dearmor to ensure a binary .gpg file for /etc/apt/keyrings compatibility
-            curl_cmd = f"curl -fsSL https://download.docker.com/linux/{os_id}/gpg | gpg --dearmor -o {docker_gpg_path}"
+            curl_cmd = (
+                f"curl -fsSL https://download.docker.com/linux/{os_id}/gpg "
+                f"| gpg --dearmor -o {docker_gpg_path}"
+            )
             exec_obj.run(curl_cmd, force_sudo=True)
             # Ensure proper read permissions for apt
             exec_obj.run(f"chmod a+r {docker_gpg_path}", force_sudo=True)
@@ -122,7 +131,10 @@ def install_docker_and_add_users(exec_obj: Executor, *users_to_add: str) -> None
             display_arch = arch
 
         # 2. Interpolate the correct ID, codename and arch into the repository line
-        repo_line = f"deb [arch={display_arch} signed-by={docker_gpg_path}] https://download.docker.com/linux/{os_id} {codename} stable"
+        repo_line = (
+            f"deb [arch={display_arch} signed-by={docker_gpg_path}]"
+            f" https://download.docker.com/linux/{os_id} {codename} stable"
+        )
 
         log.info(f"Using APT repository line: {repo_line}")
         ensure_apt_repo(exec_obj, list_file, repo_line)
@@ -144,7 +156,7 @@ def install_docker_and_add_users(exec_obj: Executor, *users_to_add: str) -> None
 
 def _verify_docker_installation(exec_obj: Executor) -> None:
     """
-    Runs a simple Docker command (like 'docker run hello-world') and cleans up the resulting image/container.
+    Runs 'docker info' and 'docker run hello-world' to verify installation, then cleans up.
     """
     log.info("Running post-installation verification test...")
 
@@ -212,7 +224,12 @@ def check_docker_volume_exists(exec_obj: Executor, volume_name: str) -> bool:
     log.info(f"Checking for existence of Docker volume: {volume_name}")
     try:
         # Use 'docker volume ls -q -f name=...' to check existence silently
-        result = exec_obj.run(["docker", "volume", "ls", "-q", "-f", f"name=^{volume_name}$"], check=True, force_sudo=True, run_quiet=True)
+        result = exec_obj.run(
+            ["docker", "volume", "ls", "-q", "-f", f"name=^{volume_name}$"],
+            check=True,
+            force_sudo=True,
+            run_quiet=True,
+        )
         if result.stdout.strip() == volume_name:
             log.success(f"Docker volume '{volume_name}' exists.")
             return True
@@ -222,7 +239,9 @@ def check_docker_volume_exists(exec_obj: Executor, volume_name: str) -> bool:
         log.error(f"Error checking Docker volumes: {e.stderr}")
         return False
 
-def are_docker_services_running(exec_obj: Executor, user: str, cwd: str, service_names: List[str]) -> bool:
+def are_docker_services_running(
+    exec_obj: Executor, user: str, cwd: str, service_names: List[str]
+) -> bool:
     """
     Checks if a list of specific Docker Compose services are currently in the 'running' state.
     Requires running as the user that owns the compose stack.
@@ -255,7 +274,7 @@ def are_docker_services_running(exec_obj: Executor, user: str, cwd: str, service
         return all_running
 
     except subprocess.CalledProcessError as e:
-        log.warning(f"Failed to execute 'docker compose ps'. Stack may not exist.")
+        log.warning("Failed to execute 'docker compose ps'. Stack may not exist.")
         log.debug(f"PS error: {e.stderr}")
         return False
     except Exception as e: