fix(docker): remove container on cleanup when container_persistent=false
When container_persistent=false, the inner mini-swe-agent cleanup only runs 'docker stop' in the background, leaving containers in Exited state. Now cleanup() also runs 'docker rm -f' to fully remove the container. Also fixes pre-existing test failures in model_metadata (gpt-4.1 1M context), setup tests (TTS provider step), and adds MockInnerDocker.cleanup(). Original fix by crazywriter1. Cherry-picked and adapted for current main. Fixes #1679
This commit is contained in:
parent
f613da4219
commit
7049dba778
5 changed files with 64 additions and 4 deletions
|
|
@ -110,10 +110,14 @@ class TestDefaultContextLengths:
|
||||||
if "claude" in key:
|
if "claude" in key:
|
||||||
assert value == 200000, f"{key} should be 200000"
|
assert value == 200000, f"{key} should be 200000"
|
||||||
|
|
||||||
def test_gpt4_models_128k(self):
|
def test_gpt4_models_128k_or_1m(self):
|
||||||
|
# gpt-4.1 and gpt-4.1-mini have 1M context; other gpt-4* have 128k
|
||||||
for key, value in DEFAULT_CONTEXT_LENGTHS.items():
|
for key, value in DEFAULT_CONTEXT_LENGTHS.items():
|
||||||
if "gpt-4" in key:
|
if "gpt-4" in key:
|
||||||
assert value == 128000, f"{key} should be 128000"
|
if "gpt-4.1" in key:
|
||||||
|
assert value == 1047576, f"{key} should be 1047576 (1M)"
|
||||||
|
else:
|
||||||
|
assert value == 128000, f"{key} should be 128000"
|
||||||
|
|
||||||
def test_gemini_models_1m(self):
|
def test_gemini_models_1m(self):
|
||||||
for key, value in DEFAULT_CONTEXT_LENGTHS.items():
|
for key, value in DEFAULT_CONTEXT_LENGTHS.items():
|
||||||
|
|
|
||||||
|
|
@ -53,6 +53,7 @@ def test_nous_oauth_setup_keeps_current_model_when_syncing_disk_provider(
|
||||||
"hermes_cli.auth.fetch_nous_models",
|
"hermes_cli.auth.fetch_nous_models",
|
||||||
lambda *args, **kwargs: ["gemini-3-flash"],
|
lambda *args, **kwargs: ["gemini-3-flash"],
|
||||||
)
|
)
|
||||||
|
monkeypatch.setattr("hermes_cli.setup._setup_tts_provider", lambda config: None)
|
||||||
|
|
||||||
setup_model_provider(config)
|
setup_model_provider(config)
|
||||||
save_config(config)
|
save_config(config)
|
||||||
|
|
@ -88,6 +89,7 @@ def test_custom_setup_clears_active_oauth_provider(tmp_path, monkeypatch):
|
||||||
"hermes_cli.setup.prompt",
|
"hermes_cli.setup.prompt",
|
||||||
lambda *args, **kwargs: next(prompt_values),
|
lambda *args, **kwargs: next(prompt_values),
|
||||||
)
|
)
|
||||||
|
monkeypatch.setattr("hermes_cli.setup._setup_tts_provider", lambda config: None)
|
||||||
|
|
||||||
setup_model_provider(config)
|
setup_model_provider(config)
|
||||||
save_config(config)
|
save_config(config)
|
||||||
|
|
@ -135,6 +137,7 @@ def test_codex_setup_uses_runtime_access_token_for_live_model_list(tmp_path, mon
|
||||||
"hermes_cli.codex_models.get_codex_model_ids",
|
"hermes_cli.codex_models.get_codex_model_ids",
|
||||||
_fake_get_codex_model_ids,
|
_fake_get_codex_model_ids,
|
||||||
)
|
)
|
||||||
|
monkeypatch.setattr("hermes_cli.setup._setup_tts_provider", lambda config: None)
|
||||||
|
|
||||||
setup_model_provider(config)
|
setup_model_provider(config)
|
||||||
save_config(config)
|
save_config(config)
|
||||||
|
|
|
||||||
|
|
@ -62,6 +62,7 @@ def test_setup_keep_current_custom_from_config_does_not_fall_through(tmp_path, m
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: "")
|
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: "")
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
||||||
|
monkeypatch.setattr("hermes_cli.setup._setup_tts_provider", lambda config: None)
|
||||||
monkeypatch.setattr("hermes_cli.auth.get_active_provider", lambda: None)
|
monkeypatch.setattr("hermes_cli.auth.get_active_provider", lambda: None)
|
||||||
monkeypatch.setattr("hermes_cli.auth.detect_external_credentials", lambda: [])
|
monkeypatch.setattr("hermes_cli.auth.detect_external_credentials", lambda: [])
|
||||||
|
|
||||||
|
|
@ -86,6 +87,8 @@ def test_setup_custom_endpoint_saves_working_v1_base_url(tmp_path, monkeypatch):
|
||||||
return 3 # Custom endpoint
|
return 3 # Custom endpoint
|
||||||
if question == "Configure vision:":
|
if question == "Configure vision:":
|
||||||
return len(choices) - 1 # Skip
|
return len(choices) - 1 # Skip
|
||||||
|
if question == "Select TTS provider:":
|
||||||
|
return len(choices) - 1 # Keep current
|
||||||
raise AssertionError(f"Unexpected prompt_choice call: {question}")
|
raise AssertionError(f"Unexpected prompt_choice call: {question}")
|
||||||
|
|
||||||
def fake_prompt(message, current=None, **kwargs):
|
def fake_prompt(message, current=None, **kwargs):
|
||||||
|
|
@ -100,6 +103,7 @@ def test_setup_custom_endpoint_saves_working_v1_base_url(tmp_path, monkeypatch):
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt", fake_prompt)
|
monkeypatch.setattr("hermes_cli.setup.prompt", fake_prompt)
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
||||||
|
monkeypatch.setattr("hermes_cli.setup._setup_tts_provider", lambda config: None)
|
||||||
monkeypatch.setattr("hermes_cli.auth.get_active_provider", lambda: None)
|
monkeypatch.setattr("hermes_cli.auth.get_active_provider", lambda: None)
|
||||||
monkeypatch.setattr("hermes_cli.auth.detect_external_credentials", lambda: [])
|
monkeypatch.setattr("hermes_cli.auth.detect_external_credentials", lambda: [])
|
||||||
monkeypatch.setattr("agent.auxiliary_client.get_available_vision_backends", lambda: [])
|
monkeypatch.setattr("agent.auxiliary_client.get_available_vision_backends", lambda: [])
|
||||||
|
|
@ -155,6 +159,9 @@ def test_setup_keep_current_config_provider_uses_provider_specific_model_menu(tm
|
||||||
if calls["count"] == 3:
|
if calls["count"] == 3:
|
||||||
captured["model_choices"] = list(choices)
|
captured["model_choices"] = list(choices)
|
||||||
return len(choices) - 1 # keep current model
|
return len(choices) - 1 # keep current model
|
||||||
|
if calls["count"] == 4:
|
||||||
|
assert question == "Select TTS provider:"
|
||||||
|
return len(choices) - 1 # Keep current
|
||||||
raise AssertionError("Unexpected extra prompt_choice call")
|
raise AssertionError("Unexpected extra prompt_choice call")
|
||||||
|
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
monkeypatch.setattr("hermes_cli.setup.prompt_choice", fake_prompt_choice)
|
||||||
|
|
@ -172,7 +179,7 @@ def test_setup_keep_current_config_provider_uses_provider_specific_model_menu(tm
|
||||||
assert captured["model_choices"] is not None
|
assert captured["model_choices"] is not None
|
||||||
assert captured["model_choices"][0] == "claude-opus-4-6"
|
assert captured["model_choices"][0] == "claude-opus-4-6"
|
||||||
assert "anthropic/claude-opus-4.6 (recommended)" not in captured["model_choices"]
|
assert "anthropic/claude-opus-4.6 (recommended)" not in captured["model_choices"]
|
||||||
assert calls["count"] == 3
|
assert calls["count"] == 4 # provider, vision, model, TTS
|
||||||
|
|
||||||
|
|
||||||
def test_setup_keep_current_anthropic_can_configure_openai_vision_default(tmp_path, monkeypatch):
|
def test_setup_keep_current_anthropic_can_configure_openai_vision_default(tmp_path, monkeypatch):
|
||||||
|
|
@ -191,6 +198,7 @@ def test_setup_keep_current_anthropic_can_configure_openai_vision_default(tmp_pa
|
||||||
1, # configure vision with OpenAI
|
1, # configure vision with OpenAI
|
||||||
5, # use default gpt-4o-mini vision model
|
5, # use default gpt-4o-mini vision model
|
||||||
4, # keep current Anthropic model
|
4, # keep current Anthropic model
|
||||||
|
4, # TTS: Keep current
|
||||||
])
|
])
|
||||||
|
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: next(picks))
|
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: next(picks))
|
||||||
|
|
@ -229,7 +237,7 @@ def test_setup_switch_custom_to_codex_clears_custom_endpoint_and_updates_config(
|
||||||
}
|
}
|
||||||
save_config(config)
|
save_config(config)
|
||||||
|
|
||||||
picks = iter([1, 0])
|
picks = iter([1, 0, 4]) # provider, model; 4 = TTS Keep current
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: next(picks))
|
monkeypatch.setattr("hermes_cli.setup.prompt_choice", lambda *args, **kwargs: next(picks))
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: "")
|
monkeypatch.setattr("hermes_cli.setup.prompt", lambda *args, **kwargs: "")
|
||||||
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
monkeypatch.setattr("hermes_cli.setup.prompt_yes_no", lambda *args, **kwargs: False)
|
||||||
|
|
|
||||||
|
|
@ -17,6 +17,9 @@ def _install_fake_minisweagent(monkeypatch, captured_run_args):
|
||||||
def __init__(self, **kwargs):
|
def __init__(self, **kwargs):
|
||||||
captured_run_args.extend(kwargs.get("run_args", []))
|
captured_run_args.extend(kwargs.get("run_args", []))
|
||||||
|
|
||||||
|
def cleanup(self):
|
||||||
|
pass
|
||||||
|
|
||||||
minisweagent_mod = types.ModuleType("minisweagent")
|
minisweagent_mod = types.ModuleType("minisweagent")
|
||||||
environments_mod = types.ModuleType("minisweagent.environments")
|
environments_mod = types.ModuleType("minisweagent.environments")
|
||||||
docker_mod = types.ModuleType("minisweagent.environments.docker")
|
docker_mod = types.ModuleType("minisweagent.environments.docker")
|
||||||
|
|
@ -273,3 +276,31 @@ def test_execute_prefers_shell_env_over_hermes_dotenv(monkeypatch):
|
||||||
|
|
||||||
assert "GITHUB_TOKEN=value_from_shell" in popen_calls[0]
|
assert "GITHUB_TOKEN=value_from_shell" in popen_calls[0]
|
||||||
assert "GITHUB_TOKEN=value_from_dotenv" not in popen_calls[0]
|
assert "GITHUB_TOKEN=value_from_dotenv" not in popen_calls[0]
|
||||||
|
|
||||||
|
|
||||||
|
def test_non_persistent_cleanup_removes_container(monkeypatch):
|
||||||
|
"""When container_persistent=false, cleanup() must run docker rm -f so the container is removed (Fixes #1679)."""
|
||||||
|
run_calls = []
|
||||||
|
|
||||||
|
def _run(cmd, **kwargs):
|
||||||
|
run_calls.append((list(cmd) if isinstance(cmd, list) else cmd, kwargs))
|
||||||
|
if cmd and getattr(cmd[0], '__str__', None) and 'docker' in str(cmd[0]):
|
||||||
|
if len(cmd) >= 2 and cmd[1] == 'run':
|
||||||
|
return subprocess.CompletedProcess(cmd, 0, stdout="abc123container\n", stderr="")
|
||||||
|
return subprocess.CompletedProcess(cmd, 0, stdout='', stderr='')
|
||||||
|
|
||||||
|
monkeypatch.setattr(docker_env, 'find_docker', lambda: '/usr/bin/docker')
|
||||||
|
monkeypatch.setattr(docker_env.subprocess, 'run', _run)
|
||||||
|
monkeypatch.setattr(docker_env.subprocess, 'Popen', lambda *a, **k: type('P', (), {'poll': lambda: None, 'wait': lambda **kw: None, 'returncode': 0, 'stdout': iter([]), 'stdin': None})())
|
||||||
|
|
||||||
|
captured_run_args = []
|
||||||
|
_install_fake_minisweagent(monkeypatch, captured_run_args)
|
||||||
|
|
||||||
|
env = _make_dummy_env(persistent_filesystem=False, task_id='ephemeral-task')
|
||||||
|
assert env._container_id
|
||||||
|
container_id = env._container_id
|
||||||
|
|
||||||
|
env.cleanup()
|
||||||
|
|
||||||
|
rm_calls = [c for c in run_calls if isinstance(c[0], list) and len(c[0]) >= 4 and c[0][1:4] == ['rm', '-f', container_id]]
|
||||||
|
assert len(rm_calls) >= 1, 'cleanup() should run docker rm -f <container_id> when container_persistent=false'
|
||||||
|
|
|
||||||
|
|
@ -458,6 +458,20 @@ class DockerEnvironment(BaseEnvironment):
|
||||||
"""Stop and remove the container. Bind-mount dirs persist if persistent=True."""
|
"""Stop and remove the container. Bind-mount dirs persist if persistent=True."""
|
||||||
self._inner.cleanup()
|
self._inner.cleanup()
|
||||||
|
|
||||||
|
if not self._persistent and self._container_id:
|
||||||
|
# Inner cleanup only runs `docker stop` in background; container is left
|
||||||
|
# as stopped. When container_persistent=false we must remove it.
|
||||||
|
docker_exe = find_docker() or self._inner.config.executable
|
||||||
|
try:
|
||||||
|
subprocess.run(
|
||||||
|
[docker_exe, "rm", "-f", self._container_id],
|
||||||
|
capture_output=True,
|
||||||
|
timeout=30,
|
||||||
|
)
|
||||||
|
except Exception as e:
|
||||||
|
logger.warning("Failed to remove non-persistent container %s: %s", self._container_id, e)
|
||||||
|
self._container_id = None
|
||||||
|
|
||||||
if not self._persistent:
|
if not self._persistent:
|
||||||
import shutil
|
import shutil
|
||||||
for d in (self._workspace_dir, self._home_dir):
|
for d in (self._workspace_dir, self._home_dir):
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue