From 40fa0c1d19d5c24955e9b9c6b1f3c6c625d1f81a Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Fri, 29 May 2026 14:09:04 +1000 Subject: [PATCH] fix(docker): skip credential/skills/cache mounts when source is invalid (#24490) (#34331) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvages #24490 by @liuhao1024 against current main. The Docker daemon will silently auto-create a directory at the host path of any `-v :` bind mount when the host path doesn't exist. In Docker-in-Docker setups (where the outer host's real credential file isn't visible inside the agent's parent container), this leaves a directory at the credential mount source — and the inner `docker run` then refuses to mount a directory over a file destination with exit 125. Add defensive shape guards to all three mount loops in DockerEnvironment.__init__: * credentials (expected: file) — skip + warn on directory or missing * skills (expected: dir) — skip + warn when not a directory * cache (expected: dir) — skip + warn when not a directory Failed mounts surface as WARN logs rather than crashing the container start. Existing well-formed sources mount unchanged. The original PR's branch was on a pre-container-reuse-rework base (May 12) and conflicted with the post-May-28 driver work (label tagging, container reuse, orphan reaper). Reconstructed the same intent on current main; the three guard blocks slot cleanly into `tools/environments/docker.py` around the existing mount loops. Three new tests pinned in `tests/tools/test_docker_environment.py`: directory-source skip, missing-source skip, valid-file mounts. Test- first regression verification: reverted just the production code to `origin/main` and confirmed the new tests fail with `'deleted_token.json' is contained here: /root/.hermes/...` — the fixed code makes them pass. Full file passes (54/54). Closes #24490 Co-authored-by: liuhao1024 <11816344+liuhao1024@users.noreply.github.com> --- tests/tools/test_docker_environment.py | 117 +++++++++++++++++++++++++ tools/environments/docker.py | 31 +++++++ 2 files changed, 148 insertions(+) diff --git a/tests/tools/test_docker_environment.py b/tests/tools/test_docker_environment.py index 3598895a7b3..13eb08c9360 100644 --- a/tests/tools/test_docker_environment.py +++ b/tests/tools/test_docker_environment.py @@ -1368,3 +1368,120 @@ def test_container_finished_at_returns_none_on_zero_value(): ): result = docker_env._container_finished_at("/usr/bin/docker", "never-finished") assert result is None + + +def test_credential_mount_skipped_when_source_is_directory(monkeypatch, tmp_path, caplog): + """Credential mount should be skipped when source path is a directory. + + In Docker-in-Docker scenarios, Docker may auto-create the source path as + a directory when it doesn't exist on the host. Mounting a directory over + a file destination causes exit 125. + """ + # Create a directory that looks like a corrupted credential file path + corrupted_dir = tmp_path / "google_token.json" + corrupted_dir.mkdir() + + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + calls = _mock_subprocess_run(monkeypatch) + + # Mock get_credential_file_mounts to return the corrupted entry + fake_mounts = [ + {"host_path": str(corrupted_dir), "container_path": "/root/.hermes/google_token.json"}, + ] + monkeypatch.setattr( + "tools.credential_files.get_credential_file_mounts", + lambda: fake_mounts, + ) + monkeypatch.setattr( + "tools.credential_files.get_skills_directory_mount", + lambda: [], + ) + monkeypatch.setattr( + "tools.credential_files.get_cache_directory_mounts", + lambda: [], + ) + + with caplog.at_level(logging.WARNING): + _make_dummy_env() + + # The corrupted mount should be skipped + run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"] + assert run_calls, "docker run should have been called" + run_args_str = " ".join(run_calls[0][0]) + assert "google_token.json" not in run_args_str + + # Should log a warning about the directory source + assert any( + "source is a directory" in rec.getMessage() + for rec in caplog.records + ) + + +def test_credential_mount_skipped_when_source_missing(monkeypatch, tmp_path, caplog): + """Credential mount should be skipped when source file no longer exists.""" + missing_path = tmp_path / "deleted_token.json" + # Don't create the file — it's "missing" + + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + calls = _mock_subprocess_run(monkeypatch) + + fake_mounts = [ + {"host_path": str(missing_path), "container_path": "/root/.hermes/deleted_token.json"}, + ] + monkeypatch.setattr( + "tools.credential_files.get_credential_file_mounts", + lambda: fake_mounts, + ) + monkeypatch.setattr( + "tools.credential_files.get_skills_directory_mount", + lambda: [], + ) + monkeypatch.setattr( + "tools.credential_files.get_cache_directory_mounts", + lambda: [], + ) + + with caplog.at_level(logging.WARNING): + _make_dummy_env() + + run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"] + assert run_calls, "docker run should have been called" + run_args_str = " ".join(run_calls[0][0]) + assert "deleted_token.json" not in run_args_str + + assert any( + "source not found" in rec.getMessage() + for rec in caplog.records + ) + + +def test_credential_mount_works_when_source_is_valid_file(monkeypatch, tmp_path): + """Credential mount should proceed normally when source is a valid file.""" + valid_file = tmp_path / "token.json" + valid_file.write_text('{"token": "REDACTED"}') + + monkeypatch.setattr(docker_env, "find_docker", lambda: "/usr/bin/docker") + calls = _mock_subprocess_run(monkeypatch) + + fake_mounts = [ + {"host_path": str(valid_file), "container_path": "/root/.hermes/token.json"}, + ] + monkeypatch.setattr( + "tools.credential_files.get_credential_file_mounts", + lambda: fake_mounts, + ) + monkeypatch.setattr( + "tools.credential_files.get_skills_directory_mount", + lambda: [], + ) + monkeypatch.setattr( + "tools.credential_files.get_cache_directory_mounts", + lambda: [], + ) + + _make_dummy_env() + + run_calls = [c for c in calls if isinstance(c[0], list) and len(c[0]) >= 2 and c[0][1] == "run"] + assert run_calls, "docker run should have been called" + run_args_str = " ".join(run_calls[0][0]) + assert "token.json" in run_args_str diff --git a/tools/environments/docker.py b/tools/environments/docker.py index 7a2728ebfe2..8ec098083f1 100644 --- a/tools/environments/docker.py +++ b/tools/environments/docker.py @@ -12,6 +12,7 @@ import shutil import subprocess import sys import uuid +from pathlib import Path from typing import Optional from tools.environments.base import BaseEnvironment, _popen_bash @@ -577,6 +578,22 @@ class DockerEnvironment(BaseEnvironment): ) for mount_entry in get_credential_file_mounts(): + src = Path(mount_entry["host_path"]) + if src.is_dir(): + # Docker-in-Docker: Docker auto-created the source path as + # a directory when it didn't exist on the host. Mounting a + # directory over a file destination causes exit 125. + logger.warning( + "Docker: skipping credential mount — source is a directory " + "(likely Docker-in-Docker auto-creation): %s", + src, + ) + continue + if not src.is_file(): + logger.warning( + "Docker: skipping credential mount — source not found: %s", src, + ) + continue volume_args.extend([ "-v", f"{mount_entry['host_path']}:{mount_entry['container_path']}:ro", @@ -590,6 +607,13 @@ class DockerEnvironment(BaseEnvironment): # Mount skill directories (local + external) so skill # scripts/templates are available inside the container. for skills_mount in get_skills_directory_mount(): + src = Path(skills_mount["host_path"]) + if not src.is_dir(): + logger.warning( + "Docker: skipping skills mount — source is not a directory: %s", + src, + ) + continue volume_args.extend([ "-v", f"{skills_mount['host_path']}:{skills_mount['container_path']}:ro", @@ -605,6 +629,13 @@ class DockerEnvironment(BaseEnvironment): # cached media from inside the container. Read-only — the # container reads these but the host gateway manages writes. for cache_mount in get_cache_directory_mounts(): + src = Path(cache_mount["host_path"]) + if not src.is_dir(): + logger.warning( + "Docker: skipping cache mount — source is not a directory: %s", + src, + ) + continue volume_args.extend([ "-v", f"{cache_mount['host_path']}:{cache_mount['container_path']}:ro",