Address PR review feedback on container shell toolbox

- Rename _container_id → _container_name throughout (it stores the name
  set via --name, not the Docker-assigned container ID)
- Add empty-image guard in _start_container: raise clear RuntimeError
  when CONTAINER_IMAGE is not set rather than passing an empty string
  to docker run
- Add 30s timeout to docker run subprocess call in _start_container
- Log warning in _stop_container when docker stop fails instead of
  silently ignoring a non-zero returncode
- Default _DEFAULT_WORKDIR to /workspace unconditionally (all images
  set WORKDIR /workspace; the previous "/" fallback when no workspace
  was mounted was inconsistent with the container image defaults)
- Add SPDX headers to container_shell.py, test_container_shell.py,
  and all three Dockerfiles that were missing them
- Remove unused importlib import from test_container_shell.py
- Fix dead sast workspace existence check in run_container_shell_demo.sh
  (mkdir -p always creates workspace so the old condition was never true;
  now checks the actual target path when a specific target is provided)
- Update build_container_images.sh usage comment to include sast
- Clarify malware analysis toolbox prompt: /workspace is bind-mounted
  RW from the host, not an isolated environment
- Update README CONTAINER_TIMEOUT defaults to mention sast profile (60s)
- Add test_start_container_rejects_empty_image and
  test_stop_container_clears_name_on_failure test cases

Author: anticomputer

scripts/build_container_images.sh

@@ -6,7 +6,7 @@
 # Must be run from the root of the seclab-taskflows repository.
 # Images must be rebuilt whenever a Dockerfile changes.
 #
-# Usage: ./scripts/build_container_images.sh [base|malware|network|all]
+# Usage: ./scripts/build_container_images.sh [base|malware|network|sast|all]
 #   default: all
 
 set -euo pipefail

scripts/run_container_shell_demo.sh

@@ -75,8 +75,9 @@ case "$demo" in
         ;;
     sast)
         target="${3:-.}"
-        if [ ! -d "$workspace" ] && [ ! -f "${workspace}/${target}" ]; then
-            echo "No source found at ${workspace}/${target}" >&2
+        target_path="${workspace}/${target}"
+        if [ "$target" != "." ] && [ ! -d "$target_path" ] && [ ! -f "$target_path" ]; then
+            echo "No source found at ${target_path}" >&2
             echo "Provide a source directory or file in workspace_dir." >&2
             exit 1
         fi

src/seclab_taskflows/containers/base/Dockerfile

@@ -1,3 +1,6 @@
+# SPDX-FileCopyrightText: GitHub, Inc.
+# SPDX-License-Identifier: MIT
+
 FROM debian:bookworm-slim
 RUN apt-get update && apt-get install -y --no-install-recommends \
     bash coreutils python3 python3-pip curl wget git ca-certificates \

src/seclab_taskflows/containers/malware_analysis/Dockerfile

@@ -1,3 +1,6 @@
+# SPDX-FileCopyrightText: GitHub, Inc.
+# SPDX-License-Identifier: MIT
+
 FROM seclab-shell-base:latest
 RUN apt-get update && apt-get install -y --no-install-recommends \
     binwalk yara libimage-exiftool-perl checksec \

src/seclab_taskflows/containers/network_analysis/Dockerfile

@@ -1,3 +1,6 @@
+# SPDX-FileCopyrightText: GitHub, Inc.
+# SPDX-License-Identifier: MIT
+
 FROM seclab-shell-base:latest
 RUN apt-get update && apt-get install -y --no-install-recommends \
     nmap tcpdump tshark netcat-openbsd dnsutils curl jq httpie \

src/seclab_taskflows/mcp_servers/container_shell.py

@@ -1,3 +1,6 @@
+# SPDX-FileCopyrightText: GitHub, Inc.
+# SPDX-License-Identifier: MIT
+
 import atexit
 import logging
 import os
@@ -18,15 +21,19 @@
 
 mcp = FastMCP("ContainerShell")
 
-_container_id: str | None = None
+_container_name: str | None = None
 
 CONTAINER_IMAGE = os.environ.get("CONTAINER_IMAGE", "")
 CONTAINER_WORKSPACE = os.environ.get("CONTAINER_WORKSPACE", "")
 CONTAINER_TIMEOUT = int(os.environ.get("CONTAINER_TIMEOUT", "30"))
 
+_DEFAULT_WORKDIR = "/workspace"
+
 
 def _start_container() -> str:
     """Start the Docker container and return its name."""
