fix: avoid shell parsing in docker cleanup

[PrAndrian]

What does this PR do?

This PR removes shell parsing from Docker container cleanup.

DockerEnvironment.cleanup() previously built shell command strings and ran them with shell=True to stop and remove containers. That made configured Docker binary paths and container IDs pass through a shell unnecessarily.

This changes cleanup to use argv form subprocess calls instead:

• docker stop --time 60 <container_id>
• docker rm -f <container_id>

This is safer, clearer, and matches the rest of the Docker environment code style.

Related Issue

No issue opened.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tools/environments/docker.py

  • Replaced shell-string cleanup commands with argv-form subprocess.run(...)
  • Removed shell=True usage from Docker cleanup
  • Captured docker_exe, container_id, and persistent before clearing _container_id
  • Kept cleanup asynchronous via a non-daemon worker thread
  • Added fallback to synchronous cleanup if thread creation fails

• tests/tools/test_docker_environment.py

  • Updated the non-persistent cleanup test to assert argv-form Docker cleanup calls

• tests/tools/test_docker_environment_cleanup.py

  • Added regression tests for cleanup command construction
  • Covered Docker paths and container IDs containing shell metacharacters
  • Verified cleanup does not pass shell=True

How to Test

1. Check out this branch:

git checkout fix/docker-cleanup-no-shell

2. Run the targeted Docker environment tests:

3. Confirm the result: 35 passed

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Ubuntu 24.04 / Linux

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — N/A
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide
• I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Targeted tests pass:

scripts/run_tests.sh tests/tools/test_docker_environment.py tests/tools/test_docker_find.py tests/tools/test_docker_environment_cleanup.py
35 passed

Full suite was attempted but did not pass in this container environment:
scripts/run_tests.sh tests/

8 tests failed across 4 unrelated areas:

• gateway service tests
• TUI npm install test
• browser hardening test
• browser Homebrew paths test

[alt-glitch]

Duplicate of #29189 (open) which already replaces shell=True Popen in DockerEnvironment.cleanup() with argv-list subprocess.run. Also overlaps with #3981 (open, same cleanup fix + non-root container changes). Related issue: #2743.

[keegoid-codex]

Bugs

• [CAT-1] Run rm -f after stop timeout; current exception path leaks containers at tools/environments/docker.py:640.

Severity

• high. Cleanup leak preserves untrusted nonpersistent containers after Docker stop hangs.

Verdict

VERDICT: request_changes

---

codex-review posting override: forced to --comment because reviewer lacks verified write permission (viewerPermission=READ; was --request-changes). GitHub only counts approvals from WRITE, MAINTAIN, or ADMIN reviewers.

[keegoid-cc]

[DEV SecOps] verdict: PASS
verified at f8a79f9

• Prompt-injection: no findings. Scanned PR title/body, commit headline/body, changed comments/string literals/test fixture text, and hidden Unicode controls; fixture strings abc; touch /tmp/pwn and /tmp/docker;evil are regression-test data for shell-metacharacter handling, not agent instructions.
• Dependency/lockfile risk: no findings. No dependency manifests or lockfiles changed.
• CI/workflow injection: no findings. No workflow/CI files changed.
• Generated/copied provenance: no findings. New 87-line test file is small, focused regression coverage; no large copied/generated blob or opaque payload.
• Secret exposure: no findings. Static diff scan found no credential-looking assignments or private-key material.
• Ops/security regression: no findings. tools/environments/docker.py cleanup now uses argv-form subprocess.run([docker_exe, ...]) with shell=True removed; old shell-string subprocess.Popen(..., shell=True) paths are removed. Residual risk: review is static/read-only; I did not execute PR code or tests.

[AxDSan]

Hey @PrAndrian, this is a clean security fix but unfortunately it's a duplicate of #29189 (same shell=True -> argv fix in DockerEnvironment.cleanup(), opened first). Appreciate the eye for shell injection vectors. Would love to see you tackle a fresh issue.

[benbarclay]

Closing — duplicate of #29189 which is also open, mergeable, and applies the same shell=True → argv-form fix to DockerEnvironment.cleanup(). #29189 was filed 2 days earlier (2026-05-20 vs 2026-05-22) and is the canonical PR for this fix.

Your version's regression test fixtures (shell-metacharacter container IDs like abc; touch /tmp/pwn and /tmp/docker;evil) are particularly nice — happy to pull those into #29189 as a follow-up if the test coverage there doesn't already exercise that case.

The underlying bug is still real (verified: grep -n shell=True tools/environments/docker.py returns lines 641 and 650 on current main), so #29189 will land. Sorry this one got crossed in flight — appreciate the careful work, including the non-daemon-thread fallback for interpreter-shutdown safety.