From d2da57261e2797d568b754a60eac34957d22c754 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 21:50:26 +0800 Subject: [PATCH 1/7] feat: add docker cloud workspace provider Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../src/coding_agent/environment/__init__.py | 2 + .../environment/docker_workspace_provider.py | 298 ++++++++++++++++++ .../test_docker_workspace_provider.py | 229 ++++++++++++++ 3 files changed, 529 insertions(+) create mode 100644 coding-agent/src/coding_agent/environment/docker_workspace_provider.py create mode 100644 coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py diff --git a/coding-agent/src/coding_agent/environment/__init__.py b/coding-agent/src/coding_agent/environment/__init__.py index 5c13e7759..af9cb531a 100644 --- a/coding-agent/src/coding_agent/environment/__init__.py +++ b/coding-agent/src/coding_agent/environment/__init__.py @@ -3,6 +3,7 @@ CloudEnvironment, CloudWorkspaceClient, ) +from .docker_workspace_provider import DockerCloudWorkspaceClient from .local import LocalEnvironment from .workspace_provider import ( CloudWorkspaceClientFactory, @@ -13,6 +14,7 @@ __all__ = [ "CloudCommandResult", + "DockerCloudWorkspaceClient", "CloudEnvironment", "CloudWorkspaceClient", "CloudWorkspaceClientFactory", diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py new file mode 100644 index 000000000..5170056f2 --- /dev/null +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -0,0 +1,298 @@ +from __future__ import annotations + +import json +import posixpath +import re +import subprocess +from collections.abc import Iterable +from dataclasses import dataclass +from pathlib import Path +from typing import TYPE_CHECKING, cast, override + +from .cloud import CloudCommandResult +from .workspace_provider import CloudWorkspaceClientFactory, WorkspaceProvider, register_workspace_provider + +if TYPE_CHECKING: + from .cloud import CloudWorkspaceClient + from ..ui.execution_binding import CloudWorkspaceBinding + + +_WORKSPACE_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.-]*$") +_ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + + +@dataclass(frozen=True) +class _DockerWorkspaceProviderConfig: + workspace_root: Path + container_workspace_root: str + container_name_prefix: str + docker_binary: str + env_allowlist: tuple[str, ...] + exec_user: str | None + + +class DockerCloudWorkspaceClient: + def __init__( + self, + *, + binding: CloudWorkspaceBinding, + config: _DockerWorkspaceProviderConfig, + ) -> None: + _validate_workspace_id(binding.workspace_id) + self.workspace_id: str = binding.workspace_id + self.workspace_url: str = binding.workspace_url + self.default_cwd: str = config.container_workspace_root + self._workspace_root: Path = _workspace_root_for_id( + config.workspace_root, binding.workspace_id + ) + self._docker_binary: str = config.docker_binary + self._container_name: str = ( + f"{config.container_name_prefix}{binding.workspace_id}" + ) + self._env_allowlist: set[str] = set(config.env_allowlist) + self._exec_user: str | None = config.exec_user + + def read_file(self, path: str) -> str: + _, host_path = self._resolve_file_path(path) + return host_path.read_text(encoding="utf-8") + + def write_file(self, path: str, content: str) -> None: + _, host_path = self._resolve_file_path(path) + host_path.parent.mkdir(parents=True, exist_ok=True) + _ = host_path.write_text(content, encoding="utf-8") + + def replace_file(self, path: str, old: str, new: str) -> None: + _, host_path = self._resolve_file_path(path) + content = host_path.read_text(encoding="utf-8") + if old not in content: + raise ValueError(f"'{old}' not found in {path}") + updated = content.replace(old, new, 1) + _ = host_path.write_text(updated, encoding="utf-8") + + def glob_files(self, pattern: str, directory: str) -> list[str]: + _, host_directory = self._resolve_directory_path(directory) + matches = [ + self._remote_path_for_host(path) + for path in sorted(host_directory.glob(pattern)) + ] + return matches + + def grep_search(self, pattern: str, directory: str, include: str) -> list[str]: + _, host_directory = self._resolve_directory_path(directory) + matcher = re.compile(pattern) + include_pattern = include or "*" + matches: list[str] = [] + for path in sorted(host_directory.rglob(include_pattern)): + if not path.is_file(): + continue + for line_number, line in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), + start=1, + ): + if matcher.search(line): + matches.append( + f"{self._remote_path_for_host(path)}:{line_number}:{line}" + ) + return matches + + def apply_patch(self, path: str, patch: str) -> dict[str, object]: + from coding_agent.tools.file_patch_tool import build_file_patch_tool + + _, host_path = self._resolve_file_path(path) + patch_tool = build_file_patch_tool(self._workspace_root) + payload = patch_tool(str(host_path.relative_to(self._workspace_root)), patch) + return _json_object(payload) + + def run_command( + self, + command: str, + *, + cwd: str | None, + env: dict[str, str] | None, + timeout: int, + ) -> CloudCommandResult: + remote_cwd, host_cwd = self._resolve_directory_path(cwd or self.default_cwd) + if not host_cwd.is_dir(): + raise ValueError(f"Working directory does not exist: {remote_cwd}") + + docker_command = [ + self._docker_binary, + "exec", + "--workdir", + remote_cwd, + ] + if self._exec_user is not None: + docker_command.extend(["--user", self._exec_user]) + for key, value in self._filtered_env_items(env): + docker_command.extend(["-e", f"{key}={value}"]) + docker_command.extend([self._container_name, "/bin/sh", "-lc", command]) + + try: + result = subprocess.run( + docker_command, + shell=False, + capture_output=True, + text=True, + timeout=timeout, + env=None, + ) + except subprocess.TimeoutExpired as exc: + raise TimeoutError("docker exec timed out") from exc + + return CloudCommandResult( + stdout=result.stdout, + stderr=result.stderr, + exit_code=result.returncode, + ) + + def _resolve_file_path(self, path: str) -> tuple[str, Path]: + remote_path = _normalize_remote_path(path, self.default_cwd) + return remote_path, _host_path_for_remote( + self._workspace_root, + self.default_cwd, + remote_path, + ) + + def _resolve_directory_path(self, directory: str) -> tuple[str, Path]: + remote_path = _normalize_remote_path(directory, self.default_cwd) + return remote_path, _host_path_for_remote( + self._workspace_root, + self.default_cwd, + remote_path, + ) + + def _filtered_env_items( + self, env: dict[str, str] | None + ) -> list[tuple[str, str]]: + if env is None: + return [] + items: list[tuple[str, str]] = [] + for key, value in env.items(): + if not _ENV_NAME_RE.fullmatch(key): + raise ValueError(f"unsafe environment variable name: {key!r}") + if key not in self._env_allowlist: + raise ValueError( + f"environment variable is not allowed for docker workspace: {key}" + ) + items.append((key, value)) + return items + + def _remote_path_for_host(self, host_path: Path) -> str: + relative = host_path.resolve().relative_to(self._workspace_root) + if not relative.parts: + return self.default_cwd + return posixpath.join(self.default_cwd, *relative.parts) + + +class DockerWorkspaceProvider(WorkspaceProvider): + @override + def build_cloud_client_factory( + self, config: dict[str, object] + ) -> CloudWorkspaceClientFactory: + provider_config = _docker_workspace_provider_config(config) + + def build_client(binding: CloudWorkspaceBinding) -> CloudWorkspaceClient: + return DockerCloudWorkspaceClient(binding=binding, config=provider_config) + + return build_client + + +def _docker_workspace_provider_config( + config: dict[str, object], +) -> _DockerWorkspaceProviderConfig: + workspace_root_raw = config.get("workspace_root") + if not isinstance(workspace_root_raw, str) or not workspace_root_raw.strip(): + raise ValueError("cloud_workspace.workspace_root is required for provider=docker") + + container_workspace_root = _container_workspace_root(config) + container_name_prefix = _optional_string(config.get("container_name_prefix"), "") + docker_binary = _optional_string(config.get("docker_binary"), "docker") + exec_user = _optional_string(config.get("exec_user"), None) + env_allowlist = tuple(_string_list(config.get("env_allowlist"), key="env_allowlist")) + assert container_name_prefix is not None + assert docker_binary is not None + + return _DockerWorkspaceProviderConfig( + workspace_root=Path(workspace_root_raw).expanduser().resolve(), + container_workspace_root=container_workspace_root, + container_name_prefix=container_name_prefix, + docker_binary=docker_binary, + env_allowlist=env_allowlist, + exec_user=exec_user, + ) + + +def _container_workspace_root(config: dict[str, object]) -> str: + root = _optional_string(config.get("container_workspace_root"), "/workspace") + assert root is not None + normalized = posixpath.normpath(root) + if not normalized.startswith("/"): + raise ValueError("cloud_workspace.container_workspace_root must be absolute") + return normalized + + +def _optional_string(value: object, default: str | None) -> str | None: + if value is None: + return default + if not isinstance(value, str): + raise ValueError(f"expected string config value, got {type(value).__name__}") + stripped = value.strip() + if not stripped: + return default + return stripped + + +def _string_list(value: object, *, key: str) -> Iterable[str]: + if value is None: + return () + if not isinstance(value, list): + raise ValueError(f"cloud_workspace.{key} must be a list of strings") + items: list[str] = [] + for item in cast(list[object], value): + if not isinstance(item, str) or not item.strip(): + raise ValueError(f"cloud_workspace.{key} must be a list of strings") + items.append(item.strip()) + return items + + +def _json_object(payload: str) -> dict[str, object]: + decoded = cast(object, json.loads(payload)) + if not isinstance(decoded, dict): + raise ValueError("docker workspace patch payload must decode to an object") + decoded_dict = cast(dict[object, object], decoded) + return {str(key): value for key, value in decoded_dict.items()} + + +def _validate_workspace_id(workspace_id: str) -> None: + if not _WORKSPACE_ID_RE.fullmatch(workspace_id): + raise ValueError(f"unsupported workspace id for docker provider: {workspace_id}") + + +def _workspace_root_for_id(workspace_root: Path, workspace_id: str) -> Path: + candidate = (workspace_root / workspace_id).resolve() + try: + _ = candidate.relative_to(workspace_root) + except ValueError as exc: + raise ValueError(f"workspace id escapes configured workspace root: {workspace_id}") from exc + return candidate + + +def _normalize_remote_path(path: str, workspace_root: str) -> str: + resolved = path if path.startswith("/") else posixpath.join(workspace_root, path) + normalized = posixpath.normpath(resolved) + workspace_prefix = workspace_root.rstrip("/") + "/" + if normalized != workspace_root and not normalized.startswith(workspace_prefix): + raise ValueError(f"Path is outside docker workspace: {normalized}") + return normalized + + +def _host_path_for_remote( + workspace_root: Path, + container_workspace_root: str, + remote_path: str, +) -> Path: + relative = remote_path.removeprefix(container_workspace_root).lstrip("/") + return workspace_root if not relative else workspace_root / Path(relative) + + +register_workspace_provider("docker", DockerWorkspaceProvider()) diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py new file mode 100644 index 000000000..e54a1de25 --- /dev/null +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -0,0 +1,229 @@ +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from coding_agent.environment import DockerCloudWorkspaceClient, cloud_client_factory_from_config +from coding_agent.ui.execution_binding import CloudWorkspaceBinding + + +def test_docker_workspace_provider_builds_client_from_config(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + workspace_root.mkdir() + (workspace_root / "ws-123").mkdir() + + factory = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + "container_name_prefix": "agent-", + "docker_binary": "/usr/bin/docker", + "env_allowlist": ["SAFE_VAR"], + } + ) + + client = factory( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + assert isinstance(client, DockerCloudWorkspaceClient) + assert client.workspace_id == "ws-123" + assert client.workspace_url == "https://workspace.example.com" + assert client.default_cwd == "/workspace" + + +def test_docker_cloud_client_maps_file_tools_to_remote_workspace(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + nested = target_root / "pkg" + nested.mkdir(parents=True) + note = nested / "note.txt" + _ = note.write_text("hello docker\n", encoding="utf-8") + other = target_root / "other.txt" + _ = other.write_text("docker hello\n", encoding="utf-8") + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + assert client.read_file("pkg/note.txt") == "hello docker\n" + _ = client.write_file("pkg/new.txt", "new file") + assert (nested / "new.txt").read_text(encoding="utf-8") == "new file" + _ = client.replace_file("pkg/new.txt", "new", "updated") + assert (nested / "new.txt").read_text(encoding="utf-8") == "updated file" + assert client.glob_files("*.txt", "/workspace/pkg") == [ + "/workspace/pkg/new.txt", + "/workspace/pkg/note.txt", + ] + assert client.grep_search("hello", "/workspace", "*.txt") == [ + "/workspace/other.txt:1:docker hello", + "/workspace/pkg/note.txt:1:hello docker", + ] + + +def test_docker_cloud_client_applies_patch_with_workspace_root(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + target = target_root / "hello.py" + _ = target.write_text("def greet():\n return 'hello'\n", encoding="utf-8") + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + payload = client.apply_patch( + "hello.py", + "@@ -1,2 +1,2 @@\n def greet():\n- return 'hello'\n+ return 'hello docker'\n", + ) + + assert payload["success"] is True + assert payload["changed"] is True + assert "hello docker" in target.read_text(encoding="utf-8") + + +def test_docker_cloud_client_runs_shell_in_workspace_cwd( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + (target_root / "pkg").mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + "container_name_prefix": "agent-", + "docker_binary": "/usr/bin/docker", + "env_allowlist": ["SAFE_VAR"], + "exec_user": "1000:1000", + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + captured: dict[str, object] = {} + + def fake_run( + command: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + captured["command"] = command + captured["kwargs"] = kwargs + return subprocess.CompletedProcess(command, 0, stdout="ok", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + + result = client.run_command( + "python -V", + cwd="/workspace/pkg", + env={"SAFE_VAR": "1"}, + timeout=9, + ) + + assert result.stdout == "ok" + assert result.stderr == "" + assert result.exit_code == 0 + assert captured["command"] == [ + "/usr/bin/docker", + "exec", + "--workdir", + "/workspace/pkg", + "--user", + "1000:1000", + "-e", + "SAFE_VAR=1", + "agent-ws-123", + "/bin/sh", + "-lc", + "python -V", + ] + assert captured["kwargs"] == { + "shell": False, + "capture_output": True, + "text": True, + "timeout": 9, + "env": None, + } + + +def test_docker_cloud_client_rejects_disallowed_env_names(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + "env_allowlist": ["SAFE_VAR"], + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + with pytest.raises( + ValueError, + match="environment variable is not allowed for docker workspace: OTHER_VAR", + ): + _ = client.run_command( + "python -V", + cwd="/workspace", + env={"OTHER_VAR": "1"}, + timeout=5, + ) + + +def test_docker_cloud_client_rejects_workspace_escape(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + with pytest.raises(ValueError, match="Path is outside docker workspace: /etc"): + _ = client.read_file("/etc/passwd") + + +def test_docker_workspace_provider_requires_workspace_root() -> None: + with pytest.raises( + ValueError, + match="cloud_workspace.workspace_root is required for provider=docker", + ): + _ = cloud_client_factory_from_config({"provider": "docker"}) From 235b666f5d6b5438965bfaf4154c4bcbe28428db Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 23:03:49 +0800 Subject: [PATCH 2/7] fix: harden docker workspace path resolution Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../environment/docker_workspace_provider.py | 49 ++++++++----- .../test_docker_workspace_provider.py | 71 ++++++++++++++++++- 2 files changed, 101 insertions(+), 19 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index 5170056f2..71c904882 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -71,10 +71,11 @@ def replace_file(self, path: str, old: str, new: str) -> None: def glob_files(self, pattern: str, directory: str) -> list[str]: _, host_directory = self._resolve_directory_path(directory) - matches = [ - self._remote_path_for_host(path) - for path in sorted(host_directory.glob(pattern)) - ] + matches: list[str] = [] + for path in sorted(host_directory.glob(pattern)): + remote_path = self._workspace_entry_remote_path(path) + _ = self._validated_workspace_path(path, remote_path) + matches.append(remote_path) return matches def grep_search(self, pattern: str, directory: str, include: str) -> list[str]: @@ -83,16 +84,16 @@ def grep_search(self, pattern: str, directory: str, include: str) -> list[str]: include_pattern = include or "*" matches: list[str] = [] for path in sorted(host_directory.rglob(include_pattern)): - if not path.is_file(): + remote_path = self._workspace_entry_remote_path(path) + resolved_path = self._validated_workspace_path(path, remote_path) + if not resolved_path.is_file(): continue for line_number, line in enumerate( - path.read_text(encoding="utf-8", errors="replace").splitlines(), + resolved_path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1, ): if matcher.search(line): - matches.append( - f"{self._remote_path_for_host(path)}:{line_number}:{line}" - ) + matches.append(f"{remote_path}:{line_number}:{line}") return matches def apply_patch(self, path: str, patch: str) -> dict[str, object]: @@ -125,7 +126,7 @@ def run_command( docker_command.extend(["--user", self._exec_user]) for key, value in self._filtered_env_items(env): docker_command.extend(["-e", f"{key}={value}"]) - docker_command.extend([self._container_name, "/bin/sh", "-lc", command]) + docker_command.extend([self._container_name, "/bin/sh", "-c", command]) try: result = subprocess.run( @@ -147,19 +148,17 @@ def run_command( def _resolve_file_path(self, path: str) -> tuple[str, Path]: remote_path = _normalize_remote_path(path, self.default_cwd) - return remote_path, _host_path_for_remote( - self._workspace_root, - self.default_cwd, - remote_path, + host_path = _host_path_for_remote( + self._workspace_root, self.default_cwd, remote_path ) + return remote_path, self._validated_workspace_path(host_path, remote_path) def _resolve_directory_path(self, directory: str) -> tuple[str, Path]: remote_path = _normalize_remote_path(directory, self.default_cwd) - return remote_path, _host_path_for_remote( - self._workspace_root, - self.default_cwd, - remote_path, + host_path = _host_path_for_remote( + self._workspace_root, self.default_cwd, remote_path ) + return remote_path, self._validated_workspace_path(host_path, remote_path) def _filtered_env_items( self, env: dict[str, str] | None @@ -183,6 +182,20 @@ def _remote_path_for_host(self, host_path: Path) -> str: return self.default_cwd return posixpath.join(self.default_cwd, *relative.parts) + def _validated_workspace_path(self, host_path: Path, remote_path: str) -> Path: + resolved_path = host_path.resolve(strict=False) + try: + _ = resolved_path.relative_to(self._workspace_root) + except ValueError as exc: + raise ValueError(f"Path is outside docker workspace: {remote_path}") from exc + return resolved_path + + def _workspace_entry_remote_path(self, host_path: Path) -> str: + relative = host_path.relative_to(self._workspace_root) + if not relative.parts: + return self.default_cwd + return posixpath.join(self.default_cwd, *relative.parts) + class DockerWorkspaceProvider(WorkspaceProvider): @override diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index e54a1de25..42341ec1a 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -158,7 +158,7 @@ def fake_run( "SAFE_VAR=1", "agent-ws-123", "/bin/sh", - "-lc", + "-c", "python -V", ] assert captured["kwargs"] == { @@ -221,6 +221,75 @@ def test_docker_cloud_client_rejects_workspace_escape(tmp_path: Path) -> None: _ = client.read_file("/etc/passwd") +def test_docker_cloud_client_rejects_symlink_escape_paths(tmp_path: Path) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + outside_root = tmp_path / "outside" + outside_root.mkdir() + secret = outside_root / "secret.txt" + _ = secret.write_text("top secret\n", encoding="utf-8") + escape_dir = target_root / "escape" + escape_dir.symlink_to(outside_root, target_is_directory=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape/secret\.txt", + ): + _ = client.read_file("escape/secret.txt") + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape/new\.txt", + ): + _ = client.write_file("escape/new.txt", "blocked") + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape/secret\.txt", + ): + _ = client.replace_file("escape/secret.txt", "top", "public") + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape/secret\.txt", + ): + _ = client.apply_patch( + "escape/secret.txt", + "@@ -1 +1 @@\n-top secret\n+public\n", + ) + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape", + ): + _ = client.glob_files("*.txt", "/workspace/escape") + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape", + ): + _ = client.grep_search("secret", "/workspace/escape", "*.txt") + + with pytest.raises( + ValueError, + match=r"Path is outside docker workspace: /workspace/escape", + ): + _ = client.run_command("python -V", cwd="/workspace/escape", env=None, timeout=5) + + def test_docker_workspace_provider_requires_workspace_root() -> None: with pytest.raises( ValueError, From 666e5820c70e7928e3e67ef10689dcd62c1b6eb3 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Tue, 5 May 2026 23:40:15 +0800 Subject: [PATCH 3/7] fix: enforce docker workspace timeout root Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../environment/docker_workspace_provider.py | 40 +++++++- .../test_docker_workspace_provider.py | 98 ++++++++++++++++++- 2 files changed, 132 insertions(+), 6 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index 71c904882..896460f2d 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -19,6 +19,8 @@ _WORKSPACE_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.-]*$") _ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +_DOCKER_TIMEOUT_KILL_AFTER_SECONDS = 2 +_DOCKER_TIMEOUT_MARGIN_SECONDS = 1 @dataclass(frozen=True) @@ -112,6 +114,8 @@ def run_command( env: dict[str, str] | None, timeout: int, ) -> CloudCommandResult: + if timeout <= 0: + raise ValueError("timeout must be positive") remote_cwd, host_cwd = self._resolve_directory_path(cwd or self.default_cwd) if not host_cwd.is_dir(): raise ValueError(f"Working directory does not exist: {remote_cwd}") @@ -126,7 +130,25 @@ def run_command( docker_command.extend(["--user", self._exec_user]) for key, value in self._filtered_env_items(env): docker_command.extend(["-e", f"{key}={value}"]) - docker_command.extend([self._container_name, "/bin/sh", "-c", command]) + docker_command.extend( + [ + self._container_name, + "timeout", + "-s", + "TERM", + "-k", + f"{_DOCKER_TIMEOUT_KILL_AFTER_SECONDS}s", + f"{timeout}s", + "/bin/sh", + "-c", + command, + ] + ) + local_timeout = ( + timeout + + _DOCKER_TIMEOUT_KILL_AFTER_SECONDS + + _DOCKER_TIMEOUT_MARGIN_SECONDS + ) try: result = subprocess.run( @@ -134,11 +156,14 @@ def run_command( shell=False, capture_output=True, text=True, - timeout=timeout, + timeout=local_timeout, env=None, ) except subprocess.TimeoutExpired as exc: - raise TimeoutError("docker exec timed out") from exc + raise TimeoutError(f"docker exec command timed out after {timeout}s") from exc + + if result.returncode in {124, 137}: + raise TimeoutError(f"docker exec command timed out after {timeout}s") return CloudCommandResult( stdout=result.stdout, @@ -241,6 +266,8 @@ def _container_workspace_root(config: dict[str, object]) -> str: normalized = posixpath.normpath(root) if not normalized.startswith("/"): raise ValueError("cloud_workspace.container_workspace_root must be absolute") + if not normalized.lstrip("/"): + raise ValueError("cloud_workspace.container_workspace_root must not resolve to /") return normalized @@ -291,10 +318,13 @@ def _workspace_root_for_id(workspace_root: Path, workspace_id: str) -> Path: def _normalize_remote_path(path: str, workspace_root: str) -> str: + normalized_root = posixpath.normpath(workspace_root) + if not normalized_root.lstrip("/"): + raise ValueError("cloud_workspace.container_workspace_root must not resolve to /") resolved = path if path.startswith("/") else posixpath.join(workspace_root, path) normalized = posixpath.normpath(resolved) - workspace_prefix = workspace_root.rstrip("/") + "/" - if normalized != workspace_root and not normalized.startswith(workspace_prefix): + workspace_prefix = normalized_root.rstrip("/") + "/" + if normalized != normalized_root and not normalized.startswith(workspace_prefix): raise ValueError(f"Path is outside docker workspace: {normalized}") return normalized diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index 42341ec1a..3b3caca0c 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -1,6 +1,8 @@ from __future__ import annotations import subprocess +import threading +import time from pathlib import Path import pytest @@ -157,6 +159,12 @@ def fake_run( "-e", "SAFE_VAR=1", "agent-ws-123", + "timeout", + "-s", + "TERM", + "-k", + "2s", + "9s", "/bin/sh", "-c", "python -V", @@ -165,11 +173,84 @@ def fake_run( "shell": False, "capture_output": True, "text": True, - "timeout": 9, + "timeout": 12, "env": None, } +def test_docker_cloud_client_raises_timeout_when_container_command_times_out( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + def fake_run( + command: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + _ = kwargs + return subprocess.CompletedProcess(command, 124, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + + with pytest.raises(TimeoutError, match="docker exec command timed out after 1s"): + _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) + + +def test_docker_cloud_client_timeout_contract_blocks_post_timeout_mutation( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + marker = target_root / "late-write.txt" + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + def fake_run( + command: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + _ = kwargs + if "timeout" not in command: + timer = threading.Timer( + 0.05, + lambda: marker.write_text("late write\n", encoding="utf-8"), + ) + timer.start() + raise subprocess.TimeoutExpired(command, timeout=1) + return subprocess.CompletedProcess(command, 124, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + + with pytest.raises(TimeoutError, match="docker exec command timed out after 1s"): + _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) + + time.sleep(0.1) + assert not marker.exists() + + def test_docker_cloud_client_rejects_disallowed_env_names(tmp_path: Path) -> None: workspace_root = tmp_path / "workspaces" target_root = workspace_root / "ws-123" @@ -296,3 +377,18 @@ def test_docker_workspace_provider_requires_workspace_root() -> None: match="cloud_workspace.workspace_root is required for provider=docker", ): _ = cloud_client_factory_from_config({"provider": "docker"}) + + +@pytest.mark.parametrize("root", ["/", "//", "/./", "/workspace/.."]) +def test_docker_workspace_provider_rejects_root_container_workspace_root(root: str) -> None: + with pytest.raises( + ValueError, + match=r"cloud_workspace\.container_workspace_root must not resolve to /", + ): + _ = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": "/srv/workspaces", + "container_workspace_root": root, + } + ) From 4ff979caa0fcb26c81fc8a7eb68b2569433186d8 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Wed, 6 May 2026 00:06:48 +0800 Subject: [PATCH 4/7] fix: preserve docker timeout exit status Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../environment/docker_workspace_provider.py | 23 +++++- .../test_docker_workspace_provider.py | 70 ++++++++++++++++--- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index 896460f2d..8abe77b8f 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -21,6 +21,7 @@ _ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") _DOCKER_TIMEOUT_KILL_AFTER_SECONDS = 2 _DOCKER_TIMEOUT_MARGIN_SECONDS = 1 +_DOCKER_TIMEOUT_SENTINEL = "__CODING_AGENT_DOCKER_TIMEOUT__" @dataclass(frozen=True) @@ -130,6 +131,24 @@ def run_command( docker_command.extend(["--user", self._exec_user]) for key, value in self._filtered_env_items(env): docker_command.extend(["-e", f"{key}={value}"]) + timeout_wrapper = "\n".join( + [ + "child=''", + "_coding_agent_timeout() {", + ' if [ -n "$child" ]; then', + ' kill -TERM "$child" 2>/dev/null || true', + " fi", + f" printf '%s\\n' '{_DOCKER_TIMEOUT_SENTINEL}' >&2", + ' wait "$child" 2>/dev/null || true', + " exit 124", + "}", + "trap _coding_agent_timeout TERM", + '/bin/sh -c "$1" &', + "child=$!", + 'wait "$child"', + "exit $?", + ] + ) docker_command.extend( [ self._container_name, @@ -141,6 +160,8 @@ def run_command( f"{timeout}s", "/bin/sh", "-c", + timeout_wrapper, + "sh", command, ] ) @@ -162,7 +183,7 @@ def run_command( except subprocess.TimeoutExpired as exc: raise TimeoutError(f"docker exec command timed out after {timeout}s") from exc - if result.returncode in {124, 137}: + if _DOCKER_TIMEOUT_SENTINEL in result.stderr: raise TimeoutError(f"docker exec command timed out after {timeout}s") return CloudCommandResult( diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index 3b3caca0c..1aaf06c9d 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -11,6 +11,9 @@ from coding_agent.ui.execution_binding import CloudWorkspaceBinding +TIMEOUT_SENTINEL = "__CODING_AGENT_DOCKER_TIMEOUT__" + + def test_docker_workspace_provider_builds_client_from_config(tmp_path: Path) -> None: workspace_root = tmp_path / "workspaces" workspace_root.mkdir() @@ -128,13 +131,15 @@ def test_docker_cloud_client_runs_shell_in_workspace_cwd( ) ) - captured: dict[str, object] = {} + captured_command: list[str] = [] + captured_kwargs: dict[str, object] = {} def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: - captured["command"] = command - captured["kwargs"] = kwargs + captured_command[:] = command + captured_kwargs.clear() + captured_kwargs.update(kwargs) return subprocess.CompletedProcess(command, 0, stdout="ok", stderr="") monkeypatch.setattr(subprocess, "run", fake_run) @@ -149,7 +154,7 @@ def fake_run( assert result.stdout == "ok" assert result.stderr == "" assert result.exit_code == 0 - assert captured["command"] == [ + assert captured_command[:17] == [ "/usr/bin/docker", "exec", "--workdir", @@ -167,9 +172,13 @@ def fake_run( "9s", "/bin/sh", "-c", - "python -V", ] - assert captured["kwargs"] == { + timeout_wrapper = captured_command[17] + assert TIMEOUT_SENTINEL in timeout_wrapper + assert 'trap _coding_agent_timeout TERM' in timeout_wrapper + assert '/bin/sh -c "$1" &' in timeout_wrapper + assert captured_command[18:] == ["sh", "python -V"] + assert captured_kwargs == { "shell": False, "capture_output": True, "text": True, @@ -201,7 +210,12 @@ def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: _ = kwargs - return subprocess.CompletedProcess(command, 124, stdout="", stderr="") + return subprocess.CompletedProcess( + command, + 124, + stdout="", + stderr=f"{TIMEOUT_SENTINEL}\n", + ) monkeypatch.setattr(subprocess, "run", fake_run) @@ -209,6 +223,39 @@ def fake_run( _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) +@pytest.mark.parametrize("exit_code", [124, 137]) +def test_docker_cloud_client_preserves_legitimate_exit_codes( + exit_code: int, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + def fake_run( + command: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + _ = kwargs + return subprocess.CompletedProcess(command, exit_code, stdout="", stderr="") + + monkeypatch.setattr(subprocess, "run", fake_run) + + result = client.run_command("exit 0", cwd="/workspace", env=None, timeout=1) + + assert result.exit_code == exit_code + + def test_docker_cloud_client_timeout_contract_blocks_post_timeout_mutation( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: @@ -233,14 +280,19 @@ def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: _ = kwargs - if "timeout" not in command: + if not any(TIMEOUT_SENTINEL in arg for arg in command): timer = threading.Timer( 0.05, lambda: marker.write_text("late write\n", encoding="utf-8"), ) timer.start() raise subprocess.TimeoutExpired(command, timeout=1) - return subprocess.CompletedProcess(command, 124, stdout="", stderr="") + return subprocess.CompletedProcess( + command, + 124, + stdout="", + stderr=f"{TIMEOUT_SENTINEL}\n", + ) monkeypatch.setattr(subprocess, "run", fake_run) From 4d5701943e420578e6ca6bc800887d3e5af2b917 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Wed, 6 May 2026 13:07:54 +0800 Subject: [PATCH 5/7] fix: scope docker timeout sentinel Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../environment/docker_workspace_provider.py | 16 ++++- .../test_docker_workspace_provider.py | 60 +++++++++++++++++-- 2 files changed, 67 insertions(+), 9 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index 8abe77b8f..351da1229 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -3,6 +3,7 @@ import json import posixpath import re +import secrets import subprocess from collections.abc import Iterable from dataclasses import dataclass @@ -21,7 +22,7 @@ _ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") _DOCKER_TIMEOUT_KILL_AFTER_SECONDS = 2 _DOCKER_TIMEOUT_MARGIN_SECONDS = 1 -_DOCKER_TIMEOUT_SENTINEL = "__CODING_AGENT_DOCKER_TIMEOUT__" +_DOCKER_TIMEOUT_SENTINEL_PREFIX = "__CODING_AGENT_DOCKER_TIMEOUT__:" @dataclass(frozen=True) @@ -131,6 +132,7 @@ def run_command( docker_command.extend(["--user", self._exec_user]) for key, value in self._filtered_env_items(env): docker_command.extend(["-e", f"{key}={value}"]) + timeout_sentinel = _docker_timeout_sentinel() timeout_wrapper = "\n".join( [ "child=''", @@ -138,7 +140,7 @@ def run_command( ' if [ -n "$child" ]; then', ' kill -TERM "$child" 2>/dev/null || true', " fi", - f" printf '%s\\n' '{_DOCKER_TIMEOUT_SENTINEL}' >&2", + f" printf '%s\\n' '{timeout_sentinel}' >&2", ' wait "$child" 2>/dev/null || true', " exit 124", "}", @@ -183,7 +185,7 @@ def run_command( except subprocess.TimeoutExpired as exc: raise TimeoutError(f"docker exec command timed out after {timeout}s") from exc - if _DOCKER_TIMEOUT_SENTINEL in result.stderr: + if _contains_timeout_sentinel(result.stderr, timeout_sentinel): raise TimeoutError(f"docker exec command timed out after {timeout}s") return CloudCommandResult( @@ -324,6 +326,14 @@ def _json_object(payload: str) -> dict[str, object]: return {str(key): value for key, value in decoded_dict.items()} +def _docker_timeout_sentinel() -> str: + return f"{_DOCKER_TIMEOUT_SENTINEL_PREFIX}{secrets.token_hex(16)}" + + +def _contains_timeout_sentinel(stderr: str, sentinel: str) -> bool: + return any(line == sentinel for line in stderr.splitlines()) + + def _validate_workspace_id(workspace_id: str) -> None: if not _WORKSPACE_ID_RE.fullmatch(workspace_id): raise ValueError(f"unsupported workspace id for docker provider: {workspace_id}") diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index 1aaf06c9d..d00c2fbb2 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -1,5 +1,6 @@ from __future__ import annotations +import re import subprocess import threading import time @@ -11,7 +12,15 @@ from coding_agent.ui.execution_binding import CloudWorkspaceBinding -TIMEOUT_SENTINEL = "__CODING_AGENT_DOCKER_TIMEOUT__" +TIMEOUT_SENTINEL_PREFIX = "__CODING_AGENT_DOCKER_TIMEOUT__:" + + +def _extract_timeout_sentinel(command: list[str]) -> str: + for arg in command: + match = re.search(r"__CODING_AGENT_DOCKER_TIMEOUT__:[0-9a-f]+", arg) + if match is not None: + return match.group(0) + raise AssertionError(f"timeout sentinel not found in command: {command}") def test_docker_workspace_provider_builds_client_from_config(tmp_path: Path) -> None: @@ -174,7 +183,7 @@ def fake_run( "-c", ] timeout_wrapper = captured_command[17] - assert TIMEOUT_SENTINEL in timeout_wrapper + assert TIMEOUT_SENTINEL_PREFIX in timeout_wrapper assert 'trap _coding_agent_timeout TERM' in timeout_wrapper assert '/bin/sh -c "$1" &' in timeout_wrapper assert captured_command[18:] == ["sh", "python -V"] @@ -210,11 +219,12 @@ def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: _ = kwargs + timeout_sentinel = _extract_timeout_sentinel(command) return subprocess.CompletedProcess( command, 124, stdout="", - stderr=f"{TIMEOUT_SENTINEL}\n", + stderr=f"{timeout_sentinel}\n", ) monkeypatch.setattr(subprocess, "run", fake_run) @@ -223,6 +233,42 @@ def fake_run( _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) +def test_docker_cloud_client_preserves_stderr_with_nonmatching_timeout_prefix( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + workspace_root = tmp_path / "workspaces" + target_root = workspace_root / "ws-123" + target_root.mkdir(parents=True) + + client = cloud_client_factory_from_config( + { + "provider": "docker", + "workspace_root": str(workspace_root), + } + )( + CloudWorkspaceBinding( + workspace_url="https://workspace.example.com", + workspace_id="ws-123", + ) + ) + + stderr = f"{TIMEOUT_SENTINEL_PREFIX}not-current-run\n" + + def fake_run( + command: list[str], **kwargs: object + ) -> subprocess.CompletedProcess[str]: + _ = command + _ = kwargs + return subprocess.CompletedProcess([], 1, stdout="", stderr=stderr) + + monkeypatch.setattr(subprocess, "run", fake_run) + + result = client.run_command("exit 1", cwd="/workspace", env=None, timeout=1) + + assert result.exit_code == 1 + assert result.stderr == stderr + + @pytest.mark.parametrize("exit_code", [124, 137]) def test_docker_cloud_client_preserves_legitimate_exit_codes( exit_code: int, monkeypatch: pytest.MonkeyPatch, tmp_path: Path @@ -279,19 +325,21 @@ def test_docker_cloud_client_timeout_contract_blocks_post_timeout_mutation( def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: - _ = kwargs - if not any(TIMEOUT_SENTINEL in arg for arg in command): + local_timeout = kwargs.get("timeout") + if local_timeout == 1: timer = threading.Timer( 0.05, lambda: marker.write_text("late write\n", encoding="utf-8"), ) timer.start() raise subprocess.TimeoutExpired(command, timeout=1) + assert local_timeout == 4 + timeout_sentinel = _extract_timeout_sentinel(command) return subprocess.CompletedProcess( command, 124, stdout="", - stderr=f"{TIMEOUT_SENTINEL}\n", + stderr=f"{timeout_sentinel}\n", ) monkeypatch.setattr(subprocess, "run", fake_run) From 7419408067386baf3ab2166ab43bdf56ea4e2b03 Mon Sep 17 00:00:00 2001 From: kina0630 Date: Wed, 6 May 2026 14:32:49 +0800 Subject: [PATCH 6/7] fix: clean up docker exec timeout fallback --- .../environment/docker_workspace_provider.py | 55 ++++++++++++++++++- .../test_docker_workspace_provider.py | 27 +++++---- 2 files changed, 69 insertions(+), 13 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index 351da1229..d9a5fc908 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -22,6 +22,7 @@ _ENV_NAME_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") _DOCKER_TIMEOUT_KILL_AFTER_SECONDS = 2 _DOCKER_TIMEOUT_MARGIN_SECONDS = 1 +_DOCKER_TIMEOUT_CLEANUP_SECONDS = 3 _DOCKER_TIMEOUT_SENTINEL_PREFIX = "__CODING_AGENT_DOCKER_TIMEOUT__:" @@ -133,20 +134,31 @@ def run_command( for key, value in self._filtered_env_items(env): docker_command.extend(["-e", f"{key}={value}"]) timeout_sentinel = _docker_timeout_sentinel() + timeout_pidfile = f"/tmp/coding-agent-docker-exec-{secrets.token_hex(16)}.pid" timeout_wrapper = "\n".join( [ + f"pidfile='{timeout_pidfile}'", "child=''", + "_coding_agent_cleanup() {", + ' rm -f "$pidfile"', + "}", "_coding_agent_timeout() {", ' if [ -n "$child" ]; then', - ' kill -TERM "$child" 2>/dev/null || true', + ' kill -TERM -- "-$child" 2>/dev/null || kill -TERM "$child" 2>/dev/null || true', " fi", f" printf '%s\\n' '{timeout_sentinel}' >&2", ' wait "$child" 2>/dev/null || true', " exit 124", "}", + "trap _coding_agent_cleanup EXIT", "trap _coding_agent_timeout TERM", - '/bin/sh -c "$1" &', + 'if command -v setsid >/dev/null 2>&1; then', + ' setsid /bin/sh -c "$1" &', + "else", + ' /bin/sh -c "$1" &', + "fi", "child=$!", + 'printf "%s\\n" "$child" > "$pidfile"', 'wait "$child"', "exit $?", ] @@ -183,6 +195,7 @@ def run_command( env=None, ) except subprocess.TimeoutExpired as exc: + self._cleanup_timed_out_command(timeout_pidfile) raise TimeoutError(f"docker exec command timed out after {timeout}s") from exc if _contains_timeout_sentinel(result.stderr, timeout_sentinel): @@ -230,6 +243,44 @@ def _remote_path_for_host(self, host_path: Path) -> str: return self.default_cwd return posixpath.join(self.default_cwd, *relative.parts) + def _cleanup_timed_out_command(self, timeout_pidfile: str) -> None: + cleanup_command = [self._docker_binary, "exec"] + if self._exec_user is not None: + cleanup_command.extend(["--user", self._exec_user]) + cleanup_script = "\n".join( + [ + 'pid=$(cat "$1" 2>/dev/null || true)', + 'if [ -z "$pid" ]; then', + " exit 0", + "fi", + 'kill -TERM -- "-$pid" 2>/dev/null || kill -TERM "$pid" 2>/dev/null || true', + f"sleep {_DOCKER_TIMEOUT_KILL_AFTER_SECONDS}", + 'kill -KILL -- "-$pid" 2>/dev/null || kill -KILL "$pid" 2>/dev/null || true', + 'rm -f "$1"', + ] + ) + cleanup_command.extend( + [ + self._container_name, + "/bin/sh", + "-c", + cleanup_script, + "sh", + timeout_pidfile, + ] + ) + try: + _ = subprocess.run( + cleanup_command, + shell=False, + capture_output=True, + text=True, + timeout=_DOCKER_TIMEOUT_CLEANUP_SECONDS, + env=None, + ) + except (OSError, subprocess.SubprocessError): + return + def _validated_workspace_path(self, host_path: Path, remote_path: str) -> Path: resolved_path = host_path.resolve(strict=False) try: diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index d00c2fbb2..cc4c6d92a 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -322,25 +322,28 @@ def test_docker_cloud_client_timeout_contract_blocks_post_timeout_mutation( ) ) + cleanup_called = threading.Event() + observed_timeouts: list[object] = [] + timers: list[threading.Timer] = [] + def fake_run( command: list[str], **kwargs: object ) -> subprocess.CompletedProcess[str]: local_timeout = kwargs.get("timeout") - if local_timeout == 1: + observed_timeouts.append(local_timeout) + if local_timeout == 4: timer = threading.Timer( 0.05, - lambda: marker.write_text("late write\n", encoding="utf-8"), + lambda: None + if cleanup_called.is_set() + else marker.write_text("late write\n", encoding="utf-8"), ) + timers.append(timer) timer.start() - raise subprocess.TimeoutExpired(command, timeout=1) - assert local_timeout == 4 - timeout_sentinel = _extract_timeout_sentinel(command) - return subprocess.CompletedProcess( - command, - 124, - stdout="", - stderr=f"{timeout_sentinel}\n", - ) + raise subprocess.TimeoutExpired(command, timeout=4) + assert local_timeout == 3 + cleanup_called.set() + return subprocess.CompletedProcess(command, 0, stdout="", stderr="") monkeypatch.setattr(subprocess, "run", fake_run) @@ -348,6 +351,8 @@ def fake_run( _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) time.sleep(0.1) + assert cleanup_called.is_set() + assert observed_timeouts == [4, 3] assert not marker.exists() From 4c7d8307b560dd98486a8ba1d431816e0bcd9bfa Mon Sep 17 00:00:00 2001 From: kina0630 Date: Wed, 6 May 2026 15:02:53 +0800 Subject: [PATCH 7/7] fix: publish docker pidfile before exec --- .../environment/docker_workspace_provider.py | 19 ++++++------ .../test_docker_workspace_provider.py | 29 ++++++++++--------- 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py index d9a5fc908..3bc42b023 100644 --- a/coding-agent/src/coding_agent/environment/docker_workspace_provider.py +++ b/coding-agent/src/coding_agent/environment/docker_workspace_provider.py @@ -135,6 +135,12 @@ def run_command( docker_command.extend(["-e", f"{key}={value}"]) timeout_sentinel = _docker_timeout_sentinel() timeout_pidfile = f"/tmp/coding-agent-docker-exec-{secrets.token_hex(16)}.pid" + child_wrapper = "\n".join( + [ + 'printf "%s\\n" "$$" > "$1" || exit 125', + 'exec /bin/sh -c "$2"', + ] + ) timeout_wrapper = "\n".join( [ f"pidfile='{timeout_pidfile}'", @@ -153,12 +159,11 @@ def run_command( "trap _coding_agent_cleanup EXIT", "trap _coding_agent_timeout TERM", 'if command -v setsid >/dev/null 2>&1; then', - ' setsid /bin/sh -c "$1" &', + ' setsid /bin/sh -c "$1" sh "$pidfile" "$2" &', "else", - ' /bin/sh -c "$1" &', + ' /bin/sh -c "$1" sh "$pidfile" "$2" &', "fi", "child=$!", - 'printf "%s\\n" "$child" > "$pidfile"', 'wait "$child"', "exit $?", ] @@ -176,6 +181,7 @@ def run_command( "-c", timeout_wrapper, "sh", + child_wrapper, command, ] ) @@ -237,12 +243,6 @@ def _filtered_env_items( items.append((key, value)) return items - def _remote_path_for_host(self, host_path: Path) -> str: - relative = host_path.resolve().relative_to(self._workspace_root) - if not relative.parts: - return self.default_cwd - return posixpath.join(self.default_cwd, *relative.parts) - def _cleanup_timed_out_command(self, timeout_pidfile: str) -> None: cleanup_command = [self._docker_binary, "exec"] if self._exec_user is not None: @@ -251,6 +251,7 @@ def _cleanup_timed_out_command(self, timeout_pidfile: str) -> None: [ 'pid=$(cat "$1" 2>/dev/null || true)', 'if [ -z "$pid" ]; then', + ' rm -f "$1"', " exit 0", "fi", 'kill -TERM -- "-$pid" 2>/dev/null || kill -TERM "$pid" 2>/dev/null || true', diff --git a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py index cc4c6d92a..2283aca4b 100644 --- a/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py +++ b/coding-agent/tests/coding_agent/environment/test_docker_workspace_provider.py @@ -3,7 +3,6 @@ import re import subprocess import threading -import time from pathlib import Path import pytest @@ -183,10 +182,13 @@ def fake_run( "-c", ] timeout_wrapper = captured_command[17] + child_wrapper = captured_command[19] assert TIMEOUT_SENTINEL_PREFIX in timeout_wrapper assert 'trap _coding_agent_timeout TERM' in timeout_wrapper - assert '/bin/sh -c "$1" &' in timeout_wrapper - assert captured_command[18:] == ["sh", "python -V"] + assert 'setsid /bin/sh -c "$1" sh "$pidfile" "$2" &' in timeout_wrapper + assert 'printf "%s\\n" "$$" > "$1"' in child_wrapper + assert 'exec /bin/sh -c "$2"' in child_wrapper + assert captured_command[18:] == ["sh", child_wrapper, "python -V"] assert captured_kwargs == { "shell": False, "capture_output": True, @@ -323,8 +325,14 @@ def test_docker_cloud_client_timeout_contract_blocks_post_timeout_mutation( ) cleanup_called = threading.Event() + late_write_checked = threading.Event() observed_timeouts: list[object] = [] - timers: list[threading.Timer] = [] + + def maybe_late_write() -> None: + cleanup_happened = cleanup_called.wait(timeout=0.2) + if not cleanup_happened: + _ = marker.write_text("late write\n", encoding="utf-8") + late_write_checked.set() def fake_run( command: list[str], **kwargs: object @@ -332,14 +340,7 @@ def fake_run( local_timeout = kwargs.get("timeout") observed_timeouts.append(local_timeout) if local_timeout == 4: - timer = threading.Timer( - 0.05, - lambda: None - if cleanup_called.is_set() - else marker.write_text("late write\n", encoding="utf-8"), - ) - timers.append(timer) - timer.start() + threading.Thread(target=maybe_late_write, daemon=True).start() raise subprocess.TimeoutExpired(command, timeout=4) assert local_timeout == 3 cleanup_called.set() @@ -350,8 +351,8 @@ def fake_run( with pytest.raises(TimeoutError, match="docker exec command timed out after 1s"): _ = client.run_command("sleep 10", cwd="/workspace", env=None, timeout=1) - time.sleep(0.1) - assert cleanup_called.is_set() + assert cleanup_called.wait(timeout=0.2) + assert late_write_checked.wait(timeout=0.2) assert observed_timeouts == [4, 3] assert not marker.exists()