+    if not CONTAINER_IMAGE:
+        raise RuntimeError("CONTAINER_IMAGE is not set — cannot start container")
     if CONTAINER_WORKSPACE and ":" in CONTAINER_WORKSPACE:
         raise RuntimeError(f"CONTAINER_WORKSPACE must not contain a colon: {CONTAINER_WORKSPACE!r}")
     name = f"seclab-shell-{uuid.uuid4().hex[:8]}"
@@ -35,7 +42,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)
+    result = subprocess.run(cmd, capture_output=True, text=True, timeout=30)
     if result.returncode != 0:
         raise RuntimeError(f"docker run failed: {result.stderr.strip()}")
     logging.debug(f"Container started: {name}")
@@ -44,39 +51,42 @@ def _start_container() -> str:
 
 def _stop_container() -> None:
     """Stop the running container."""
-    global _container_id
-    if _container_id is None:
+    global _container_name
+    if _container_name is None:
         return
-    logging.debug(f"Stopping container: {_container_id}")
-    subprocess.run(
-        ["docker", "stop", "--time", "5", _container_id],
+    logging.debug(f"Stopping container: {_container_name}")
+    result = subprocess.run(
+        ["docker", "stop", "--time", "5", _container_name],
         capture_output=True,
         text=True,
     )
-    _container_id = None
+    if result.returncode != 0:
+        logging.warning(
+            "docker stop failed for container %s: %s",
+            _container_name,
+            result.stderr.strip(),
+        )
+    _container_name = None
 
 
 atexit.register(_stop_container)
 
 
-_DEFAULT_WORKDIR = "/workspace" if CONTAINER_WORKSPACE else "/"
-
-
 @mcp.tool()
 def shell_exec(
     command: Annotated[str, Field(description="Shell command to execute inside the container")],
     timeout: Annotated[int, Field(description="Timeout in seconds")] = CONTAINER_TIMEOUT,
     workdir: Annotated[str, Field(description="Working directory inside the container")] = _DEFAULT_WORKDIR,
 ) -> str:
     """Execute a shell command inside the managed Docker container."""
-    global _container_id
-    if _container_id is None:
+    global _container_name
+    if _container_name is None:
         try:
-            _container_id = _start_container()
+            _container_name = _start_container()
         except RuntimeError as e:
             return f"Failed to start container: {e}"
 
-    cmd = ["docker", "exec", "-w", workdir, _container_id, "bash", "-c", command]
+    cmd = ["docker", "exec", "-w", workdir, _container_name, "bash", "-c", command]
     logging.debug(f"Executing: {' '.join(cmd)}")
     try:
         result = subprocess.run(cmd, capture_output=True, text=True, timeout=timeout)

src/seclab_taskflows/taskflows/container_shell/README.md

@@ -50,7 +50,7 @@ Images only need to be rebuilt when a Dockerfile changes.
 you do not need to pass files into the container.
 
 `CONTAINER_TIMEOUT` — default command timeout in seconds. Defaults to 30 (base
-and network) or 60 (malware analysis).
+and network) or 60 (malware analysis and sast).
 
 `LOG_DIR` — where to write `container_shell.log`.
 

src/seclab_taskflows/toolboxes/container_shell_malware_analysis.yaml

@@ -22,7 +22,8 @@ server_prompt: |
   ## Container Shell (malware analysis)
   You have access to an isolated Docker container for binary and malware analysis.
   The working directory is /workspace — place samples here before analysis.
-  ALL tooling runs inside the container, keeping the host safe.
+  ALL tooling runs inside the container. Note: /workspace is bind-mounted from
+  the host, so files written there are visible on the host side as well.
 
   Available tools:
   - file, strings, xxd, hexdump  — initial triage / string extraction

tests/test_container_shell.py

@@ -1,4 +1,6 @@
-import importlib
+# SPDX-FileCopyrightText: GitHub, Inc.
+# SPDX-License-Identifier: MIT
+
 import subprocess
 from unittest.mock import MagicMock, patch
 
@@ -22,7 +24,7 @@ def _make_proc(returncode=0, stdout="", stderr=""):
 
 def _reset_container():
     """Reset global container state between tests."""
-    cs_mod._container_id = None
+    cs_mod._container_name = None
 
 
 # ---------------------------------------------------------------------------
