GRM-32: fix: security, dead code, idempotence, and documentation cleanup

tests/unit/test_runner_manager.py

@@ -1,5 +1,8 @@
 """Unit tests for runner_manager module."""
 
+import json
+import os
+from contextlib import contextmanager
 from pathlib import Path
 from unittest.mock import MagicMock, patch
 
@@ -9,7 +12,20 @@ from gitea_runner_manager.exceptions import AnsibleError
 from gitea_runner_manager.runner_manager import RunnerManager
 
 
+@contextmanager
+def _capture_extra_vars(self, extra_vars: dict[str, str | int] | None):
+    """Capture extra_vars dict for inspection instead of writing to file."""
+    self._captured_extra_vars = extra_vars
+    yield "/tmp/fake-vars.json" if extra_vars else None
+
+
 class TestRunnerManager:
+    @pytest.fixture(autouse=True)
+    def _patch_extra_vars_file(self) -> None:
+        """Patch ``_extra_vars_file`` so no real temp files are created."""
+        with patch.object(RunnerManager, "_extra_vars_file", _capture_extra_vars):
+            yield
+
     def test_init(self) -> None:
         manager = RunnerManager()
         assert manager is not None
@@ -30,9 +46,11 @@ class TestRunnerManager:
         assert "192.168.1.10," in cmd_str
         assert "-u" in cmd_str
         assert "ubuntu" in cmd_str
