address second round of review feedback

- docker rm without -f to avoid killing running containers on transient failures
- use _DOCKER_TIMEOUT constant for docker run
- assert full docker inspect argv in reuse test

Author: anticomputer

src/seclab_taskflows/mcp_servers/container_shell.py

@@ -67,20 +67,24 @@ def _is_running(name: str) -> bool:
 
 
 def _remove_container(name: str) -> None:
-    """Remove a stopped container by name. Logs failures for diagnostics."""
+    """Remove a stopped container by name. Logs failures for diagnostics.
+
+    Uses ``docker rm`` (without -f) so that running containers are NOT
+    killed — only genuinely stopped leftovers are cleaned up.
+    """
     try:
         result = subprocess.run(
-            ["docker", "rm", "-f", name],
+            ["docker", "rm", name],
             capture_output=True,
             text=True,
             timeout=_DOCKER_TIMEOUT,
         )
         if result.returncode != 0:
-            logging.warning(
-                "docker rm -f failed for %s: %s", name, result.stderr.strip()
+            logging.debug(
+                "docker rm skipped for %s: %s", name, result.stderr.strip()
             )
     except subprocess.TimeoutExpired:
-        logging.error("docker rm -f timed out for %s after %ds", name, _DOCKER_TIMEOUT)
+        logging.error("docker rm timed out for %s after %ds", name, _DOCKER_TIMEOUT)
 
 
 def _start_container() -> str:
@@ -109,7 +113,7 @@ def _start_container() -> str:
         cmd += ["-v", f"{CONTAINER_WORKSPACE}:/workspace"]
     cmd += [CONTAINER_IMAGE, "tail", "-f", "/dev/null"]
     logging.debug(f"Starting container: {' '.join(cmd)}")
-    result = subprocess.run(cmd, capture_output=True, text=True, timeout=30)
+    result = subprocess.run(cmd, capture_output=True, text=True, timeout=_DOCKER_TIMEOUT)
     if result.returncode != 0:
         msg = f"docker run failed: {result.stderr.strip()}"
         raise RuntimeError(msg)

tests/test_container_shell.py

@@ -235,7 +235,8 @@ def test_start_reuses_running_persistent_container(self):
             assert name.startswith("seclab-persist-")
             # Only docker inspect should be called, NOT docker run
             assert mock_run.call_count == 1
-            assert "inspect" in mock_run.call_args[0][0]
+            cmd = mock_run.call_args[0][0]
+            assert cmd == ["docker", "inspect", "--format", "json", name]
 
     def test_start_persistent_no_rm_flag(self):
         inspect_proc = _make_proc(
@@ -269,9 +270,9 @@ def test_stop_skips_persistent_container(self):
 
     def test_remove_container_logs_failure(self):
         with patch("subprocess.run", return_value=_make_proc(returncode=1, stderr="conflict")):
-            with patch.object(cs_mod.logging, "warning") as mock_warn:
+            with patch.object(cs_mod.logging, "debug") as mock_debug:
                 cs_mod._remove_container("test-name")
-                mock_warn.assert_called_once()
+                mock_debug.assert_called_once()
 
     def test_remove_container_logs_timeout(self):
         with patch("subprocess.run", side_effect=subprocess.TimeoutExpired(cmd="docker", timeout=30)):