@@ -78,6 +80,14 @@ def test_start_container_rejects_colon_in_workspace(self):
             with pytest.raises(RuntimeError, match="CONTAINER_WORKSPACE must not contain a colon"):
                 cs_mod._start_container()
 
+    def test_start_container_rejects_empty_image(self):
+        with (
+            patch.object(cs_mod, "CONTAINER_IMAGE", ""),
+            patch.object(cs_mod, "CONTAINER_WORKSPACE", ""),
+        ):
+            with pytest.raises(RuntimeError, match="CONTAINER_IMAGE is not set"):
+                cs_mod._start_container()
+
 
 # ---------------------------------------------------------------------------
 # shell_exec tests
@@ -95,13 +105,13 @@ def test_shell_exec_lazy_start(self):
             patch.object(cs_mod, "CONTAINER_WORKSPACE", ""),
             patch("subprocess.run", side_effect=[start_proc, exec_proc]),
         ):
-            assert cs_mod._container_id is None
+            assert cs_mod._container_name is None
             result = cs_mod.shell_exec.fn(command="echo hello")
-            assert cs_mod._container_id is not None
+            assert cs_mod._container_name is not None
             assert "hello" in result
 
     def test_shell_exec_runs_command(self):
-        cs_mod._container_id = "seclab-shell-testtest"
+        cs_mod._container_name = "seclab-shell-testtest"
         exec_proc = _make_proc(returncode=0, stdout="output\n")
         with patch("subprocess.run", return_value=exec_proc) as mock_run:
             result = cs_mod.shell_exec.fn(command="echo output", workdir="/workspace")
@@ -115,22 +125,22 @@ def test_shell_exec_runs_command(self):
             assert "output" in result
 
     def test_shell_exec_includes_exit_code(self):
-        cs_mod._container_id = "seclab-shell-testtest"
+        cs_mod._container_name = "seclab-shell-testtest"
         exec_proc = _make_proc(returncode=0, stdout="done\n")
         with patch("subprocess.run", return_value=exec_proc):
             result = cs_mod.shell_exec.fn(command="true")
             assert "[exit code: 0]" in result
 
     def test_shell_exec_nonzero_exit(self):
-        cs_mod._container_id = "seclab-shell-testtest"
+        cs_mod._container_name = "seclab-shell-testtest"
         exec_proc = _make_proc(returncode=1, stdout="", stderr="error\n")
         with patch("subprocess.run", return_value=exec_proc):
             result = cs_mod.shell_exec.fn(command="false")
             assert "[exit code: 1]" in result
             assert "error" in result
 
     def test_shell_exec_timeout(self):
-        cs_mod._container_id = "seclab-shell-testtest"
+        cs_mod._container_name = "seclab-shell-testtest"
         with patch("subprocess.run", side_effect=subprocess.TimeoutExpired(cmd="docker", timeout=5)):
             result = cs_mod.shell_exec.fn(command="sleep 999", timeout=5)
             assert "timeout" in result
@@ -144,7 +154,7 @@ def test_shell_exec_start_failure_returns_error(self):
         ):
             result = cs_mod.shell_exec.fn(command="echo hi")
             assert "Failed to start container" in result
-            assert cs_mod._container_id is None
+            assert cs_mod._container_name is None
 
 
 # ---------------------------------------------------------------------------
@@ -156,21 +166,27 @@ def setup_method(self):
         _reset_container()
 
     def test_stop_container_called_on_atexit(self):
-        cs_mod._container_id = "seclab-shell-tostop"
+        cs_mod._container_name = "seclab-shell-tostop"
         with patch("subprocess.run", return_value=_make_proc(returncode=0)) as mock_run:
             cs_mod._stop_container()
             cmd = mock_run.call_args[0][0]
             assert "docker" in cmd
             assert "stop" in cmd
             assert "seclab-shell-tostop" in cmd
-            assert cs_mod._container_id is None
+            assert cs_mod._container_name is None
 
     def test_stop_container_no_op_when_none(self):
-        cs_mod._container_id = None
+        cs_mod._container_name = None
         with patch("subprocess.run") as mock_run:
             cs_mod._stop_container()
             mock_run.assert_not_called()
 
+    def test_stop_container_clears_name_on_failure(self):
+        cs_mod._container_name = "seclab-shell-tostop"
+        with patch("subprocess.run", return_value=_make_proc(returncode=1, stderr="not found")):
+            cs_mod._stop_container()
+            assert cs_mod._container_name is None
+
 
 # ---------------------------------------------------------------------------
 # Toolbox YAML validation