-        assert "registration_token=tok" in cmd_str
-        assert "runner_name=192.168.1.10" in cmd_str
-        assert "gitea_url=https://git.example.com" in cmd_str
+        assert "--extra-vars" in cmd_str
+        assert "@/tmp/fake-vars.json" in cmd_str
+        assert manager._captured_extra_vars["registration_token"] == "tok"
+        assert manager._captured_extra_vars["runner_name"] == "192.168.1.10"
+        assert manager._captured_extra_vars["gitea_url"] == "https://git.example.com"
         assert "Installing Gitea Runner on 192.168.1.10" in mock_executor.run.call_args.kwargs["description"]
         mock_registry.add.assert_called_once_with(
             name="192.168.1.10",
@@ -56,8 +74,8 @@ class TestRunnerManager:
         cmd_str = " ".join(cmd)
         assert "--private-key" in cmd_str
         assert "/key" in cmd_str
-        assert "registration_token=preset" in cmd_str
-        assert "runner_name=my-runner" in cmd_str
+        assert manager._captured_extra_vars["registration_token"] == "preset"
+        assert manager._captured_extra_vars["runner_name"] == "my-runner"
         assert "--ask-become-pass" not in cmd_str
         mock_registry.add.assert_called_once_with(
             name="my-runner",
@@ -108,9 +126,7 @@ class TestRunnerManager:
             gitea_url="https://git.example.com",
             labels="docker:docker://alpine:latest",
         )
-        cmd = mock_executor.run.call_args.args[0]
-        cmd_str = " ".join(cmd)
-        assert "runner_labels=docker:docker://alpine:latest" in cmd_str
+        assert manager._captured_extra_vars["runner_labels"] == "docker:docker://alpine:latest"
 
     def test_install_no_labels(self) -> None:
         mock_registry = MagicMock()
@@ -119,9 +135,7 @@ class TestRunnerManager:
         manager._executor = mock_executor
 
         manager.install("host1", "root", token="tok", gitea_url="https://git.example.com")
-        cmd = mock_executor.run.call_args.args[0]
-        cmd_str = " ".join(cmd)
-        assert "runner_labels" not in cmd_str
+        assert "runner_labels" not in manager._captured_extra_vars
 
     def test_install_with_admin_token(self) -> None:
         mock_registry = MagicMock()
@@ -130,9 +144,7 @@ class TestRunnerManager:
         manager._executor = mock_executor
 
         manager.install("host1", "root", token="tok", gitea_url="https://git.example.com", admin_token="admin-tok")
-        cmd = mock_executor.run.call_args.args[0]
-        cmd_str = " ".join(cmd)
-        assert "gitea_admin_token=admin-tok" in cmd_str
+        assert manager._captured_extra_vars["gitea_admin_token"] == "admin-tok"
 
     def test_install_ansible_failure(self) -> None:
         mock_registry = MagicMock()
@@ -155,7 +167,7 @@ class TestRunnerManager:
         assert "update-runner.yml" in cmd_str
         assert "--private-key" in cmd_str
         assert "/key" in cmd_str
-        assert "gitea_runner_version=v0.2.0" in cmd_str
+        assert manager._captured_extra_vars["gitea_runner_version"] == "v0.2.0"
         assert "--ask-become-pass" not in cmd_str
         assert "Updating Gitea Runner on host" in mock_executor.run.call_args.kwargs["description"]
 
@@ -168,6 +180,7 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "--ask-become-pass" in cmd_str
+        assert "--extra-vars" not in cmd_str
 
     def test_update_playbook_not_found(self) -> None:
         manager = RunnerManager()
@@ -242,7 +255,7 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "start-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
         assert "Starting Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]
 
     def test_start_with_override(self) -> None:
@@ -281,7 +294,7 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "stop-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
         assert "Stopping Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]
 
     def test_enable(self) -> None:
@@ -295,7 +308,7 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "enable-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
         assert "Enabling Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]
 
     def test_disable(self) -> None:
@@ -314,9 +327,9 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "disable-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
-        assert "registration_token=tok" in cmd_str
-        assert "gitea_url=https://git.example.com" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
+        assert manager._captured_extra_vars["registration_token"] == "tok"
+        assert manager._captured_extra_vars["gitea_url"] == "https://git.example.com"
         assert "Disabling Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]
 
     def test_disable_uses_registry_gitea_url(self) -> None:
@@ -332,9 +345,7 @@ class TestRunnerManager:
         manager._executor = mock_executor
 
         manager.disable("r1", token="tok")
-        cmd = mock_executor.run.call_args.args[0]
-        cmd_str = " ".join(cmd)
-        assert "gitea_url=https://registry.example.com" in cmd_str
+        assert manager._captured_extra_vars["gitea_url"] == "https://registry.example.com"
 
     def test_disable_missing_token(self) -> None:
         mock_registry = MagicMock()
@@ -361,7 +372,7 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "status-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
         assert "Checking status of Gitea Runner r1 on host" in mock_executor.run.call_args.kwargs["description"]
 
     def test_remove(self) -> None:
@@ -380,9 +391,9 @@ class TestRunnerManager:
         cmd = mock_executor.run.call_args.args[0]
         cmd_str = " ".join(cmd)
         assert "remove-runner.yml" in cmd_str
-        assert "runner_name=r1" in cmd_str
-        assert "registration_token=tok" in cmd_str
-        assert "gitea_url=https://git.example.com" in cmd_str
+        assert manager._captured_extra_vars["runner_name"] == "r1"
+        assert manager._captured_extra_vars["registration_token"] == "tok"
+        assert manager._captured_extra_vars["gitea_url"] == "https://git.example.com"
         assert "Removing Gitea Runner r1 from host" in mock_executor.run.call_args.kwargs["description"]
         mock_registry.remove.assert_called_once_with("r1")
 
@@ -538,33 +549,72 @@ class TestRunnerManager:
         assert runners[0]["status"] == "unknown"
 
 
+class TestExtraVarsFile:
+    """Tests for the ``_extra_vars_file`` context manager."""
+
+    def test_writes_temp_file_with_content(self) -> None:
+        manager = RunnerManager()
+        with manager._extra_vars_file({"foo": "bar", "count": 3}) as path:
+            assert path is not None
+            assert path.endswith(".json")
+            with open(path) as f:
+                data = json.load(f)
+            assert data == {"foo": "bar", "count": 3}
+            mode = os.stat(path).st_mode & 0o777
+            assert mode == 0o600
+        assert not os.path.exists(path)
+
+    def test_none_yields_none(self) -> None:
+        manager = RunnerManager()
+        with manager._extra_vars_file(None) as path:
+            assert path is None
+
+    def test_empty_dict_yields_none(self) -> None:
+        manager = RunnerManager()
+        with manager._extra_vars_file({}) as path:
+            assert path is None
+
+    def test_cleans_up_even_if_file_deleted(self) -> None:
+        manager = RunnerManager()
+        with manager._extra_vars_file({"foo": "bar"}) as path:
+            os.unlink(path)
+        # Should not raise despite missing file on cleanup.
+
+
 class TestBuildCmd:
     def test_build_cmd_basic(self) -> None:
         manager = RunnerManager()
         with patch.object(Path, "exists", return_value=True):
-            cmd = manager._build_cmd("test.yml", "host1", "user1", "foo=bar")
+            cmd = manager._build_cmd("test.yml", "host1", "user1", "/tmp/vars.json")
         cmd_str = " ".join(cmd)
         assert "ansible-playbook" in cmd_str
         assert "test.yml" in cmd_str
         assert "host1," in cmd_str
         assert "user1" in cmd_str
-        assert "foo=bar" in cmd_str
+        assert "--extra-vars" in cmd_str
+        assert "@/tmp/vars.json" in cmd_str
 
     def test_build_cmd_with_key(self) -> None:
         manager = RunnerManager()
         with patch.object(Path, "exists", return_value=True):
-            cmd = manager._build_cmd("test.yml", "host1", "user1", "foo=bar", key="/key")
+            cmd = manager._build_cmd("test.yml", "host1", "user1", "/tmp/vars.json", key="/key")
         assert "--private-key" in cmd
         assert "/key" in cmd
 
     def test_build_cmd_ask_become_pass(self) -> None:
         manager = RunnerManager()
         with patch.object(Path, "exists", return_value=True):
-            cmd = manager._build_cmd("test.yml", "host1", "user1", "foo=bar", ask_become_pass=True)
+            cmd = manager._build_cmd("test.yml", "host1", "user1", "/tmp/vars.json", ask_become_pass=True)
         assert "--ask-become-pass" in cmd
 
+    def test_build_cmd_no_extra_vars(self) -> None:
+        manager = RunnerManager()
+        with patch.object(Path, "exists", return_value=True):
+            cmd = manager._build_cmd("test.yml", "host1", "user1")
+        assert "--extra-vars" not in cmd
+
     def test_build_cmd_playbook_not_found(self) -> None:
         manager = RunnerManager()
         with patch.object(Path, "exists", return_value=False):
             with pytest.raises(AnsibleError, match="Playbook not found"):
-                manager._build_cmd("missing.yml", "host1", "user1", "foo=bar")
+                manager._build_cmd("missing.yml", "host1", "user1", "/tmp/vars.json")