From 634c5dd0086271a28f538961ecd5655ed1dc71ef Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 7 Mar 2026 12:28:08 +0100 Subject: [PATCH 1/3] feat: implement subprocess sandbox for tool execution isolation (#131) Add a pluggable SandboxBackend protocol with a subprocess-based implementation that provides environment filtering, workspace boundary enforcement, timeout management, and PATH restriction for git tools. - SandboxBackend protocol (runtime_checkable) with execute/cleanup/health_check - SubprocessSandbox implementation with env allowlist/denylist, PATH filtering - SandboxResult frozen model with computed success field - SubprocessSandboxConfig with timeout, workspace_only, restricted_path options - SandboxError hierarchy inheriting from ToolError - Git tools integration: optional sandbox injection, dual code path in _run_git - Sandbox event constants for observability - 100% test coverage on sandbox module (unit + integration) Co-Authored-By: Claude Opus 4.6 --- .../observability/events/sandbox.py | 13 + src/ai_company/tools/__init__.py | 16 + src/ai_company/tools/_git_base.py | 135 ++++++- src/ai_company/tools/git_tools.py | 59 ++- src/ai_company/tools/sandbox/__init__.py | 17 + src/ai_company/tools/sandbox/config.py | 50 +++ src/ai_company/tools/sandbox/errors.py | 19 + src/ai_company/tools/sandbox/protocol.py | 55 +++ src/ai_company/tools/sandbox/result.py | 28 ++ .../tools/sandbox/subprocess_sandbox.py | 318 ++++++++++++++++ tests/integration/tools/__init__.py | 1 + .../tools/test_sandbox_integration.py | 120 ++++++ tests/unit/observability/test_events.py | 1 + .../tools/git/test_git_sandbox_integration.py | 162 ++++++++ tests/unit/tools/sandbox/__init__.py | 1 + tests/unit/tools/sandbox/conftest.py | 32 ++ tests/unit/tools/sandbox/test_config.py | 60 +++ tests/unit/tools/sandbox/test_errors.py | 48 +++ tests/unit/tools/sandbox/test_protocol.py | 56 +++ tests/unit/tools/sandbox/test_result.py | 71 ++++ .../tools/sandbox/test_subprocess_sandbox.py | 356 ++++++++++++++++++ 21 files changed, 1602 insertions(+), 16 deletions(-) create mode 100644 src/ai_company/observability/events/sandbox.py create mode 100644 src/ai_company/tools/sandbox/__init__.py create mode 100644 src/ai_company/tools/sandbox/config.py create mode 100644 src/ai_company/tools/sandbox/errors.py create mode 100644 src/ai_company/tools/sandbox/protocol.py create mode 100644 src/ai_company/tools/sandbox/result.py create mode 100644 src/ai_company/tools/sandbox/subprocess_sandbox.py create mode 100644 tests/integration/tools/__init__.py create mode 100644 tests/integration/tools/test_sandbox_integration.py create mode 100644 tests/unit/tools/git/test_git_sandbox_integration.py create mode 100644 tests/unit/tools/sandbox/__init__.py create mode 100644 tests/unit/tools/sandbox/conftest.py create mode 100644 tests/unit/tools/sandbox/test_config.py create mode 100644 tests/unit/tools/sandbox/test_errors.py create mode 100644 tests/unit/tools/sandbox/test_protocol.py create mode 100644 tests/unit/tools/sandbox/test_result.py create mode 100644 tests/unit/tools/sandbox/test_subprocess_sandbox.py diff --git a/src/ai_company/observability/events/sandbox.py b/src/ai_company/observability/events/sandbox.py new file mode 100644 index 0000000000..bfd51a9379 --- /dev/null +++ b/src/ai_company/observability/events/sandbox.py @@ -0,0 +1,13 @@ +"""Sandbox event constants.""" + +from typing import Final + +SANDBOX_EXECUTE_START: Final[str] = "sandbox.execute.start" +SANDBOX_EXECUTE_SUCCESS: Final[str] = "sandbox.execute.success" +SANDBOX_EXECUTE_FAILED: Final[str] = "sandbox.execute.failed" +SANDBOX_EXECUTE_TIMEOUT: Final[str] = "sandbox.execute.timeout" +SANDBOX_SPAWN_FAILED: Final[str] = "sandbox.spawn.failed" +SANDBOX_ENV_FILTERED: Final[str] = "sandbox.env.filtered" +SANDBOX_WORKSPACE_VIOLATION: Final[str] = "sandbox.workspace.violation" +SANDBOX_CLEANUP: Final[str] = "sandbox.cleanup" +SANDBOX_HEALTH_CHECK: Final[str] = "sandbox.health_check" diff --git a/src/ai_company/tools/__init__.py b/src/ai_company/tools/__init__.py index 2e32135a1c..b71e9bd9c4 100644 --- a/src/ai_company/tools/__init__.py +++ b/src/ai_company/tools/__init__.py @@ -29,6 +29,15 @@ from .invoker import ToolInvoker from .permissions import ToolPermissionChecker from .registry import ToolRegistry +from .sandbox import ( + SandboxBackend, + SandboxError, + SandboxResult, + SandboxStartError, + SandboxTimeoutError, + SubprocessSandbox, + SubprocessSandboxConfig, +) __all__ = [ "BaseFileSystemTool", @@ -45,6 +54,13 @@ "ListDirectoryTool", "PathValidator", "ReadFileTool", + "SandboxBackend", + "SandboxError", + "SandboxResult", + "SandboxStartError", + "SandboxTimeoutError", + "SubprocessSandbox", + "SubprocessSandboxConfig", "ToolError", "ToolExecutionError", "ToolExecutionResult", diff --git a/src/ai_company/tools/_git_base.py b/src/ai_company/tools/_git_base.py index 5d3504b6d7..96065d1d62 100644 --- a/src/ai_company/tools/_git_base.py +++ b/src/ai_company/tools/_git_base.py @@ -8,6 +8,11 @@ ``GIT_CONFIG_NOSYSTEM=1``, ``GIT_CONFIG_GLOBAL`` pointed to ``/dev/null``, and ``GIT_PROTOCOL_FROM_USER=0`` to prevent interactive prompts and restrict config/protocol attack surfaces. + +When a ``SandboxBackend`` is injected, subprocess management is +delegated to the sandbox, which provides environment filtering, +workspace enforcement, and timeout support on top of the git +hardening env vars. """ import asyncio @@ -15,7 +20,7 @@ import re from abc import ABC from pathlib import Path # noqa: TC003 — used at runtime -from typing import Any, Final +from typing import TYPE_CHECKING, Any, Final from ai_company.core.enums import ToolCategory from ai_company.observability import get_logger @@ -28,6 +33,11 @@ GIT_WORKSPACE_VIOLATION, ) from ai_company.tools.base import BaseTool, ToolExecutionResult +from ai_company.tools.sandbox.errors import SandboxError + +if TYPE_CHECKING: + from ai_company.tools.sandbox.protocol import SandboxBackend + from ai_company.tools.sandbox.result import SandboxResult logger = get_logger(__name__) @@ -35,6 +45,13 @@ _CREDENTIAL_RE = re.compile(r"(https?://)[^@/]+@") +_GIT_HARDENING_OVERRIDES: Final[dict[str, str]] = { + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_PROTOCOL_FROM_USER": "0", +} + def _sanitize_command(args: list[str]) -> list[str]: """Redact embedded credentials from git command args for logging.""" @@ -48,6 +65,10 @@ class _BaseGitTool(BaseTool, ABC): git commands and validating relative paths against the workspace boundary. + When a ``SandboxBackend`` is provided, ``_run_git`` delegates + subprocess management to the sandbox. Without a sandbox, the + existing direct-subprocess logic is used (backward compatible). + Attributes: workspace: Absolute path to the agent's workspace directory. """ @@ -59,6 +80,7 @@ def __init__( description: str, parameters_schema: dict[str, Any], workspace: Path, + sandbox: SandboxBackend | None = None, ) -> None: """Initialize a git tool bound to a workspace. @@ -67,6 +89,7 @@ def __init__( description: Human-readable description. parameters_schema: JSON Schema for tool parameters. workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. Raises: ValueError: If *workspace* is not an absolute path. @@ -81,6 +104,7 @@ def __init__( category=ToolCategory.VERSION_CONTROL, ) self._workspace = workspace.resolve() + self._sandbox = sandbox @property def workspace(self) -> Path: @@ -174,13 +198,16 @@ def _check_ref( @staticmethod def _build_git_env() -> dict[str, str]: """Build a hardened environment for git subprocesses.""" - return { - **os.environ, - "GIT_TERMINAL_PROMPT": "0", - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_CONFIG_GLOBAL": os.devnull, - "GIT_PROTOCOL_FROM_USER": "0", - } + return {**os.environ, **_GIT_HARDENING_OVERRIDES} + + @staticmethod + def _build_git_env_overrides() -> dict[str, str]: + """Return only git-specific hardening env vars. + + Used by the sandbox code path — the sandbox handles base env + filtering, and these overrides are applied on top. + """ + return dict(_GIT_HARDENING_OVERRIDES) async def _start_git_process( self, @@ -272,6 +299,51 @@ def _process_git_output( ) return ToolExecutionResult(content=stdout) + @staticmethod + def _sandbox_result_to_execution_result( + args: list[str], + result: SandboxResult, + ) -> ToolExecutionResult: + """Convert a ``SandboxResult`` to a ``ToolExecutionResult``. + + Mirrors ``_process_git_output`` but operates on the sandbox + result model. + + Args: + args: Git command arguments (for logging). + result: The sandbox execution result. + + Returns: + A ``ToolExecutionResult`` with the appropriate content. + """ + if result.timed_out: + logger.warning( + GIT_COMMAND_TIMEOUT, + command=_sanitize_command(["git", *args]), + deadline="sandbox", + ) + return ToolExecutionResult( + content=result.stderr or "Git command timed out", + is_error=True, + ) + if result.returncode != 0: + logger.warning( + GIT_COMMAND_FAILED, + command=_sanitize_command(["git", *args]), + returncode=result.returncode, + stderr=result.stderr, + stdout=result.stdout, + ) + return ToolExecutionResult( + content=(result.stderr or result.stdout or "Unknown git error"), + is_error=True, + ) + logger.debug( + GIT_COMMAND_SUCCESS, + command=_sanitize_command(["git", *args]), + ) + return ToolExecutionResult(content=result.stdout) + async def _run_git( self, args: list[str], @@ -281,6 +353,9 @@ async def _run_git( ) -> ToolExecutionResult: """Run a git subprocess and return the result. + When a sandbox backend is available, delegates execution to it. + Otherwise uses the direct subprocess path. + Args: args: Arguments to pass after ``git``. cwd: Working directory (defaults to workspace). @@ -291,7 +366,6 @@ async def _run_git( error message with ``is_error=True`` on failure. """ work_dir = cwd or self._workspace - env = self._build_git_env() logger.debug( GIT_COMMAND_START, @@ -299,6 +373,49 @@ async def _run_git( cwd=str(work_dir), ) + if self._sandbox is not None: + return await self._run_git_sandboxed(args, work_dir, deadline) + + return await self._run_git_direct(args, work_dir, deadline) + + async def _run_git_sandboxed( + self, + args: list[str], + work_dir: Path, + deadline: float, + ) -> ToolExecutionResult: + """Execute git through the sandbox backend.""" + assert self._sandbox is not None # noqa: S101 + + try: + result = await self._sandbox.execute( + command="git", + args=tuple(args), + cwd=work_dir, + env_overrides=self._build_git_env_overrides(), + timeout=deadline, + ) + except SandboxError as exc: + logger.warning( + GIT_COMMAND_FAILED, + command=_sanitize_command(["git", *args]), + error=str(exc), + ) + return ToolExecutionResult( + content=str(exc), + is_error=True, + ) + return self._sandbox_result_to_execution_result(args, result) + + async def _run_git_direct( + self, + args: list[str], + work_dir: Path, + deadline: float, + ) -> ToolExecutionResult: + """Execute git via direct subprocess (no sandbox).""" + env = self._build_git_env() + proc_or_err = await self._start_git_process( args, work_dir=work_dir, diff --git a/src/ai_company/tools/git_tools.py b/src/ai_company/tools/git_tools.py index 51d0688626..55705b444a 100644 --- a/src/ai_company/tools/git_tools.py +++ b/src/ai_company/tools/git_tools.py @@ -8,7 +8,7 @@ """ from pathlib import Path # noqa: TC003 — used at runtime -from typing import Any, Final +from typing import TYPE_CHECKING, Any, Final from ai_company.observability import get_logger from ai_company.observability.events.git import ( @@ -18,6 +18,9 @@ from ai_company.tools._git_base import _BaseGitTool from ai_company.tools.base import ToolExecutionResult +if TYPE_CHECKING: + from ai_company.tools.sandbox.protocol import SandboxBackend + logger = get_logger(__name__) _CLONE_TIMEOUT: Final[float] = 120.0 @@ -62,11 +65,17 @@ class GitStatusTool(_BaseGitTool): porcelain formatting. """ - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_status tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_status", @@ -91,6 +100,7 @@ def __init__(self, *, workspace: Path) -> None: "additionalProperties": False, }, workspace=workspace, + sandbox=sandbox, ) async def execute( @@ -167,11 +177,17 @@ class GitLogTool(_BaseGitTool): _MAX_COUNT_LIMIT: int = 100 - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_log tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_log", @@ -181,6 +197,7 @@ def __init__(self, *, workspace: Path) -> None: ), parameters_schema=_GIT_LOG_SCHEMA, workspace=workspace, + sandbox=sandbox, ) def _build_filter_args( @@ -260,11 +277,17 @@ class GitDiffTool(_BaseGitTool): staged changes view, stat summary, and path filtering. """ - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_diff tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_diff", @@ -303,6 +326,7 @@ def __init__(self, *, workspace: Path) -> None: "additionalProperties": False, }, workspace=workspace, + sandbox=sandbox, ) async def execute( # noqa: C901 @@ -367,11 +391,17 @@ class GitBranchTool(_BaseGitTool): _ACTIONS_REQUIRING_NAME = frozenset({"create", "switch", "delete"}) - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_branch tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_branch", @@ -415,6 +445,7 @@ def __init__(self, *, workspace: Path) -> None: "additionalProperties": False, }, workspace=workspace, + sandbox=sandbox, ) async def _list_branches(self) -> ToolExecutionResult: @@ -494,11 +525,17 @@ class GitCommitTool(_BaseGitTool): creates a commit with the provided message. """ - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_commit tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_commit", @@ -529,6 +566,7 @@ def __init__(self, *, workspace: Path) -> None: "additionalProperties": False, }, workspace=workspace, + sandbox=sandbox, ) async def execute( @@ -581,11 +619,17 @@ class GitCloneTool(_BaseGitTool): rejected. """ - def __init__(self, *, workspace: Path) -> None: + def __init__( + self, + *, + workspace: Path, + sandbox: SandboxBackend | None = None, + ) -> None: """Initialize the git_clone tool. Args: workspace: Absolute path to the workspace root. + sandbox: Optional sandbox backend for subprocess isolation. """ super().__init__( name="git_clone", @@ -618,6 +662,7 @@ def __init__(self, *, workspace: Path) -> None: "additionalProperties": False, }, workspace=workspace, + sandbox=sandbox, ) async def execute( diff --git a/src/ai_company/tools/sandbox/__init__.py b/src/ai_company/tools/sandbox/__init__.py new file mode 100644 index 0000000000..ce98ec968f --- /dev/null +++ b/src/ai_company/tools/sandbox/__init__.py @@ -0,0 +1,17 @@ +"""Subprocess sandbox for tool execution isolation.""" + +from .config import SubprocessSandboxConfig +from .errors import SandboxError, SandboxStartError, SandboxTimeoutError +from .protocol import SandboxBackend +from .result import SandboxResult +from .subprocess_sandbox import SubprocessSandbox + +__all__ = [ + "SandboxBackend", + "SandboxError", + "SandboxResult", + "SandboxStartError", + "SandboxTimeoutError", + "SubprocessSandbox", + "SubprocessSandboxConfig", +] diff --git a/src/ai_company/tools/sandbox/config.py b/src/ai_company/tools/sandbox/config.py new file mode 100644 index 0000000000..8798c37d68 --- /dev/null +++ b/src/ai_company/tools/sandbox/config.py @@ -0,0 +1,50 @@ +"""Subprocess sandbox configuration model.""" + +from pydantic import BaseModel, ConfigDict, Field + + +class SubprocessSandboxConfig(BaseModel): + """Configuration for the subprocess sandbox backend. + + Attributes: + timeout_seconds: Default command timeout in seconds. + workspace_only: Enforce cwd within the workspace boundary. + restricted_path: Filter PATH entries to safe directories. + env_allowlist: Environment variable names allowed to pass through. + Supports ``LC_*`` as a glob for all locale variables. + env_denylist_patterns: fnmatch patterns to strip even if in + the allowlist (e.g. ``*KEY*`` catches ``API_KEY``). + """ + + model_config = ConfigDict(frozen=True) + + timeout_seconds: float = Field( + default=30.0, + gt=0, + le=600, + ) + workspace_only: bool = True + restricted_path: bool = True + env_allowlist: tuple[str, ...] = ( + "HOME", + "PATH", + "USER", + "LANG", + "LC_*", + "TERM", + "TZ", + "TMPDIR", + "TEMP", + "TMP", + "SYSTEMROOT", + "WINDIR", + "COMSPEC", + ) + env_denylist_patterns: tuple[str, ...] = ( + "*KEY*", + "*SECRET*", + "*TOKEN*", + "*PASSWORD*", + "*CREDENTIAL*", + "*PRIVATE*", + ) diff --git a/src/ai_company/tools/sandbox/errors.py b/src/ai_company/tools/sandbox/errors.py new file mode 100644 index 0000000000..e71d76469e --- /dev/null +++ b/src/ai_company/tools/sandbox/errors.py @@ -0,0 +1,19 @@ +"""Sandbox error hierarchy. + +All sandbox errors inherit from ``ToolError`` so that sandbox failures +surface through the standard tool error path. +""" + +from ai_company.tools.errors import ToolError + + +class SandboxError(ToolError): + """Base exception for sandbox-layer errors.""" + + +class SandboxTimeoutError(SandboxError): + """Execution was killed because it exceeded the timeout.""" + + +class SandboxStartError(SandboxError): + """Failed to start the sandboxed subprocess.""" diff --git a/src/ai_company/tools/sandbox/protocol.py b/src/ai_company/tools/sandbox/protocol.py new file mode 100644 index 0000000000..6230c6020f --- /dev/null +++ b/src/ai_company/tools/sandbox/protocol.py @@ -0,0 +1,55 @@ +"""Sandbox backend protocol definition.""" + +from pathlib import Path # noqa: TC003 — used at runtime in Protocol +from typing import TYPE_CHECKING, Protocol, runtime_checkable + +if TYPE_CHECKING: + from collections.abc import Mapping + + from ai_company.tools.sandbox.result import SandboxResult + + +@runtime_checkable +class SandboxBackend(Protocol): + """Protocol for pluggable sandbox backends. + + Implementations execute commands in an isolated environment with + environment filtering, workspace enforcement, and timeout support. + Subprocess is the M3 backend; Docker/K8s are future work. + """ + + async def execute( + self, + *, + command: str, + args: tuple[str, ...], + cwd: Path | None = None, + env_overrides: Mapping[str, str] | None = None, + timeout: float | None = None, # noqa: ASYNC109 + ) -> SandboxResult: + """Execute a command in the sandbox. + + Args: + command: Executable name or path. + args: Command arguments. + cwd: Working directory (defaults to sandbox workspace root). + env_overrides: Extra env vars applied on top of filtered env. + timeout: Seconds before the process is killed. Falls back + to the backend's default timeout if ``None``. + + Returns: + A ``SandboxResult`` with captured output and exit status. + """ + ... + + async def cleanup(self) -> None: + """Release any resources held by the backend.""" + ... + + async def health_check(self) -> bool: + """Return ``True`` if the backend is operational.""" + ... + + def get_backend_type(self) -> str: + """Return a short identifier for this backend type.""" + ... diff --git a/src/ai_company/tools/sandbox/result.py b/src/ai_company/tools/sandbox/result.py new file mode 100644 index 0000000000..2ca7359692 --- /dev/null +++ b/src/ai_company/tools/sandbox/result.py @@ -0,0 +1,28 @@ +"""Sandbox execution result model.""" + +from pydantic import BaseModel, ConfigDict, computed_field + + +class SandboxResult(BaseModel): + """Immutable result of a sandboxed command execution. + + Attributes: + stdout: Captured standard output. + stderr: Captured standard error. + returncode: Process exit code. + timed_out: Whether the process was killed due to timeout. + success: Computed — ``True`` when returncode is 0 and not timed out. + """ + + model_config = ConfigDict(frozen=True) + + stdout: str + stderr: str + returncode: int + timed_out: bool = False + + @computed_field # type: ignore[prop-decorator] + @property + def success(self) -> bool: + """Whether the execution succeeded.""" + return self.returncode == 0 and not self.timed_out diff --git a/src/ai_company/tools/sandbox/subprocess_sandbox.py b/src/ai_company/tools/sandbox/subprocess_sandbox.py new file mode 100644 index 0000000000..45bdb6254e --- /dev/null +++ b/src/ai_company/tools/sandbox/subprocess_sandbox.py @@ -0,0 +1,318 @@ +"""Subprocess-based sandbox backend. + +Executes commands via ``asyncio.create_subprocess_exec`` with +environment filtering, workspace boundary enforcement, timeout +management, and PATH restriction. +""" + +import asyncio +import contextlib +import fnmatch +import os +from pathlib import Path +from typing import TYPE_CHECKING + +from ai_company.observability import get_logger +from ai_company.observability.events.sandbox import ( + SANDBOX_CLEANUP, + SANDBOX_ENV_FILTERED, + SANDBOX_EXECUTE_FAILED, + SANDBOX_EXECUTE_START, + SANDBOX_EXECUTE_SUCCESS, + SANDBOX_EXECUTE_TIMEOUT, + SANDBOX_HEALTH_CHECK, + SANDBOX_SPAWN_FAILED, + SANDBOX_WORKSPACE_VIOLATION, +) +from ai_company.tools.sandbox.config import SubprocessSandboxConfig +from ai_company.tools.sandbox.errors import ( + SandboxError, + SandboxStartError, +) +from ai_company.tools.sandbox.result import SandboxResult + +if TYPE_CHECKING: + from collections.abc import Mapping + +logger = get_logger(__name__) + +_PATH_SEP = ";" if os.name == "nt" else ":" + +_DEFAULT_CONFIG = SubprocessSandboxConfig() + + +class SubprocessSandbox: + """Subprocess sandbox backend. + + Runs commands in child processes with filtered environment variables, + workspace boundary checks, and configurable timeouts. + + Attributes: + config: Sandbox configuration. + workspace: Absolute path to the workspace root directory. + """ + + def __init__( + self, + *, + config: SubprocessSandboxConfig | None = None, + workspace: Path, + ) -> None: + """Initialize the subprocess sandbox. + + Args: + config: Sandbox configuration (defaults to standard config). + workspace: Absolute path to the workspace root. Must exist. + + Raises: + ValueError: If *workspace* is not absolute or does not exist. + """ + if not workspace.is_absolute(): + msg = f"workspace must be an absolute path, got: {workspace}" + raise ValueError(msg) + resolved = workspace.resolve() + if not resolved.is_dir(): + msg = f"workspace directory does not exist: {resolved}" + raise ValueError(msg) + self._config = config or _DEFAULT_CONFIG + self._workspace = resolved + + @property + def config(self) -> SubprocessSandboxConfig: + """Sandbox configuration.""" + return self._config + + @property + def workspace(self) -> Path: + """Workspace root directory.""" + return self._workspace + + def _matches_allowlist(self, name: str) -> bool: + """Check if an env var name matches any entry in the allowlist.""" + for pattern in self._config.env_allowlist: + if pattern == name: + return True + if fnmatch.fnmatch(name, pattern): + return True + return False + + def _matches_denylist(self, name: str) -> bool: + """Check if an env var name matches any denylist pattern.""" + upper = name.upper() + return any( + fnmatch.fnmatch(upper, pat) for pat in self._config.env_denylist_patterns + ) + + def _filter_path(self, path_value: str) -> str: + """Filter PATH entries, keeping only safe system directories.""" + safe_prefixes = self._get_safe_path_prefixes() + entries = path_value.split(_PATH_SEP) + filtered = [ + e + for e in entries + if any(e.lower().startswith(prefix.lower()) for prefix in safe_prefixes) + ] + return _PATH_SEP.join(filtered) if filtered else path_value + + @staticmethod + def _get_safe_path_prefixes() -> tuple[str, ...]: + """Return safe PATH prefixes for the current platform.""" + if os.name == "nt": + system_root = os.environ.get("SYSTEMROOT", r"C:\WINDOWS") + return ( + system_root, + str(Path(system_root) / "system32"), + r"C:\Program Files\Git", + r"C:\Program Files (x86)\Git", + ) + return ("/usr/bin", "/usr/local/bin", "/bin", "/usr/sbin", "/sbin") + + def _build_filtered_env( + self, + env_overrides: Mapping[str, str] | None = None, + ) -> dict[str, str]: + """Build a filtered environment for the subprocess. + + Starts with an empty dict, copies allowed vars from the current + process environment, strips denylist matches, optionally filters + PATH, then applies overrides. + + Args: + env_overrides: Extra vars applied on top (always win). + + Returns: + The filtered environment mapping. + """ + env: dict[str, str] = {} + filtered_count = 0 + + for name, value in os.environ.items(): + if self._matches_allowlist(name) and not self._matches_denylist( + name, + ): + env[name] = value + else: + filtered_count += 1 + + if self._config.restricted_path and "PATH" in env: + env["PATH"] = self._filter_path(env["PATH"]) + + if env_overrides: + env.update(env_overrides) + + logger.debug( + SANDBOX_ENV_FILTERED, + filtered_count=filtered_count, + kept_count=len(env), + ) + return env + + def _validate_cwd(self, cwd: Path) -> None: + """Validate that *cwd* is within the workspace boundary. + + Args: + cwd: Working directory to validate. + + Raises: + SandboxError: If *cwd* is outside the workspace and + ``workspace_only`` is enabled. + """ + if not self._config.workspace_only: + return + try: + cwd.resolve().relative_to(self._workspace) + except ValueError: + logger.warning( + SANDBOX_WORKSPACE_VIOLATION, + cwd=str(cwd), + workspace=str(self._workspace), + ) + msg = f"Working directory '{cwd}' is outside workspace '{self._workspace}'" + raise SandboxError(msg) from None + + async def execute( + self, + *, + command: str, + args: tuple[str, ...], + cwd: Path | None = None, + env_overrides: Mapping[str, str] | None = None, + timeout: float | None = None, # noqa: ASYNC109 + ) -> SandboxResult: + """Execute a command in the sandbox. + + Args: + command: Executable name or path. + args: Command arguments. + cwd: Working directory (defaults to workspace root). + env_overrides: Extra env vars applied on top of filtered env. + timeout: Seconds before the process is killed. + + Returns: + A ``SandboxResult`` with captured output and exit status. + + Raises: + SandboxStartError: If the subprocess could not be started. + SandboxError: If cwd is outside the workspace boundary. + """ + work_dir = cwd or self._workspace + self._validate_cwd(work_dir) + + effective_timeout = timeout or self._config.timeout_seconds + env = self._build_filtered_env(env_overrides) + + logger.debug( + SANDBOX_EXECUTE_START, + command=command, + args=args, + cwd=str(work_dir), + timeout=effective_timeout, + ) + + try: + proc = await asyncio.create_subprocess_exec( + command, + *args, + cwd=work_dir, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + env=env, + ) + except OSError as exc: + logger.warning( + SANDBOX_SPAWN_FAILED, + command=command, + error=str(exc), + exc_info=True, + ) + msg = f"Failed to start '{command}': {exc}" + raise SandboxStartError( + msg, + context={"command": command}, + ) from exc + + try: + stdout_bytes, stderr_bytes = await asyncio.wait_for( + proc.communicate(), + timeout=effective_timeout, + ) + except TimeoutError: + proc.kill() + with contextlib.suppress(TimeoutError): + await asyncio.wait_for(proc.communicate(), timeout=5.0) + logger.warning( + SANDBOX_EXECUTE_TIMEOUT, + command=command, + args=args, + timeout=effective_timeout, + ) + return SandboxResult( + stdout="", + stderr=f"Process timed out after {effective_timeout}s", + returncode=-1, + timed_out=True, + ) + + stdout = stdout_bytes.decode("utf-8", errors="replace").strip() + stderr = stderr_bytes.decode("utf-8", errors="replace").strip() + returncode = proc.returncode or 0 + + if returncode != 0: + logger.warning( + SANDBOX_EXECUTE_FAILED, + command=command, + args=args, + returncode=returncode, + stderr=stderr, + ) + else: + logger.debug( + SANDBOX_EXECUTE_SUCCESS, + command=command, + args=args, + ) + + return SandboxResult( + stdout=stdout, + stderr=stderr, + returncode=returncode, + ) + + async def cleanup(self) -> None: + """No-op — subprocesses are ephemeral.""" + logger.debug(SANDBOX_CLEANUP, backend="subprocess") + + async def health_check(self) -> bool: + """Return ``True`` if the workspace directory exists.""" + healthy = self._workspace.is_dir() + logger.debug( + SANDBOX_HEALTH_CHECK, + backend="subprocess", + healthy=healthy, + workspace=str(self._workspace), + ) + return healthy + + def get_backend_type(self) -> str: + """Return ``'subprocess'``.""" + return "subprocess" diff --git a/tests/integration/tools/__init__.py b/tests/integration/tools/__init__.py new file mode 100644 index 0000000000..35b0f73aa1 --- /dev/null +++ b/tests/integration/tools/__init__.py @@ -0,0 +1 @@ +"""Integration tests for the tool system.""" diff --git a/tests/integration/tools/test_sandbox_integration.py b/tests/integration/tools/test_sandbox_integration.py new file mode 100644 index 0000000000..447e89692b --- /dev/null +++ b/tests/integration/tools/test_sandbox_integration.py @@ -0,0 +1,120 @@ +"""Integration tests for subprocess sandbox with real git.""" + +import os +import subprocess +from pathlib import Path # noqa: TC003 — used at runtime + +import pytest + +from ai_company.tools.git_tools import GitStatusTool +from ai_company.tools.sandbox.config import SubprocessSandboxConfig +from ai_company.tools.sandbox.errors import SandboxError +from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox + +pytestmark = [pytest.mark.integration, pytest.mark.timeout(30)] + +_GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@test.local", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@test.local", + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_PROTOCOL_FROM_USER": "0", +} + + +def _init_repo(path: Path) -> None: + """Initialize a git repo with one commit.""" + for args in ( + ["init"], + ["config", "user.name", "Test"], + ["config", "user.email", "test@test.local"], + ): + subprocess.run( # noqa: S603 + ["git", *args], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + (path / "README.md").write_text("# Test\n") + subprocess.run( + ["git", "add", "."], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + subprocess.run( + ["git", "commit", "-m", "initial"], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + + +class TestRealGitWithSandbox: + """Real git repo + SubprocessSandbox + GitStatusTool.""" + + async def test_git_status_via_sandbox(self, tmp_path: Path) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert not result.is_error + + async def test_git_status_porcelain_via_sandbox( + self, + tmp_path: Path, + ) -> None: + _init_repo(tmp_path) + (tmp_path / "new.txt").write_text("new file") + sandbox = SubprocessSandbox(workspace=tmp_path) + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={"porcelain": True}) + assert not result.is_error + assert "new.txt" in result.content + + +class TestSandboxWorkspaceEscape: + """Sandbox blocks workspace escape in real subprocess.""" + + async def test_cwd_escape_blocked(self, tmp_path: Path) -> None: + workspace = tmp_path / "workspace" + workspace.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + sandbox = SubprocessSandbox(workspace=workspace) + with pytest.raises(SandboxError, match="outside workspace"): + await sandbox.execute( + command="echo", + args=("test",), + cwd=outside, + ) + + +class TestSandboxTimeout: + """Sandbox timeout on slow command.""" + + async def test_timeout_on_slow_command(self, tmp_path: Path) -> None: + sandbox = SubprocessSandbox( + workspace=tmp_path, + config=SubprocessSandboxConfig(timeout_seconds=1.0), + ) + if os.name == "nt": + result = await sandbox.execute( + command="cmd", + args=("/c", "ping", "-n", "10", "127.0.0.1"), + timeout=0.5, + ) + else: + result = await sandbox.execute( + command="sleep", + args=("10",), + timeout=0.5, + ) + assert result.timed_out + assert not result.success diff --git a/tests/unit/observability/test_events.py b/tests/unit/observability/test_events.py index c6071671c5..6550acd97c 100644 --- a/tests/unit/observability/test_events.py +++ b/tests/unit/observability/test_events.py @@ -89,6 +89,7 @@ def test_all_domain_modules_discovered(self) -> None: "provider", "role", "routing", + "sandbox", "task", "template", "tool", diff --git a/tests/unit/tools/git/test_git_sandbox_integration.py b/tests/unit/tools/git/test_git_sandbox_integration.py new file mode 100644 index 0000000000..ab5c678514 --- /dev/null +++ b/tests/unit/tools/git/test_git_sandbox_integration.py @@ -0,0 +1,162 @@ +"""Tests for git tools with sandbox integration.""" + +import os +import subprocess +from pathlib import Path # noqa: TC003 — used at runtime +from unittest.mock import AsyncMock + +import pytest + +from ai_company.tools.git_tools import GitLogTool, GitStatusTool +from ai_company.tools.sandbox.errors import SandboxError +from ai_company.tools.sandbox.result import SandboxResult +from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + +_GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@test.local", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@test.local", + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_PROTOCOL_FROM_USER": "0", +} + + +def _init_repo(path: Path) -> None: + """Initialize a git repo with one commit.""" + for args in ( + ["init"], + ["config", "user.name", "Test"], + ["config", "user.email", "test@test.local"], + ): + subprocess.run( # noqa: S603 + ["git", *args], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + (path / "README.md").write_text("# Test\n") + subprocess.run( + ["git", "add", "."], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + subprocess.run( + ["git", "commit", "-m", "initial"], # noqa: S607 + cwd=path, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + + +class TestGitToolsWithSandbox: + """Git tools work when a sandbox is injected.""" + + async def test_status_with_sandbox(self, tmp_path: Path) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert not result.is_error + + async def test_log_with_sandbox(self, tmp_path: Path) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + tool = GitLogTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={"max_count": 1}) + assert not result.is_error + assert "initial" in result.content + + +class TestGitToolsWithoutSandbox: + """Git tools work without sandbox (backward compat).""" + + async def test_status_without_sandbox(self, tmp_path: Path) -> None: + _init_repo(tmp_path) + tool = GitStatusTool(workspace=tmp_path) + result = await tool.execute(arguments={}) + assert not result.is_error + + async def test_log_without_sandbox(self, tmp_path: Path) -> None: + _init_repo(tmp_path) + tool = GitLogTool(workspace=tmp_path) + result = await tool.execute(arguments={"max_count": 1}) + assert not result.is_error + assert "initial" in result.content + + +class TestSandboxTimeoutSurfaces: + """Sandbox timeout surfaces as ToolExecutionResult(is_error=True).""" + + async def test_sandbox_timeout_returns_error( + self, + tmp_path: Path, + ) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + # Mock execute to return a timed-out result + sandbox.execute = AsyncMock( # type: ignore[method-assign] + return_value=SandboxResult( + stdout="", + stderr="Process timed out after 1s", + returncode=-1, + timed_out=True, + ), + ) + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert result.is_error + assert "timed out" in result.content.lower() + + +class TestSandboxErrorSurfaces: + """SandboxError surfaces as ToolExecutionResult(is_error=True).""" + + async def test_sandbox_error_returns_error( + self, + tmp_path: Path, + ) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + sandbox.execute = AsyncMock( # type: ignore[method-assign] + side_effect=SandboxError("workspace violation"), + ) + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert result.is_error + assert "workspace violation" in result.content + + +class TestGitHardeningWithSandbox: + """Git hardening env vars are passed via env_overrides.""" + + async def test_env_overrides_contain_hardening_vars( + self, + tmp_path: Path, + ) -> None: + _init_repo(tmp_path) + sandbox = SubprocessSandbox(workspace=tmp_path) + original_execute = sandbox.execute + captured_overrides: dict[str, str] = {} + + async def capture_execute(**kwargs: object) -> SandboxResult: + overrides = kwargs.get("env_overrides") + if isinstance(overrides, dict): + captured_overrides.update(overrides) + return await original_execute(**kwargs) # type: ignore[arg-type] + + sandbox.execute = capture_execute # type: ignore[method-assign] + tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + await tool.execute(arguments={}) + + assert captured_overrides.get("GIT_TERMINAL_PROMPT") == "0" + assert captured_overrides.get("GIT_CONFIG_NOSYSTEM") == "1" + assert captured_overrides.get("GIT_PROTOCOL_FROM_USER") == "0" diff --git a/tests/unit/tools/sandbox/__init__.py b/tests/unit/tools/sandbox/__init__.py new file mode 100644 index 0000000000..b030fb3235 --- /dev/null +++ b/tests/unit/tools/sandbox/__init__.py @@ -0,0 +1 @@ +"""Sandbox tool unit tests.""" diff --git a/tests/unit/tools/sandbox/conftest.py b/tests/unit/tools/sandbox/conftest.py new file mode 100644 index 0000000000..b8d5d05bf5 --- /dev/null +++ b/tests/unit/tools/sandbox/conftest.py @@ -0,0 +1,32 @@ +"""Fixtures for sandbox tests.""" + +from pathlib import Path # noqa: TC003 — pytest evaluates annotations + +import pytest + +from ai_company.tools.sandbox.config import SubprocessSandboxConfig +from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox + + +@pytest.fixture +def sandbox_workspace(tmp_path: Path) -> Path: + """Temporary workspace directory for sandbox tests.""" + return tmp_path + + +@pytest.fixture +def sandbox_config() -> SubprocessSandboxConfig: + """Default sandbox configuration.""" + return SubprocessSandboxConfig() + + +@pytest.fixture +def subprocess_sandbox( + sandbox_workspace: Path, + sandbox_config: SubprocessSandboxConfig, +) -> SubprocessSandbox: + """SubprocessSandbox instance with default config.""" + return SubprocessSandbox( + config=sandbox_config, + workspace=sandbox_workspace, + ) diff --git a/tests/unit/tools/sandbox/test_config.py b/tests/unit/tools/sandbox/test_config.py new file mode 100644 index 0000000000..7d1232d973 --- /dev/null +++ b/tests/unit/tools/sandbox/test_config.py @@ -0,0 +1,60 @@ +"""Tests for SubprocessSandboxConfig model.""" + +import pytest +from pydantic import ValidationError + +from ai_company.tools.sandbox.config import SubprocessSandboxConfig + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + + +class TestSubprocessSandboxConfig: + """SubprocessSandboxConfig defaults, validation, and immutability.""" + + def test_defaults(self) -> None: + config = SubprocessSandboxConfig() + assert config.timeout_seconds == 30.0 + assert config.workspace_only is True + assert config.restricted_path is True + assert isinstance(config.env_allowlist, tuple) + assert isinstance(config.env_denylist_patterns, tuple) + + def test_timeout_must_be_positive(self) -> None: + with pytest.raises(ValidationError): + SubprocessSandboxConfig(timeout_seconds=0) + + def test_timeout_must_not_exceed_max(self) -> None: + with pytest.raises(ValidationError): + SubprocessSandboxConfig(timeout_seconds=601) + + def test_timeout_at_max_boundary(self) -> None: + config = SubprocessSandboxConfig(timeout_seconds=600) + assert config.timeout_seconds == 600 + + def test_custom_allowlist(self) -> None: + config = SubprocessSandboxConfig( + env_allowlist=("HOME", "CUSTOM"), + ) + assert config.env_allowlist == ("HOME", "CUSTOM") + + def test_custom_denylist_patterns(self) -> None: + config = SubprocessSandboxConfig( + env_denylist_patterns=("*DANGER*",), + ) + assert config.env_denylist_patterns == ("*DANGER*",) + + def test_frozen(self) -> None: + config = SubprocessSandboxConfig() + with pytest.raises(ValidationError): + config.timeout_seconds = 10.0 # type: ignore[misc] + + def test_env_allowlist_contains_path(self) -> None: + config = SubprocessSandboxConfig() + assert "PATH" in config.env_allowlist + + def test_denylist_patterns_cover_secrets(self) -> None: + config = SubprocessSandboxConfig() + patterns = config.env_denylist_patterns + assert any("KEY" in p for p in patterns) + assert any("SECRET" in p for p in patterns) + assert any("TOKEN" in p for p in patterns) diff --git a/tests/unit/tools/sandbox/test_errors.py b/tests/unit/tools/sandbox/test_errors.py new file mode 100644 index 0000000000..6eb7c1382b --- /dev/null +++ b/tests/unit/tools/sandbox/test_errors.py @@ -0,0 +1,48 @@ +"""Tests for sandbox error hierarchy.""" + +import pytest + +from ai_company.tools.errors import ToolError +from ai_company.tools.sandbox.errors import ( + SandboxError, + SandboxStartError, + SandboxTimeoutError, +) + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + + +class TestSandboxErrors: + """Sandbox errors inherit from ToolError and carry context.""" + + def test_sandbox_error_inherits_tool_error(self) -> None: + err = SandboxError("boom") + assert isinstance(err, ToolError) + + def test_sandbox_timeout_inherits_sandbox_error(self) -> None: + err = SandboxTimeoutError("timed out") + assert isinstance(err, SandboxError) + assert isinstance(err, ToolError) + + def test_sandbox_start_inherits_sandbox_error(self) -> None: + err = SandboxStartError("start failed") + assert isinstance(err, SandboxError) + assert isinstance(err, ToolError) + + def test_error_message(self) -> None: + err = SandboxError("test message") + assert err.message == "test message" + assert str(err) == "test message" + + def test_error_with_context(self) -> None: + err = SandboxStartError( + "failed", + context={"command": "git"}, + ) + assert err.context["command"] == "git" + assert "git" in str(err) + + def test_error_context_is_immutable(self) -> None: + err = SandboxError("boom", context={"key": "value"}) + with pytest.raises(TypeError): + err.context["new_key"] = "nope" # type: ignore[index] diff --git a/tests/unit/tools/sandbox/test_protocol.py b/tests/unit/tools/sandbox/test_protocol.py new file mode 100644 index 0000000000..4f673bf99c --- /dev/null +++ b/tests/unit/tools/sandbox/test_protocol.py @@ -0,0 +1,56 @@ +"""Tests for SandboxBackend protocol.""" + +from collections.abc import Mapping # noqa: TC003 — used at runtime +from pathlib import Path # noqa: TC003 — used at runtime + +import pytest + +from ai_company.tools.sandbox.protocol import SandboxBackend +from ai_company.tools.sandbox.result import SandboxResult +from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox # noqa: TC001 + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + + +class _FakeSandbox: + """Minimal fake that satisfies the SandboxBackend protocol.""" + + async def execute( + self, + *, + command: str, + args: tuple[str, ...], + cwd: Path | None = None, + env_overrides: Mapping[str, str] | None = None, + timeout: float | None = None, # noqa: ASYNC109 + ) -> SandboxResult: + return SandboxResult( + stdout="fake", + stderr="", + returncode=0, + ) + + async def cleanup(self) -> None: + pass + + async def health_check(self) -> bool: + return True + + def get_backend_type(self) -> str: + return "fake" + + +class TestSandboxBackendProtocol: + """SandboxBackend is runtime_checkable and satisfied by impls.""" + + def test_protocol_is_runtime_checkable(self) -> None: + assert isinstance(_FakeSandbox(), SandboxBackend) + + def test_subprocess_sandbox_satisfies_protocol( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + assert isinstance(subprocess_sandbox, SandboxBackend) + + def test_arbitrary_object_does_not_satisfy(self) -> None: + assert not isinstance(object(), SandboxBackend) diff --git a/tests/unit/tools/sandbox/test_result.py b/tests/unit/tools/sandbox/test_result.py new file mode 100644 index 0000000000..8344663091 --- /dev/null +++ b/tests/unit/tools/sandbox/test_result.py @@ -0,0 +1,71 @@ +"""Tests for SandboxResult model.""" + +import pytest +from pydantic import ValidationError + +from ai_company.tools.sandbox.result import SandboxResult + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + + +class TestSandboxResult: + """SandboxResult is frozen with a computed ``success`` field.""" + + def test_success_when_zero_returncode_no_timeout(self) -> None: + result = SandboxResult( + stdout="ok", + stderr="", + returncode=0, + ) + assert result.success is True + + def test_failure_when_nonzero_returncode(self) -> None: + result = SandboxResult( + stdout="", + stderr="error", + returncode=1, + ) + assert result.success is False + + def test_failure_when_timed_out(self) -> None: + result = SandboxResult( + stdout="", + stderr="timeout", + returncode=0, + timed_out=True, + ) + assert result.success is False + + def test_failure_when_both_nonzero_and_timed_out(self) -> None: + result = SandboxResult( + stdout="", + stderr="", + returncode=-1, + timed_out=True, + ) + assert result.success is False + + def test_frozen(self) -> None: + result = SandboxResult( + stdout="ok", + stderr="", + returncode=0, + ) + with pytest.raises(ValidationError): + result.stdout = "modified" # type: ignore[misc] + + def test_timed_out_defaults_false(self) -> None: + result = SandboxResult( + stdout="", + stderr="", + returncode=0, + ) + assert result.timed_out is False + + def test_negative_returncode(self) -> None: + result = SandboxResult( + stdout="", + stderr="signal", + returncode=-9, + ) + assert result.success is False diff --git a/tests/unit/tools/sandbox/test_subprocess_sandbox.py b/tests/unit/tools/sandbox/test_subprocess_sandbox.py new file mode 100644 index 0000000000..e318f4dfe6 --- /dev/null +++ b/tests/unit/tools/sandbox/test_subprocess_sandbox.py @@ -0,0 +1,356 @@ +"""Tests for SubprocessSandbox implementation.""" + +import os +from pathlib import Path +from unittest.mock import patch + +import pytest + +from ai_company.tools.sandbox.config import SubprocessSandboxConfig +from ai_company.tools.sandbox.errors import SandboxError, SandboxStartError +from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox + +pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] + + +# ── Constructor ────────────────────────────────────────────────── + + +class TestSubprocessSandboxInit: + """Constructor validation.""" + + def test_workspace_must_be_absolute(self, tmp_path: Path) -> None: + with pytest.raises(ValueError, match="absolute path"): + SubprocessSandbox(workspace=Path("relative")) + + def test_workspace_must_exist(self, tmp_path: Path) -> None: + missing = tmp_path / "nonexistent" + with pytest.raises(ValueError, match="does not exist"): + SubprocessSandbox(workspace=missing) + + def test_valid_workspace(self, tmp_path: Path) -> None: + sandbox = SubprocessSandbox(workspace=tmp_path) + assert sandbox.workspace == tmp_path.resolve() + + def test_default_config(self, tmp_path: Path) -> None: + sandbox = SubprocessSandbox(workspace=tmp_path) + assert sandbox.config.timeout_seconds == 30.0 + + +# ── Environment filtering ─────────────────────────────────────── + + +class TestEnvironmentFiltering: + """Environment variable filtering and denylist enforcement.""" + + def test_secrets_stripped( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + with patch.dict( + os.environ, + {"API_KEY": "secret123", "HOME": "/home/test"}, + clear=True, + ): + env = subprocess_sandbox._build_filtered_env() + assert "API_KEY" not in env + assert "HOME" in env + + def test_allowlist_passes_through( + self, + sandbox_workspace: Path, + ) -> None: + config = SubprocessSandboxConfig( + env_allowlist=("CUSTOM_VAR",), + env_denylist_patterns=(), + ) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + with patch.dict( + os.environ, + {"CUSTOM_VAR": "yes", "OTHER": "no"}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert env["CUSTOM_VAR"] == "yes" + assert "OTHER" not in env + + def test_denylist_overrides_allowlist( + self, + sandbox_workspace: Path, + ) -> None: + config = SubprocessSandboxConfig( + env_allowlist=("MY_SECRET_KEY",), + env_denylist_patterns=("*SECRET*",), + ) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + with patch.dict( + os.environ, + {"MY_SECRET_KEY": "hidden"}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert "MY_SECRET_KEY" not in env + + def test_env_overrides_applied( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + with patch.dict(os.environ, {}, clear=True): + env = subprocess_sandbox._build_filtered_env( + env_overrides={"GIT_TERMINAL_PROMPT": "0"}, + ) + assert env["GIT_TERMINAL_PROMPT"] == "0" + + def test_lc_glob_matching( + self, + sandbox_workspace: Path, + ) -> None: + config = SubprocessSandboxConfig( + env_allowlist=("LC_*",), + env_denylist_patterns=(), + restricted_path=False, + ) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + with patch.dict( + os.environ, + {"LC_ALL": "en_US.UTF-8", "HOME": "/home/test"}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert "LC_ALL" in env + assert "HOME" not in env + + def test_restricted_path_filters_entries( + self, + sandbox_workspace: Path, + ) -> None: + config = SubprocessSandboxConfig(restricted_path=True) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + if os.name == "nt": + fake_path = r"C:\WINDOWS\system32;C:\suspicious\dir" + else: + fake_path = "/usr/bin:/suspicious/dir" + with patch.dict( + os.environ, + {"PATH": fake_path}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert "suspicious" not in env.get("PATH", "").lower() + + +# ── Workspace boundary ─────────────────────────────────────────── + + +class TestWorkspaceBoundary: + """Workspace cwd validation.""" + + def test_traversal_blocked( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + outside = subprocess_sandbox.workspace.parent / "outside" + outside.mkdir(exist_ok=True) + with pytest.raises(SandboxError, match="outside workspace"): + subprocess_sandbox._validate_cwd(outside) + + def test_valid_cwd_accepted( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + subdir = subprocess_sandbox.workspace / "sub" + subdir.mkdir() + subprocess_sandbox._validate_cwd(subdir) + + def test_workspace_root_accepted( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + subprocess_sandbox._validate_cwd(subprocess_sandbox.workspace) + + def test_workspace_only_disabled( + self, + sandbox_workspace: Path, + ) -> None: + config = SubprocessSandboxConfig(workspace_only=False) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + outside = sandbox_workspace.parent / "outside" + outside.mkdir(exist_ok=True) + # Should not raise + sandbox._validate_cwd(outside) + + +# ── Execution ──────────────────────────────────────────────────── + + +class TestExecution: + """Command execution tests.""" + + async def test_successful_command( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "echo", "hello"), + ) + else: + result = await subprocess_sandbox.execute( + command="echo", + args=("hello",), + ) + assert result.success + assert "hello" in result.stdout + + async def test_failed_command( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "exit", "1"), + ) + else: + result = await subprocess_sandbox.execute( + command="sh", + args=("-c", "exit 1"), + ) + assert not result.success + assert result.returncode != 0 + + async def test_timeout_kills_process( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "ping", "-n", "10", "127.0.0.1"), + timeout=0.5, + ) + else: + result = await subprocess_sandbox.execute( + command="sleep", + args=("10",), + timeout=0.5, + ) + assert result.timed_out + assert not result.success + + async def test_start_failure( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + with pytest.raises(SandboxStartError, match="Failed to start"): + await subprocess_sandbox.execute( + command="nonexistent_binary_xyz", + args=(), + ) + + async def test_default_cwd_is_workspace( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "cd"), + ) + else: + result = await subprocess_sandbox.execute( + command="pwd", + args=(), + ) + assert result.success + # Compare resolved paths (noqa: pathlib in async is fine for tests) + expected = subprocess_sandbox.workspace.resolve() + actual = Path(result.stdout.strip()).resolve() # noqa: ASYNC240 + assert actual == expected + + async def test_custom_cwd_within_workspace( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + subdir = subprocess_sandbox.workspace / "sub" + subdir.mkdir() + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "cd"), + cwd=subdir, + ) + else: + result = await subprocess_sandbox.execute( + command="pwd", + args=(), + cwd=subdir, + ) + assert result.success + actual = Path(result.stdout.strip()).resolve() # noqa: ASYNC240 + assert actual == subdir.resolve() + + async def test_cwd_outside_workspace_blocked( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + outside = subprocess_sandbox.workspace.parent / "outside" + outside.mkdir(exist_ok=True) + with pytest.raises(SandboxError, match="outside workspace"): + await subprocess_sandbox.execute( + command="echo", + args=("test",), + cwd=outside, + ) + + +# ── Health check & cleanup ─────────────────────────────────────── + + +class TestHealthCheckAndCleanup: + """Health check and cleanup behavior.""" + + async def test_health_check_valid_workspace( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + assert await subprocess_sandbox.health_check() is True + + async def test_health_check_missing_workspace( + self, + tmp_path: Path, + ) -> None: + workspace = tmp_path / "exists" + workspace.mkdir() + sandbox = SubprocessSandbox(workspace=workspace) + workspace.rmdir() + assert await sandbox.health_check() is False + + async def test_cleanup_is_noop( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + # Should not raise + await subprocess_sandbox.cleanup() + + def test_backend_type( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + assert subprocess_sandbox.get_backend_type() == "subprocess" From 158578be0f7d6688b78b77c747c217ba4e53df53 Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 7 Mar 2026 12:53:36 +0100 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20harden=20subprocess=20sandbox=20?= =?UTF-8?q?=E2=80=94=20security,=20correctness,=20and=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-reviewed by 10 agents, 21 findings addressed. Security: - Block library injection env vars (LD_PRELOAD, PYTHONPATH, etc.) in denylist - PATH fallback uses safe dirs instead of unfiltered original PATH - Remove http:// from allowed clone schemes (cleartext credential risk) - Add secret-substring filtering in direct git subprocess path - Case-insensitive env var matching on Windows Correctness: - Fix timeout=0.0 silently using default (use `is not None` check) - Fix returncode=0 mapped to -1 (explicit None check) - Add process-group kill on Unix to prevent orphaned grandchildren - Use `raise from exc` instead of `from None` for proper chaining Quality: - Extract shared _init_repo fixture, remove test code duplication - Add sandbox tests for GitDiffTool, GitBranchTool, GitCommitTool - Add denylist test for library injection entries - Use MappingProxyType for _GIT_HARDENING_OVERRIDES - Update DESIGN_SPEC.md: fix sandbox filenames, add missing files, update implementation status, add sandbox.py to events listing Co-Authored-By: Claude Opus 4.6 --- DESIGN_SPEC.md | 18 ++- .../observability/events/sandbox.py | 1 + src/ai_company/tools/_git_base.py | 96 ++++++++++--- src/ai_company/tools/git_tools.py | 1 - src/ai_company/tools/sandbox/config.py | 14 +- src/ai_company/tools/sandbox/errors.py | 9 +- src/ai_company/tools/sandbox/protocol.py | 29 +++- .../tools/sandbox/subprocess_sandbox.py | 85 +++++++++--- tests/integration/tools/conftest.py | 41 ++++++ .../tools/test_sandbox_integration.py | 59 +------- .../tools/git/test_git_sandbox_integration.py | 127 ++++++++---------- tests/unit/tools/sandbox/test_config.py | 14 ++ .../tools/sandbox/test_subprocess_sandbox.py | 1 + 13 files changed, 317 insertions(+), 178 deletions(-) create mode 100644 tests/integration/tools/conftest.py diff --git a/DESIGN_SPEC.md b/DESIGN_SPEC.md index 8b9e80110e..dd2ffda846 100644 --- a/DESIGN_SPEC.md +++ b/DESIGN_SPEC.md @@ -1687,8 +1687,8 @@ Tool execution requires safety boundaries proportional to the risk of each tool | Backend | Isolation | Latency | Dependencies | Status | |---------|-----------|---------|--------------|--------| -| `SubprocessSandbox` | Process-level: timeout, restricted PATH, workspace-scoped paths | ~ms | None | M3 | -| `DockerSandbox` | Container-level: ephemeral container, mounted workspace, no network, resource limits (CPU/memory/time) | ~1-2s cold start | Docker | M3 | +| `SubprocessSandbox` | Process-level: env filtering (allowlist + denylist), restricted PATH, workspace-scoped cwd, timeout + process-group kill, library injection var blocking | ~ms | None | **Implemented** | +| `DockerSandbox` | Container-level: ephemeral container, mounted workspace, no network, resource limits (CPU/memory/time) | ~1-2s cold start | Docker | Planned | | `K8sSandbox` | Pod-level: per-agent containers, namespace isolation, resource quotas, network policies | ~2-5s | Kubernetes | Future | #### Default Layered Configuration @@ -2335,6 +2335,7 @@ ai-company/ │ │ │ ├── provider.py # PROVIDER_* constants │ │ │ ├── role.py # ROLE_* constants │ │ │ ├── routing.py # ROUTING_* constants +│ │ │ ├── sandbox.py # SANDBOX_* constants │ │ │ ├── task.py # TASK_* constants │ │ │ ├── template.py # TEMPLATE_* constants │ │ │ └── tool.py # TOOL_* constants @@ -2372,10 +2373,13 @@ ai-company/ │ │ ├── errors.py # Tool error hierarchy (incl. ToolPermissionDeniedError) │ │ ├── examples/ # Example tool implementations │ │ │ └── echo.py # Echo tool (for testing) -│ │ ├── sandbox/ # Sandboxing backends (M3) +│ │ ├── sandbox/ # Sandboxing backends +│ │ │ ├── __init__.py # Package exports +│ │ │ ├── config.py # SubprocessSandboxConfig model +│ │ │ ├── errors.py # SandboxError hierarchy │ │ │ ├── protocol.py # SandboxBackend protocol -│ │ │ ├── subprocess.py # SubprocessSandbox (default for low-risk) -│ │ │ └── docker.py # DockerSandbox (for code_runner, terminal) +│ │ │ ├── result.py # SandboxResult model +│ │ │ └── subprocess_sandbox.py # SubprocessSandbox (default) │ │ ├── file_system/ # Built-in file system tools │ │ │ ├── __init__.py # Package exports │ │ │ ├── _base_fs_tool.py # BaseFileSystemTool ABC @@ -2385,8 +2389,8 @@ ai-company/ │ │ │ ├── list_directory.py # ListDirectoryTool │ │ │ ├── read_file.py # ReadFileTool │ │ │ └── write_file.py # WriteFileTool -│ │ ├── _git_base.py # Base class for git tools (workspace, subprocess) -│ │ ├── git_tools.py # Git operations — 6 built-in tools +│ │ ├── _git_base.py # Base class for git tools (workspace, subprocess, sandbox integration) +│ │ ├── git_tools.py # Git operations — 6 built-in tools (sandbox-aware) │ │ ├── code_runner.py # Code execution (M3) │ │ ├── web_tools.py # HTTP, search (M3) │ │ └── mcp_bridge.py # MCP server integration (M7) diff --git a/src/ai_company/observability/events/sandbox.py b/src/ai_company/observability/events/sandbox.py index bfd51a9379..8f09ab9a1c 100644 --- a/src/ai_company/observability/events/sandbox.py +++ b/src/ai_company/observability/events/sandbox.py @@ -10,4 +10,5 @@ SANDBOX_ENV_FILTERED: Final[str] = "sandbox.env.filtered" SANDBOX_WORKSPACE_VIOLATION: Final[str] = "sandbox.workspace.violation" SANDBOX_CLEANUP: Final[str] = "sandbox.cleanup" +SANDBOX_PATH_FALLBACK: Final[str] = "sandbox.path.fallback" SANDBOX_HEALTH_CHECK: Final[str] = "sandbox.health_check" diff --git a/src/ai_company/tools/_git_base.py b/src/ai_company/tools/_git_base.py index 96065d1d62..6bccf398c2 100644 --- a/src/ai_company/tools/_git_base.py +++ b/src/ai_company/tools/_git_base.py @@ -10,9 +10,12 @@ interactive prompts and restrict config/protocol attack surfaces. When a ``SandboxBackend`` is injected, subprocess management is -delegated to the sandbox, which provides environment filtering, -workspace enforcement, and timeout support on top of the git -hardening env vars. +delegated to the sandbox — the sandbox handles environment +filtering and workspace boundary enforcement for the ``cwd``, +while ``_BaseGitTool._validate_path`` independently enforces +workspace boundaries for git path arguments. Git hardening +env vars are passed as ``env_overrides`` to the sandbox. +Without a sandbox, the direct-subprocess path is used. """ import asyncio @@ -20,6 +23,7 @@ import re from abc import ABC from pathlib import Path # noqa: TC003 — used at runtime +from types import MappingProxyType from typing import TYPE_CHECKING, Any, Final from ai_company.core.enums import ToolCategory @@ -43,14 +47,27 @@ _DEFAULT_TIMEOUT: Final[float] = 30.0 +# Matches http(s)://user:pass@host patterns in git URLs. _CREDENTIAL_RE = re.compile(r"(https?://)[^@/]+@") -_GIT_HARDENING_OVERRIDES: Final[dict[str, str]] = { - "GIT_TERMINAL_PROMPT": "0", - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_CONFIG_GLOBAL": os.devnull, - "GIT_PROTOCOL_FROM_USER": "0", -} +_GIT_HARDENING_OVERRIDES: Final[MappingProxyType[str, str]] = MappingProxyType( + { + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_PROTOCOL_FROM_USER": "0", + } +) + +# Substrings that indicate secret env vars (defense-in-depth for direct path). +_SECRET_SUBSTRINGS: Final[tuple[str, ...]] = ( + "KEY", + "SECRET", + "TOKEN", + "PASSWORD", + "CREDENTIAL", + "PRIVATE", +) def _sanitize_command(args: list[str]) -> list[str]: @@ -126,7 +143,7 @@ def _validate_path(self, relative: str) -> Path: """ try: resolved = (self._workspace / relative).resolve() - except OSError: + except OSError as exc: logger.warning( GIT_WORKSPACE_VIOLATION, path=relative, @@ -134,17 +151,17 @@ def _validate_path(self, relative: str) -> Path: error="path resolution failed", ) msg = f"Path '{relative}' could not be resolved" - raise ValueError(msg) from None + raise ValueError(msg) from exc try: resolved.relative_to(self._workspace) - except ValueError: + except ValueError as exc: logger.warning( GIT_WORKSPACE_VIOLATION, path=relative, workspace=str(self._workspace), ) msg = f"Path '{relative}' is outside workspace" - raise ValueError(msg) from None + raise ValueError(msg) from exc return resolved def _check_paths(self, paths: list[str]) -> ToolExecutionResult | None: @@ -197,8 +214,18 @@ def _check_ref( @staticmethod def _build_git_env() -> dict[str, str]: - """Build a hardened environment for git subprocesses.""" - return {**os.environ, **_GIT_HARDENING_OVERRIDES} + """Build a hardened environment for git subprocesses. + + Applies git hardening overrides and strips obvious secret + env vars as defense-in-depth. For full environment filtering, + use a ``SandboxBackend``. + """ + env = {**os.environ, **_GIT_HARDENING_OVERRIDES} + for key in list(env): + upper = key.upper() + if any(sub in upper for sub in _SECRET_SUBSTRINGS): + del env[key] + return env @staticmethod def _build_git_env_overrides() -> dict[str, str]: @@ -216,7 +243,17 @@ async def _start_git_process( work_dir: Path, env: dict[str, str], ) -> asyncio.subprocess.Process | ToolExecutionResult: - """Start the git subprocess, returning an error on failure.""" + """Start the git subprocess, returning an error on failure. + + Args: + args: Git command arguments. + work_dir: Working directory for the subprocess. + env: Environment variables for the subprocess. + + Returns: + The started ``Process`` on success, or a + ``ToolExecutionResult`` with ``is_error=True`` on failure. + """ try: return await asyncio.create_subprocess_exec( "git", @@ -226,15 +263,15 @@ async def _start_git_process( stderr=asyncio.subprocess.PIPE, env=env, ) - except OSError: + except OSError as exc: logger.warning( GIT_COMMAND_FAILED, command=_sanitize_command(["git", *args]), - error="subprocess start failed", + error=f"subprocess start failed: {exc}", exc_info=True, ) return ToolExecutionResult( - content="Failed to start git process", + content=f"Failed to start git process: {exc}", is_error=True, ) @@ -245,7 +282,11 @@ async def _await_git_process( *, deadline: float, ) -> tuple[bytes, bytes] | ToolExecutionResult: - """Wait for the process with a timeout, returning output or error.""" + """Wait for the process with a timeout, returning output or error. + + On timeout, kills the process and waits up to 5 seconds for + termination before returning an error result. + """ try: return await asyncio.wait_for( proc.communicate(), @@ -385,7 +426,9 @@ async def _run_git_sandboxed( deadline: float, ) -> ToolExecutionResult: """Execute git through the sandbox backend.""" - assert self._sandbox is not None # noqa: S101 + if self._sandbox is None: # pragma: no cover — guarded by caller + msg = "_run_git_sandboxed called without sandbox" + raise RuntimeError(msg) try: result = await self._sandbox.execute( @@ -405,6 +448,17 @@ async def _run_git_sandboxed( content=str(exc), is_error=True, ) + except Exception as exc: + logger.error( + GIT_COMMAND_FAILED, + command=_sanitize_command(["git", *args]), + error=f"Unexpected sandbox error: {exc}", + exc_info=True, + ) + return ToolExecutionResult( + content=f"Sandbox error: {exc}", + is_error=True, + ) return self._sandbox_result_to_execution_result(args, result) async def _run_git_direct( diff --git a/src/ai_company/tools/git_tools.py b/src/ai_company/tools/git_tools.py index 55705b444a..cd680c3dbf 100644 --- a/src/ai_company/tools/git_tools.py +++ b/src/ai_company/tools/git_tools.py @@ -26,7 +26,6 @@ _CLONE_TIMEOUT: Final[float] = 120.0 _ALLOWED_CLONE_SCHEMES: Final[tuple[str, ...]] = ( "https://", - "http://", "ssh://", "git://", ) diff --git a/src/ai_company/tools/sandbox/config.py b/src/ai_company/tools/sandbox/config.py index 8798c37d68..c592be3c9c 100644 --- a/src/ai_company/tools/sandbox/config.py +++ b/src/ai_company/tools/sandbox/config.py @@ -8,12 +8,15 @@ class SubprocessSandboxConfig(BaseModel): Attributes: timeout_seconds: Default command timeout in seconds. - workspace_only: Enforce cwd within the workspace boundary. - restricted_path: Filter PATH entries to safe directories. + workspace_only: When enabled, rejects commands whose working + directory falls outside the workspace boundary. + restricted_path: When enabled, filters PATH entries to retain + only known safe system directories. env_allowlist: Environment variable names allowed to pass through. Supports ``LC_*`` as a glob for all locale variables. env_denylist_patterns: fnmatch patterns to strip even if in the allowlist (e.g. ``*KEY*`` catches ``API_KEY``). + Includes secret-name heuristics and library injection vars. """ model_config = ConfigDict(frozen=True) @@ -47,4 +50,11 @@ class SubprocessSandboxConfig(BaseModel): "*PASSWORD*", "*CREDENTIAL*", "*PRIVATE*", + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "DYLD_INSERT_LIBRARIES", + "PYTHONPATH", + "NODE_PATH", + "RUBYLIB", + "PERL5LIB", ) diff --git a/src/ai_company/tools/sandbox/errors.py b/src/ai_company/tools/sandbox/errors.py index e71d76469e..e7162719ba 100644 --- a/src/ai_company/tools/sandbox/errors.py +++ b/src/ai_company/tools/sandbox/errors.py @@ -12,7 +12,14 @@ class SandboxError(ToolError): class SandboxTimeoutError(SandboxError): - """Execution was killed because it exceeded the timeout.""" + """Execution was killed because it exceeded the timeout. + + Note: ``SubprocessSandbox`` signals timeouts via + ``SandboxResult.timed_out`` rather than raising this exception, + so callers can access partial output. This class exists for + future sandbox backends (e.g. Docker) that may raise on timeout + instead of returning a result. + """ class SandboxStartError(SandboxError): diff --git a/src/ai_company/tools/sandbox/protocol.py b/src/ai_company/tools/sandbox/protocol.py index 6230c6020f..0765c97cd0 100644 --- a/src/ai_company/tools/sandbox/protocol.py +++ b/src/ai_company/tools/sandbox/protocol.py @@ -1,6 +1,8 @@ """Sandbox backend protocol definition.""" -from pathlib import Path # noqa: TC003 — used at runtime in Protocol +from pathlib import ( + Path, # noqa: TC003 — needed at runtime for @runtime_checkable Protocol +) from typing import TYPE_CHECKING, Protocol, runtime_checkable if TYPE_CHECKING: @@ -15,7 +17,8 @@ class SandboxBackend(Protocol): Implementations execute commands in an isolated environment with environment filtering, workspace enforcement, and timeout support. - Subprocess is the M3 backend; Docker/K8s are future work. + Subprocess is the initial backend; Docker/K8s are planned for + future milestones. """ async def execute( @@ -39,17 +42,33 @@ async def execute( Returns: A ``SandboxResult`` with captured output and exit status. + + Raises: + SandboxStartError: If the subprocess could not be started. + SandboxError: If cwd is outside the workspace boundary. """ ... async def cleanup(self) -> None: - """Release any resources held by the backend.""" + """Release any resources held by the backend. + + Returns: + Nothing. + """ ... async def health_check(self) -> bool: - """Return ``True`` if the backend is operational.""" + """Return ``True`` if the backend is operational. + + Returns: + ``True`` if healthy, ``False`` otherwise. + """ ... def get_backend_type(self) -> str: - """Return a short identifier for this backend type.""" + """Return a short identifier for this backend type. + + Returns: + A string like ``'subprocess'`` or ``'docker'``. + """ ... diff --git a/src/ai_company/tools/sandbox/subprocess_sandbox.py b/src/ai_company/tools/sandbox/subprocess_sandbox.py index 45bdb6254e..33765777fe 100644 --- a/src/ai_company/tools/sandbox/subprocess_sandbox.py +++ b/src/ai_company/tools/sandbox/subprocess_sandbox.py @@ -6,11 +6,11 @@ """ import asyncio -import contextlib import fnmatch import os +import signal from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Final from ai_company.observability import get_logger from ai_company.observability.events.sandbox import ( @@ -21,6 +21,7 @@ SANDBOX_EXECUTE_SUCCESS, SANDBOX_EXECUTE_TIMEOUT, SANDBOX_HEALTH_CHECK, + SANDBOX_PATH_FALLBACK, SANDBOX_SPAWN_FAILED, SANDBOX_WORKSPACE_VIOLATION, ) @@ -40,6 +41,9 @@ _DEFAULT_CONFIG = SubprocessSandboxConfig() +# Unix process-group support for killing child process trees. +_HAS_PROCESS_GROUPS: Final[bool] = hasattr(os, "killpg") + class SubprocessSandbox: """Subprocess sandbox backend. @@ -88,11 +92,17 @@ def workspace(self) -> Path: return self._workspace def _matches_allowlist(self, name: str) -> bool: - """Check if an env var name matches any entry in the allowlist.""" + """Check if an env var name matches any entry in the allowlist. + + Uses case-insensitive matching on Windows where env var names + are case-insensitive. + """ + check_name = name.upper() if os.name == "nt" else name for pattern in self._config.env_allowlist: - if pattern == name: + check_pattern = pattern.upper() if os.name == "nt" else pattern + if check_pattern == check_name: return True - if fnmatch.fnmatch(name, pattern): + if fnmatch.fnmatch(check_name, check_pattern): return True return False @@ -104,7 +114,12 @@ def _matches_denylist(self, name: str) -> bool: ) def _filter_path(self, path_value: str) -> str: - """Filter PATH entries, keeping only safe system directories.""" + """Filter PATH entries, keeping only safe system directories. + + When no entries survive filtering, falls back to known safe + directories that actually exist on the system rather than + returning the unfiltered original PATH. + """ safe_prefixes = self._get_safe_path_prefixes() entries = path_value.split(_PATH_SEP) filtered = [ @@ -112,7 +127,15 @@ def _filter_path(self, path_value: str) -> str: for e in entries if any(e.lower().startswith(prefix.lower()) for prefix in safe_prefixes) ] - return _PATH_SEP.join(filtered) if filtered else path_value + if filtered: + return _PATH_SEP.join(filtered) + logger.warning( + SANDBOX_PATH_FALLBACK, + reason="no PATH entries matched safe prefixes; using safe defaults", + original_entry_count=len(entries), + ) + safe_dirs = [p for p in safe_prefixes if Path(p).is_dir()] + return _PATH_SEP.join(safe_dirs) @staticmethod def _get_safe_path_prefixes() -> tuple[str, ...]: @@ -137,8 +160,13 @@ def _build_filtered_env( process environment, strips denylist matches, optionally filters PATH, then applies overrides. + Note: ``env_overrides`` bypass the denylist by design — they + are trusted internal overrides (e.g. git hardening vars). + Callers must not pass untrusted user-controlled data as + overrides. + Args: - env_overrides: Extra vars applied on top (always win). + env_overrides: Trusted internal vars applied on top. Returns: The filtered environment mapping. @@ -181,14 +209,31 @@ def _validate_cwd(self, cwd: Path) -> None: return try: cwd.resolve().relative_to(self._workspace) - except ValueError: + except ValueError as exc: logger.warning( SANDBOX_WORKSPACE_VIOLATION, cwd=str(cwd), workspace=str(self._workspace), ) msg = f"Working directory '{cwd}' is outside workspace '{self._workspace}'" - raise SandboxError(msg) from None + raise SandboxError(msg) from exc + + @staticmethod + def _kill_process(proc: asyncio.subprocess.Process) -> None: + """Kill the process, targeting the process group on Unix. + + On Unix with ``start_new_session=True``, kills the entire + process group to prevent orphaned grandchild processes. + Falls back to direct ``proc.kill()`` on Windows or on error. + """ + if _HAS_PROCESS_GROUPS: + try: + os.killpg(os.getpgid(proc.pid), signal.SIGKILL) # type: ignore[attr-defined] + except OSError: + proc.kill() + else: + return + proc.kill() async def execute( self, @@ -215,10 +260,12 @@ async def execute( SandboxStartError: If the subprocess could not be started. SandboxError: If cwd is outside the workspace boundary. """ - work_dir = cwd or self._workspace + work_dir = cwd if cwd is not None else self._workspace self._validate_cwd(work_dir) - effective_timeout = timeout or self._config.timeout_seconds + effective_timeout = ( + timeout if timeout is not None else self._config.timeout_seconds + ) env = self._build_filtered_env(env_overrides) logger.debug( @@ -237,6 +284,7 @@ async def execute( stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, env=env, + start_new_session=_HAS_PROCESS_GROUPS, ) except OSError as exc: logger.warning( @@ -257,9 +305,16 @@ async def execute( timeout=effective_timeout, ) except TimeoutError: - proc.kill() - with contextlib.suppress(TimeoutError): + self._kill_process(proc) + try: await asyncio.wait_for(proc.communicate(), timeout=5.0) + except TimeoutError: + logger.warning( + SANDBOX_EXECUTE_TIMEOUT, + command=command, + args=args, + error="process did not terminate after kill", + ) logger.warning( SANDBOX_EXECUTE_TIMEOUT, command=command, @@ -275,7 +330,7 @@ async def execute( stdout = stdout_bytes.decode("utf-8", errors="replace").strip() stderr = stderr_bytes.decode("utf-8", errors="replace").strip() - returncode = proc.returncode or 0 + returncode = proc.returncode if proc.returncode is not None else -1 if returncode != 0: logger.warning( diff --git a/tests/integration/tools/conftest.py b/tests/integration/tools/conftest.py new file mode 100644 index 0000000000..bbddee04be --- /dev/null +++ b/tests/integration/tools/conftest.py @@ -0,0 +1,41 @@ +"""Fixtures for integration tool tests.""" + +import os +import subprocess +from pathlib import Path # noqa: TC003 — pytest evaluates annotations + +import pytest + +_GIT_ENV = { + **os.environ, + "GIT_AUTHOR_NAME": "Test", + "GIT_AUTHOR_EMAIL": "test@test.local", + "GIT_COMMITTER_NAME": "Test", + "GIT_COMMITTER_EMAIL": "test@test.local", + "GIT_TERMINAL_PROMPT": "0", + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_PROTOCOL_FROM_USER": "0", +} + + +def _run_git(args: list[str], cwd: Path) -> None: + """Run a git command synchronously.""" + subprocess.run( # noqa: S603 + ["git", *args], # noqa: S607 + cwd=cwd, + check=True, + capture_output=True, + env=_GIT_ENV, + ) + + +@pytest.fixture +def git_repo(tmp_path: Path) -> Path: + """Initialized git repo with one commit.""" + _run_git(["init"], tmp_path) + _run_git(["config", "user.name", "Test"], tmp_path) + _run_git(["config", "user.email", "test@test.local"], tmp_path) + (tmp_path / "README.md").write_text("# Test\n") + _run_git(["add", "."], tmp_path) + _run_git(["commit", "-m", "initial commit"], tmp_path) + return tmp_path diff --git a/tests/integration/tools/test_sandbox_integration.py b/tests/integration/tools/test_sandbox_integration.py index 447e89692b..fdc411a070 100644 --- a/tests/integration/tools/test_sandbox_integration.py +++ b/tests/integration/tools/test_sandbox_integration.py @@ -1,7 +1,6 @@ """Integration tests for subprocess sandbox with real git.""" import os -import subprocess from pathlib import Path # noqa: TC003 — used at runtime import pytest @@ -13,67 +12,23 @@ pytestmark = [pytest.mark.integration, pytest.mark.timeout(30)] -_GIT_ENV = { - **os.environ, - "GIT_AUTHOR_NAME": "Test", - "GIT_AUTHOR_EMAIL": "test@test.local", - "GIT_COMMITTER_NAME": "Test", - "GIT_COMMITTER_EMAIL": "test@test.local", - "GIT_TERMINAL_PROMPT": "0", - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_PROTOCOL_FROM_USER": "0", -} - - -def _init_repo(path: Path) -> None: - """Initialize a git repo with one commit.""" - for args in ( - ["init"], - ["config", "user.name", "Test"], - ["config", "user.email", "test@test.local"], - ): - subprocess.run( # noqa: S603 - ["git", *args], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - (path / "README.md").write_text("# Test\n") - subprocess.run( - ["git", "add", "."], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - subprocess.run( - ["git", "commit", "-m", "initial"], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - class TestRealGitWithSandbox: """Real git repo + SubprocessSandbox + GitStatusTool.""" - async def test_git_status_via_sandbox(self, tmp_path: Path) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + async def test_git_status_via_sandbox(self, git_repo: Path) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={}) assert not result.is_error async def test_git_status_porcelain_via_sandbox( self, - tmp_path: Path, + git_repo: Path, ) -> None: - _init_repo(tmp_path) - (tmp_path / "new.txt").write_text("new file") - sandbox = SubprocessSandbox(workspace=tmp_path) - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + (git_repo / "new.txt").write_text("new file") + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={"porcelain": True}) assert not result.is_error assert "new.txt" in result.content diff --git a/tests/unit/tools/git/test_git_sandbox_integration.py b/tests/unit/tools/git/test_git_sandbox_integration.py index ab5c678514..cc97efc418 100644 --- a/tests/unit/tools/git/test_git_sandbox_integration.py +++ b/tests/unit/tools/git/test_git_sandbox_integration.py @@ -1,96 +1,79 @@ """Tests for git tools with sandbox integration.""" -import os -import subprocess from pathlib import Path # noqa: TC003 — used at runtime from unittest.mock import AsyncMock import pytest -from ai_company.tools.git_tools import GitLogTool, GitStatusTool +from ai_company.tools.git_tools import ( + GitBranchTool, + GitCommitTool, + GitDiffTool, + GitLogTool, + GitStatusTool, +) from ai_company.tools.sandbox.errors import SandboxError from ai_company.tools.sandbox.result import SandboxResult from ai_company.tools.sandbox.subprocess_sandbox import SubprocessSandbox pytestmark = [pytest.mark.unit, pytest.mark.timeout(30)] -_GIT_ENV = { - **os.environ, - "GIT_AUTHOR_NAME": "Test", - "GIT_AUTHOR_EMAIL": "test@test.local", - "GIT_COMMITTER_NAME": "Test", - "GIT_COMMITTER_EMAIL": "test@test.local", - "GIT_TERMINAL_PROMPT": "0", - "GIT_CONFIG_NOSYSTEM": "1", - "GIT_PROTOCOL_FROM_USER": "0", -} - - -def _init_repo(path: Path) -> None: - """Initialize a git repo with one commit.""" - for args in ( - ["init"], - ["config", "user.name", "Test"], - ["config", "user.email", "test@test.local"], - ): - subprocess.run( # noqa: S603 - ["git", *args], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - (path / "README.md").write_text("# Test\n") - subprocess.run( - ["git", "add", "."], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - subprocess.run( - ["git", "commit", "-m", "initial"], # noqa: S607 - cwd=path, - check=True, - capture_output=True, - env=_GIT_ENV, - ) - class TestGitToolsWithSandbox: """Git tools work when a sandbox is injected.""" - async def test_status_with_sandbox(self, tmp_path: Path) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + async def test_status_with_sandbox(self, git_repo: Path) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={}) assert not result.is_error - async def test_log_with_sandbox(self, tmp_path: Path) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) - tool = GitLogTool(workspace=tmp_path, sandbox=sandbox) + async def test_log_with_sandbox(self, git_repo: Path) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitLogTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={"max_count": 1}) assert not result.is_error - assert "initial" in result.content + assert "initial" in result.content.lower() + + async def test_diff_with_sandbox(self, git_repo: Path) -> None: + (git_repo / "new.txt").write_text("new content\n") + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitDiffTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert not result.is_error + + async def test_branch_list_with_sandbox(self, git_repo: Path) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitBranchTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute(arguments={"action": "list"}) + assert not result.is_error + + async def test_commit_with_sandbox(self, git_repo: Path) -> None: + (git_repo / "staged.txt").write_text("staged\n") + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitCommitTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute( + arguments={ + "message": "sandbox commit", + "paths": ["staged.txt"], + }, + ) + assert not result.is_error class TestGitToolsWithoutSandbox: """Git tools work without sandbox (backward compat).""" - async def test_status_without_sandbox(self, tmp_path: Path) -> None: - _init_repo(tmp_path) - tool = GitStatusTool(workspace=tmp_path) + async def test_status_without_sandbox(self, git_repo: Path) -> None: + tool = GitStatusTool(workspace=git_repo) result = await tool.execute(arguments={}) assert not result.is_error - async def test_log_without_sandbox(self, tmp_path: Path) -> None: - _init_repo(tmp_path) - tool = GitLogTool(workspace=tmp_path) + async def test_log_without_sandbox(self, git_repo: Path) -> None: + tool = GitLogTool(workspace=git_repo) result = await tool.execute(arguments={"max_count": 1}) assert not result.is_error - assert "initial" in result.content + assert "initial" in result.content.lower() class TestSandboxTimeoutSurfaces: @@ -98,11 +81,9 @@ class TestSandboxTimeoutSurfaces: async def test_sandbox_timeout_returns_error( self, - tmp_path: Path, + git_repo: Path, ) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) - # Mock execute to return a timed-out result + sandbox = SubprocessSandbox(workspace=git_repo) sandbox.execute = AsyncMock( # type: ignore[method-assign] return_value=SandboxResult( stdout="", @@ -111,7 +92,7 @@ async def test_sandbox_timeout_returns_error( timed_out=True, ), ) - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={}) assert result.is_error assert "timed out" in result.content.lower() @@ -122,14 +103,13 @@ class TestSandboxErrorSurfaces: async def test_sandbox_error_returns_error( self, - tmp_path: Path, + git_repo: Path, ) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) + sandbox = SubprocessSandbox(workspace=git_repo) sandbox.execute = AsyncMock( # type: ignore[method-assign] side_effect=SandboxError("workspace violation"), ) - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) result = await tool.execute(arguments={}) assert result.is_error assert "workspace violation" in result.content @@ -140,10 +120,9 @@ class TestGitHardeningWithSandbox: async def test_env_overrides_contain_hardening_vars( self, - tmp_path: Path, + git_repo: Path, ) -> None: - _init_repo(tmp_path) - sandbox = SubprocessSandbox(workspace=tmp_path) + sandbox = SubprocessSandbox(workspace=git_repo) original_execute = sandbox.execute captured_overrides: dict[str, str] = {} @@ -154,7 +133,7 @@ async def capture_execute(**kwargs: object) -> SandboxResult: return await original_execute(**kwargs) # type: ignore[arg-type] sandbox.execute = capture_execute # type: ignore[method-assign] - tool = GitStatusTool(workspace=tmp_path, sandbox=sandbox) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) await tool.execute(arguments={}) assert captured_overrides.get("GIT_TERMINAL_PROMPT") == "0" diff --git a/tests/unit/tools/sandbox/test_config.py b/tests/unit/tools/sandbox/test_config.py index 7d1232d973..f164771051 100644 --- a/tests/unit/tools/sandbox/test_config.py +++ b/tests/unit/tools/sandbox/test_config.py @@ -58,3 +58,17 @@ def test_denylist_patterns_cover_secrets(self) -> None: assert any("KEY" in p for p in patterns) assert any("SECRET" in p for p in patterns) assert any("TOKEN" in p for p in patterns) + + def test_denylist_patterns_cover_library_injection(self) -> None: + config = SubprocessSandboxConfig() + patterns = config.env_denylist_patterns + for var in ( + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "DYLD_INSERT_LIBRARIES", + "PYTHONPATH", + "NODE_PATH", + "RUBYLIB", + "PERL5LIB", + ): + assert var in patterns, f"{var} missing from denylist" diff --git a/tests/unit/tools/sandbox/test_subprocess_sandbox.py b/tests/unit/tools/sandbox/test_subprocess_sandbox.py index e318f4dfe6..84f82ac02f 100644 --- a/tests/unit/tools/sandbox/test_subprocess_sandbox.py +++ b/tests/unit/tools/sandbox/test_subprocess_sandbox.py @@ -254,6 +254,7 @@ async def test_timeout_kills_process( assert result.timed_out assert not result.success + @pytest.mark.filterwarnings("ignore::ResourceWarning") async def test_start_failure( self, subprocess_sandbox: SubprocessSandbox, From b7fc1ce1504c7d244e9b1d46112a789f21b7a24e Mon Sep 17 00:00:00 2001 From: Aurelio <19254254+Aureliolo@users.noreply.github.com> Date: Sat, 7 Mar 2026 13:23:17 +0100 Subject: [PATCH 3/3] fix: address 31 PR review items from local agents, CodeRabbit, Gemini, Greptile, and Copilot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Harden PATH filtering with normpath + os.sep boundary check (prefix spoofing fix) - Fix case-insensitive denylist matching (uppercase both name and pattern) - Add credential redaction in logged args via _redact_args helper - Split execute() into _spawn_process, _communicate_with_timeout, _drain_after_kill - Fix _kill_process double-kill + add ProcessLookupError handling - Add SANDBOX_KILL_FAILED event constant for unkillable processes - Remove broad except Exception from _run_git_sandboxed - Add _sandbox_result_to_execution_result deadline parameter - Make _MAX_COUNT_LIMIT Final, update GitCloneTool docstring - Update DESIGN_SPEC.md §11.1.1, §11.2, §15.5 for sandbox status - Update CLAUDE.md package structure with sandbox - Make git test fixtures hermetic (GIT_CONFIG_GLOBAL=os.devnull) - Add tests: env_overrides bypass, prefix spoofing, path fallback, zero timeout - Add parametrized SandboxResult success matrix test - Add sandbox events existence test Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 2 +- DESIGN_SPEC.md | 6 +- .../observability/events/sandbox.py | 1 + src/ai_company/tools/_git_base.py | 56 ++-- src/ai_company/tools/git_tools.py | 13 +- .../tools/sandbox/subprocess_sandbox.py | 258 +++++++++++++----- tests/integration/tools/conftest.py | 3 +- tests/unit/observability/test_events.py | 26 ++ .../tools/git/test_git_sandbox_integration.py | 85 ++++++ tests/unit/tools/sandbox/test_result.py | 48 ++-- .../tools/sandbox/test_subprocess_sandbox.py | 84 ++++++ 11 files changed, 460 insertions(+), 122 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 2bbf5f0d98..68a8fd531a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -54,7 +54,7 @@ src/ai_company/ providers/ # LLM provider abstraction (LiteLLM adapter) security/ # SecOps agent, approval gates, audit templates/ # Pre-built company templates and builder - tools/ # Tool registry, built-in tools (file_system/, git), MCP integration, role-based access, sandboxing + tools/ # Tool registry, built-in tools (file_system/, git, sandbox/), MCP integration, role-based access ``` ## Shell Usage diff --git a/DESIGN_SPEC.md b/DESIGN_SPEC.md index dd2ffda846..c008726929 100644 --- a/DESIGN_SPEC.md +++ b/DESIGN_SPEC.md @@ -1675,7 +1675,7 @@ When the LLM requests multiple tool calls in a single turn, `ToolInvoker.invoke_ The `ToolPermissionChecker` resolves permissions using a priority-based system: denied list (highest) → allowed list → access-level categories → deny (default). `AgentEngine._make_tool_invoker()` creates a permission-aware invoker from the agent's `ToolPermissions` at the start of each `run()` call. Note: M3 implements category-level gating only; the granular sub-constraints described in §11.2 (workspace scope, network mode) are planned for when sandboxing is implemented. -> **M3 implementation note — Built-in git tools:** Six workspace-scoped git tools are implemented in `tools/git_tools.py` with a shared `_BaseGitTool` base class in `tools/_git_base.py`: `GitStatusTool`, `GitLogTool`, `GitDiffTool`, `GitBranchTool`, `GitCommitTool`, and `GitCloneTool`. The base class enforces workspace boundary security (path traversal prevention via `resolve()` + `relative_to()`) and provides a common `_run_git()` helper using `asyncio.create_subprocess_exec` (never `shell=True`). Security hardening includes: `GIT_TERMINAL_PROMPT=0` to prevent credential prompts, `GIT_CONFIG_NOSYSTEM=1`, `GIT_CONFIG_GLOBAL=/dev/null`, and `GIT_PROTOCOL_FROM_USER=0` to restrict config/protocol attack surfaces, rejection of flag-like ref/branch values (starting with `-`), URL scheme validation on clone (only `https://`, `http://`, `ssh://`, `git://`, and SCP-like syntax) with `--` separator before positional URL argument, and clone URLs starting with `-` are rejected. All tools return `ToolExecutionResult` for errors rather than raising exceptions. **Future:** Consider adding host/IP allowlisting for clone URLs to prevent SSRF against internal networks (loopback, link-local, private ranges). +> **M3 implementation note — Built-in git tools:** Six workspace-scoped git tools are implemented in `tools/git_tools.py` with a shared `_BaseGitTool` base class in `tools/_git_base.py`: `GitStatusTool`, `GitLogTool`, `GitDiffTool`, `GitBranchTool`, `GitCommitTool`, and `GitCloneTool`. The base class enforces workspace boundary security (path traversal prevention via `resolve()` + `relative_to()`) and provides a common `_run_git()` helper using `asyncio.create_subprocess_exec` (never `shell=True`). Security hardening includes: `GIT_TERMINAL_PROMPT=0` to prevent credential prompts, `GIT_CONFIG_NOSYSTEM=1`, `GIT_CONFIG_GLOBAL=os.devnull`, and `GIT_PROTOCOL_FROM_USER=0` to restrict config/protocol attack surfaces, rejection of flag-like ref/branch values (starting with `-`), URL scheme validation on clone (only `https://`, `ssh://`, `git://`, and SCP-like syntax — plain `http://` rejected for security) with `--` separator before positional URL argument, and clone URLs starting with `-` are rejected. All tools return `ToolExecutionResult` for errors rather than raising exceptions. When a `SandboxBackend` is injected, `_run_git()` delegates subprocess management to the sandbox via `_run_git_sandboxed()` — the sandbox handles environment filtering and workspace-scoped cwd enforcement, while `_validate_path` independently enforces workspace boundaries for git path arguments. Git hardening env vars are passed as `env_overrides` to the sandbox, and `SandboxResult` is converted to `ToolExecutionResult` via `_sandbox_result_to_execution_result`. Without a sandbox, the direct-subprocess path is used (backward compatible). **Future:** Consider adding host/IP allowlisting for clone URLs to prevent SSRF against internal networks (loopback, link-local, private ranges). ### 11.1.2 Tool Sandboxing @@ -1775,7 +1775,7 @@ tool_access: description: "Per-agent custom configuration." ``` -> **M3 implementation note:** The current `ToolPermissionChecker` implements **category-level gating only** — each access level maps to a set of permitted `ToolCategory` values (e.g., `STANDARD` permits `file_system`, `code_execution`, `version_control`, `web`, `terminal`, `analytics`). The granular sub-constraints shown above (workspace scope, network mode, containerization) are planned for when sandboxing backends (§11.1.2) are implemented. +> **M3 implementation note:** The current `ToolPermissionChecker` implements **category-level gating only** — each access level maps to a set of permitted `ToolCategory` values (e.g., `STANDARD` permits `file_system`, `code_execution`, `version_control`, `web`, `terminal`, `analytics`). `SubprocessSandbox` provides workspace-scoped cwd enforcement and env filtering (see §11.1.2). The granular sub-constraints shown above (network mode, containerization) are planned for Docker/K8s sandbox backends. ### 11.3 Progressive Trust @@ -2472,7 +2472,7 @@ These conventions were established during the M0–M2+ review cycle. **Adopted** | **Event constants** | Adopted (per-domain) | Per-domain submodules under `events/` package (e.g. `events.provider`, `events.budget`). Import directly: `from ai_company.observability.events. import CONSTANT` | Split by domain for discoverability, co-location with domain logic, and reduced merge conflicts as constants grow. `__init__.py` serves as package marker with usage documentation; no re-exports. | | **Parallel tool execution** | Adopted (M2.5) | `asyncio.TaskGroup` in `ToolInvoker.invoke_all` with optional `max_concurrency` semaphore | Structured concurrency with proper cancellation semantics. Fatal errors collected via guarded wrapper and re-raised after all tasks complete. | | **Tool permission checking** | Adopted (M3) | `ToolPermissionChecker` enforces category-level gating based on `ToolAccessLevel` (sandboxed → restricted → standard → elevated, plus custom). Priority-based resolution: denied list → allowed list → level categories → deny. Case-insensitive name matching. `ToolInvoker` filters definitions for prompt and checks at invocation time. | Defense-in-depth: agents only see permitted tools in the LLM prompt, and invocations are re-checked at execution time. Explicit allow/deny lists provide per-agent overrides. See §11.1.1. | -| **Tool sandboxing** | Partial (M3) | File system tools use in-process `PathValidator` for workspace-scoped path validation (symlink resolution + containment check). `BaseFileSystemTool` ABC provides shared `ToolCategory.FILE_SYSTEM` and `PathValidator` integration — all file system tools extend this base. `SandboxBackend` protocol with `SubprocessSandbox` / `DockerSandbox` remains planned for git, code_runner, terminal, web, and database tools. `K8sSandbox` planned for future container deployments. | File system tools use defence-in-depth path validation; heavier sandbox isolation reserved for higher-risk tool categories (code execution, network). See §11.1.2. | +| **Tool sandboxing** | Adopted (M3, incremental) | File system tools use in-process `PathValidator` for workspace-scoped path validation (symlink resolution + containment check). `BaseFileSystemTool` ABC provides shared `ToolCategory.FILE_SYSTEM` and `PathValidator` integration — all file system tools extend this base. `SandboxBackend` protocol with `SubprocessSandbox` implemented — git tools accept optional `SandboxBackend` injection and delegate subprocess management to it (env filtering, workspace enforcement, timeout + process-group kill). `DockerSandbox` planned for code_runner, terminal, web, and database tools. `K8sSandbox` planned for future container deployments. Config-driven per-category backend selection planned for engine wiring. | File system tools use defence-in-depth path validation; subprocess sandbox provides lightweight isolation for git tools; heavier Docker/K8s isolation reserved for higher-risk tool categories (code execution, network). See §11.1.2. | | **Crash recovery** | Adopted (M3) | Pluggable `RecoveryStrategy` protocol. M3: `FailAndReassignStrategy` (catch at engine boundary, log snapshot, mark FAILED / eligible for reassignment). M4/M5: `CheckpointStrategy` (persist `AgentContext` per turn, resume from last checkpoint). | Immutable `model_copy` pattern makes checkpoint serialization trivial to add later. Fail-and-reassign is sufficient for short MVP tasks. See §6.6. | | **Agent behavior testing** | Planned (M3) | Scripted `FakeProvider` for unit tests (deterministic turn sequences); behavioral outcome assertions for integration tests (task completed, tools called, cost within budget). | Leverages existing `FakeProvider` and `CompletionResponseFactory` fixtures. Precise engine testing without brittle response-matching at integration level. | | **LLM call analytics** | Planned (incremental) | M3: proxy metrics (`turns_per_task`, `tokens_per_task`). M4: call categorization (`productive`, `coordination`, `system`) + orchestration ratio. M5+: full analytics (retry tracking, latency, cache hits, per-provider comparison). | Append-only, never blocks execution. Builds on existing `CostRecord` infrastructure. Detects orchestration overhead early. See §10.5. | diff --git a/src/ai_company/observability/events/sandbox.py b/src/ai_company/observability/events/sandbox.py index 8f09ab9a1c..8c99fe722e 100644 --- a/src/ai_company/observability/events/sandbox.py +++ b/src/ai_company/observability/events/sandbox.py @@ -12,3 +12,4 @@ SANDBOX_CLEANUP: Final[str] = "sandbox.cleanup" SANDBOX_PATH_FALLBACK: Final[str] = "sandbox.path.fallback" SANDBOX_HEALTH_CHECK: Final[str] = "sandbox.health_check" +SANDBOX_KILL_FAILED: Final[str] = "sandbox.kill.failed" diff --git a/src/ai_company/tools/_git_base.py b/src/ai_company/tools/_git_base.py index 6bccf398c2..e7aaf80e50 100644 --- a/src/ai_company/tools/_git_base.py +++ b/src/ai_company/tools/_git_base.py @@ -5,8 +5,8 @@ boundary, and rejecting flag-injection attempts. Subprocess execution uses ``asyncio.create_subprocess_exec`` (never ``shell=True``) with ``GIT_TERMINAL_PROMPT=0``, -``GIT_CONFIG_NOSYSTEM=1``, ``GIT_CONFIG_GLOBAL`` pointed to -``/dev/null``, and ``GIT_PROTOCOL_FROM_USER=0`` to prevent +``GIT_CONFIG_NOSYSTEM=1``, ``GIT_CONFIG_GLOBAL`` pointed to the platform null device +(``os.devnull``), and ``GIT_PROTOCOL_FROM_USER=0`` to prevent interactive prompts and restrict config/protocol attack surfaces. When a ``SandboxBackend`` is injected, subprocess management is @@ -19,6 +19,7 @@ """ import asyncio +import contextlib import os import re from abc import ABC @@ -47,7 +48,7 @@ _DEFAULT_TIMEOUT: Final[float] = 30.0 -# Matches http(s)://user:pass@host patterns in git URLs. +# Matches http(s)://userinfo@host patterns in git URLs. _CREDENTIAL_RE = re.compile(r"(https?://)[^@/]+@") _GIT_HARDENING_OVERRIDES: Final[MappingProxyType[str, str]] = MappingProxyType( @@ -286,6 +287,15 @@ async def _await_git_process( On timeout, kills the process and waits up to 5 seconds for termination before returning an error result. + + Args: + proc: The running subprocess. + args: Git command arguments (for logging). + deadline: Seconds before the process is killed. + + Returns: + A ``(stdout, stderr)`` tuple on success, or a + ``ToolExecutionResult`` with ``is_error=True`` on timeout. """ try: return await asyncio.wait_for( @@ -293,7 +303,8 @@ async def _await_git_process( timeout=deadline, ) except TimeoutError: - proc.kill() + with contextlib.suppress(ProcessLookupError): + proc.kill() try: await asyncio.wait_for(proc.communicate(), timeout=5.0) except TimeoutError: @@ -319,7 +330,20 @@ def _process_git_output( stdout_bytes: bytes, stderr_bytes: bytes, ) -> ToolExecutionResult: - """Decode output and build the result.""" + """Decode output and build the result. + + Prefers stderr for error content; falls back to stdout, then + a generic "Unknown git error" message. + + Args: + args: Git command arguments (for logging). + returncode: Process exit code (``None`` treated as error). + stdout_bytes: Raw stdout from the process. + stderr_bytes: Raw stderr from the process. + + Returns: + A ``ToolExecutionResult`` with decoded content. + """ stdout = stdout_bytes.decode("utf-8", errors="replace").strip() stderr = stderr_bytes.decode("utf-8", errors="replace").strip() if returncode != 0: @@ -344,6 +368,8 @@ def _process_git_output( def _sandbox_result_to_execution_result( args: list[str], result: SandboxResult, + *, + deadline: float, ) -> ToolExecutionResult: """Convert a ``SandboxResult`` to a ``ToolExecutionResult``. @@ -353,6 +379,7 @@ def _sandbox_result_to_execution_result( Args: args: Git command arguments (for logging). result: The sandbox execution result. + deadline: Timeout that was used (for logging). Returns: A ``ToolExecutionResult`` with the appropriate content. @@ -361,7 +388,7 @@ def _sandbox_result_to_execution_result( logger.warning( GIT_COMMAND_TIMEOUT, command=_sanitize_command(["git", *args]), - deadline="sandbox", + deadline=deadline, ) return ToolExecutionResult( content=result.stderr or "Git command timed out", @@ -448,18 +475,11 @@ async def _run_git_sandboxed( content=str(exc), is_error=True, ) - except Exception as exc: - logger.error( - GIT_COMMAND_FAILED, - command=_sanitize_command(["git", *args]), - error=f"Unexpected sandbox error: {exc}", - exc_info=True, - ) - return ToolExecutionResult( - content=f"Sandbox error: {exc}", - is_error=True, - ) - return self._sandbox_result_to_execution_result(args, result) + return self._sandbox_result_to_execution_result( + args, + result, + deadline=deadline, + ) async def _run_git_direct( self, diff --git a/src/ai_company/tools/git_tools.py b/src/ai_company/tools/git_tools.py index cd680c3dbf..caabcfd9cb 100644 --- a/src/ai_company/tools/git_tools.py +++ b/src/ai_company/tools/git_tools.py @@ -174,7 +174,7 @@ class GitLogTool(_BaseGitTool): date range, ref, and paths. """ - _MAX_COUNT_LIMIT: int = 100 + _MAX_COUNT_LIMIT: Final[int] = 100 def __init__( self, @@ -613,9 +613,9 @@ class GitCloneTool(_BaseGitTool): Validates that the target directory stays within the workspace boundary. Supports optional branch selection and shallow clone - depth. URLs are validated against allowed schemes (https, http, - ssh, git, SCP-like). Local paths and ``file://`` URLs are - rejected. + depth. URLs are validated against allowed schemes (https, ssh, + git, SCP-like). Local paths, ``file://``, and plain ``http://`` + URLs are rejected. """ def __init__( @@ -684,10 +684,11 @@ async def execute( GIT_CLONE_URL_REJECTED, url=url, ) + schemes = ", ".join(_ALLOWED_CLONE_SCHEMES) return ToolExecutionResult( content=( - "Invalid clone URL. Only https://, http://, ssh://, " - "git://, and SCP-like (user@host:path) URLs are " + f"Invalid clone URL. Only {schemes}" + "and SCP-like (user@host:path) URLs are " "allowed" ), is_error=True, diff --git a/src/ai_company/tools/sandbox/subprocess_sandbox.py b/src/ai_company/tools/sandbox/subprocess_sandbox.py index 33765777fe..6e67a2bab2 100644 --- a/src/ai_company/tools/sandbox/subprocess_sandbox.py +++ b/src/ai_company/tools/sandbox/subprocess_sandbox.py @@ -6,8 +6,10 @@ """ import asyncio +import contextlib import fnmatch import os +import re import signal from pathlib import Path from typing import TYPE_CHECKING, Final @@ -21,6 +23,7 @@ SANDBOX_EXECUTE_SUCCESS, SANDBOX_EXECUTE_TIMEOUT, SANDBOX_HEALTH_CHECK, + SANDBOX_KILL_FAILED, SANDBOX_PATH_FALLBACK, SANDBOX_SPAWN_FAILED, SANDBOX_WORKSPACE_VIOLATION, @@ -44,6 +47,14 @@ # Unix process-group support for killing child process trees. _HAS_PROCESS_GROUPS: Final[bool] = hasattr(os, "killpg") +# Matches http(s)://user:pass@host patterns in URLs. +_CREDENTIAL_RE = re.compile(r"(https?://)[^@/]+@") + + +def _redact_args(args: tuple[str, ...]) -> tuple[str, ...]: + """Redact embedded credentials from command args for logging.""" + return tuple(_CREDENTIAL_RE.sub(r"\1***@", a) for a in args) + class SubprocessSandbox: """Subprocess sandbox backend. @@ -72,10 +83,20 @@ def __init__( ValueError: If *workspace* is not absolute or does not exist. """ if not workspace.is_absolute(): + logger.warning( + SANDBOX_WORKSPACE_VIOLATION, + workspace=str(workspace), + error="workspace must be an absolute path", + ) msg = f"workspace must be an absolute path, got: {workspace}" raise ValueError(msg) resolved = workspace.resolve() if not resolved.is_dir(): + logger.warning( + SANDBOX_WORKSPACE_VIOLATION, + workspace=str(resolved), + error="workspace directory does not exist", + ) msg = f"workspace directory does not exist: {resolved}" raise ValueError(msg) self._config = config or _DEFAULT_CONFIG @@ -100,33 +121,36 @@ def _matches_allowlist(self, name: str) -> bool: check_name = name.upper() if os.name == "nt" else name for pattern in self._config.env_allowlist: check_pattern = pattern.upper() if os.name == "nt" else pattern - if check_pattern == check_name: - return True if fnmatch.fnmatch(check_name, check_pattern): return True return False def _matches_denylist(self, name: str) -> bool: - """Check if an env var name matches any denylist pattern.""" + """Check if an env var name matches any denylist pattern. + + Both name and patterns are uppercased for case-insensitive + matching — denylist patterns must catch secrets regardless of + casing. + """ upper = name.upper() return any( - fnmatch.fnmatch(upper, pat) for pat in self._config.env_denylist_patterns + fnmatch.fnmatch(upper, pat.upper()) + for pat in self._config.env_denylist_patterns ) def _filter_path(self, path_value: str) -> str: """Filter PATH entries, keeping only safe system directories. + Uses directory-boundary checking to prevent prefix spoofing + (e.g. ``/usr/bin-malicious`` is rejected even though it starts + with ``/usr/bin``). Entries are normalized before comparison. + When no entries survive filtering, falls back to known safe - directories that actually exist on the system rather than - returning the unfiltered original PATH. + directories that actually exist on the system. """ safe_prefixes = self._get_safe_path_prefixes() entries = path_value.split(_PATH_SEP) - filtered = [ - e - for e in entries - if any(e.lower().startswith(prefix.lower()) for prefix in safe_prefixes) - ] + filtered = [e for e in entries if self._is_safe_path_entry(e, safe_prefixes)] if filtered: return _PATH_SEP.join(filtered) logger.warning( @@ -135,8 +159,35 @@ def _filter_path(self, path_value: str) -> str: original_entry_count=len(entries), ) safe_dirs = [p for p in safe_prefixes if Path(p).is_dir()] + if not safe_dirs: + logger.error( + SANDBOX_PATH_FALLBACK, + reason=( + "no safe PATH directories exist on system — PATH will be empty" + ), + ) return _PATH_SEP.join(safe_dirs) + @staticmethod + def _is_safe_path_entry( + entry: str, + safe_prefixes: tuple[str, ...], + ) -> bool: + """Check if a PATH entry falls within a safe prefix directory. + + Normalizes both the entry and prefix, then checks for exact + match or directory boundary containment (prevents + ``/usr/bin-malicious`` from matching ``/usr/bin``). + """ + entry_norm = os.path.normcase(os.path.normpath(entry)) + for prefix in safe_prefixes: + prefix_norm = os.path.normcase(os.path.normpath(prefix)) + if entry_norm == prefix_norm or entry_norm.startswith( + prefix_norm + os.sep, + ): + return True + return False + @staticmethod def _get_safe_path_prefixes() -> tuple[str, ...]: """Return safe PATH prefixes for the current platform.""" @@ -225,59 +276,42 @@ def _kill_process(proc: asyncio.subprocess.Process) -> None: On Unix with ``start_new_session=True``, kills the entire process group to prevent orphaned grandchild processes. Falls back to direct ``proc.kill()`` on Windows or on error. + Handles ``ProcessLookupError`` when the process already exited. """ if _HAS_PROCESS_GROUPS: try: os.killpg(os.getpgid(proc.pid), signal.SIGKILL) # type: ignore[attr-defined] + except ProcessLookupError: + return except OSError: - proc.kill() + with contextlib.suppress(ProcessLookupError): + proc.kill() + return else: return - proc.kill() + with contextlib.suppress(ProcessLookupError): + proc.kill() - async def execute( + async def _spawn_process( self, - *, command: str, args: tuple[str, ...], - cwd: Path | None = None, - env_overrides: Mapping[str, str] | None = None, - timeout: float | None = None, # noqa: ASYNC109 - ) -> SandboxResult: - """Execute a command in the sandbox. + work_dir: Path, + env: dict[str, str], + ) -> asyncio.subprocess.Process: + """Start the subprocess, raising on failure. Args: command: Executable name or path. args: Command arguments. - cwd: Working directory (defaults to workspace root). - env_overrides: Extra env vars applied on top of filtered env. - timeout: Seconds before the process is killed. - - Returns: - A ``SandboxResult`` with captured output and exit status. + work_dir: Working directory. + env: Filtered environment. Raises: SandboxStartError: If the subprocess could not be started. - SandboxError: If cwd is outside the workspace boundary. """ - work_dir = cwd if cwd is not None else self._workspace - self._validate_cwd(work_dir) - - effective_timeout = ( - timeout if timeout is not None else self._config.timeout_seconds - ) - env = self._build_filtered_env(env_overrides) - - logger.debug( - SANDBOX_EXECUTE_START, - command=command, - args=args, - cwd=str(work_dir), - timeout=effective_timeout, - ) - try: - proc = await asyncio.create_subprocess_exec( + return await asyncio.create_subprocess_exec( command, *args, cwd=work_dir, @@ -299,44 +333,140 @@ async def execute( context={"command": command}, ) from exc + async def _communicate_with_timeout( + self, + proc: asyncio.subprocess.Process, + command: str, + args: tuple[str, ...], + deadline: float, + ) -> tuple[bytes, bytes, bool]: + """Wait for process output with timeout handling. + + On timeout, kills the process and captures any partial output. + + Args: + proc: The running subprocess. + command: Command name (for logging). + args: Command arguments (for logging). + deadline: Seconds before kill. + + Returns: + Tuple of (stdout_bytes, stderr_bytes, timed_out). + """ try: stdout_bytes, stderr_bytes = await asyncio.wait_for( proc.communicate(), - timeout=effective_timeout, + timeout=deadline, ) except TimeoutError: self._kill_process(proc) - try: - await asyncio.wait_for(proc.communicate(), timeout=5.0) - except TimeoutError: - logger.warning( - SANDBOX_EXECUTE_TIMEOUT, - command=command, - args=args, - error="process did not terminate after kill", - ) + stdout_bytes, stderr_bytes = await self._drain_after_kill( + proc, + command, + args, + ) logger.warning( SANDBOX_EXECUTE_TIMEOUT, command=command, - args=args, - timeout=effective_timeout, + args=_redact_args(args), + timeout=deadline, ) - return SandboxResult( - stdout="", - stderr=f"Process timed out after {effective_timeout}s", - returncode=-1, - timed_out=True, + return stdout_bytes, stderr_bytes, True + return stdout_bytes, stderr_bytes, False + + async def _drain_after_kill( + self, + proc: asyncio.subprocess.Process, + command: str, + args: tuple[str, ...], + ) -> tuple[bytes, bytes]: + """Drain remaining output after killing a process. + + Waits up to 5 seconds for the process to terminate. If the + process does not terminate, logs an error and returns empty + output. + """ + try: + return await asyncio.wait_for( + proc.communicate(), + timeout=5.0, + ) + except TimeoutError: + logger.exception( + SANDBOX_KILL_FAILED, + command=command, + args=_redact_args(args), + pid=proc.pid, + error="process did not terminate 5s after kill", ) + return b"", b"" + + async def execute( + self, + *, + command: str, + args: tuple[str, ...], + cwd: Path | None = None, + env_overrides: Mapping[str, str] | None = None, + timeout: float | None = None, # noqa: ASYNC109 + ) -> SandboxResult: + """Execute a command in the sandbox. + + Args: + command: Executable name or path. + args: Command arguments. + cwd: Working directory (defaults to workspace root). + env_overrides: Extra env vars applied on top of filtered env. + timeout: Seconds before the process is killed. + + Returns: + A ``SandboxResult`` with captured output and exit status. + + Raises: + SandboxStartError: If the subprocess could not be started. + SandboxError: If cwd is outside the workspace boundary. + """ + work_dir = cwd if cwd is not None else self._workspace + self._validate_cwd(work_dir) + + effective_timeout = ( + timeout if timeout is not None else self._config.timeout_seconds + ) + env = self._build_filtered_env(env_overrides) + + logger.debug( + SANDBOX_EXECUTE_START, + command=command, + args=_redact_args(args), + cwd=str(work_dir), + timeout=effective_timeout, + ) + + proc = await self._spawn_process(command, args, work_dir, env) + stdout_bytes, stderr_bytes, timed_out = await self._communicate_with_timeout( + proc, + command, + args, + effective_timeout, + ) stdout = stdout_bytes.decode("utf-8", errors="replace").strip() stderr = stderr_bytes.decode("utf-8", errors="replace").strip() returncode = proc.returncode if proc.returncode is not None else -1 + if timed_out: + return SandboxResult( + stdout=stdout, + stderr=(stderr or f"Process timed out after {effective_timeout}s"), + returncode=returncode, + timed_out=True, + ) + if returncode != 0: logger.warning( SANDBOX_EXECUTE_FAILED, command=command, - args=args, + args=_redact_args(args), returncode=returncode, stderr=stderr, ) @@ -344,7 +474,7 @@ async def execute( logger.debug( SANDBOX_EXECUTE_SUCCESS, command=command, - args=args, + args=_redact_args(args), ) return SandboxResult( @@ -354,7 +484,7 @@ async def execute( ) async def cleanup(self) -> None: - """No-op — subprocesses are ephemeral.""" + """Subprocesses are ephemeral — no resources to release.""" logger.debug(SANDBOX_CLEANUP, backend="subprocess") async def health_check(self) -> bool: diff --git a/tests/integration/tools/conftest.py b/tests/integration/tools/conftest.py index bbddee04be..7d33626b60 100644 --- a/tests/integration/tools/conftest.py +++ b/tests/integration/tools/conftest.py @@ -7,11 +7,12 @@ import pytest _GIT_ENV = { - **os.environ, + **{k: v for k, v in os.environ.items() if not k.startswith("GIT_")}, "GIT_AUTHOR_NAME": "Test", "GIT_AUTHOR_EMAIL": "test@test.local", "GIT_COMMITTER_NAME": "Test", "GIT_COMMITTER_EMAIL": "test@test.local", + "GIT_CONFIG_GLOBAL": os.devnull, "GIT_TERMINAL_PROMPT": "0", "GIT_CONFIG_NOSYSTEM": "1", "GIT_PROTOCOL_FROM_USER": "0", diff --git a/tests/unit/observability/test_events.py b/tests/unit/observability/test_events.py index 6550acd97c..3d4e3f5c92 100644 --- a/tests/unit/observability/test_events.py +++ b/tests/unit/observability/test_events.py @@ -30,6 +30,19 @@ ) from ai_company.observability.events.role import ROLE_LOOKUP_MISS from ai_company.observability.events.routing import ROUTING_DECISION_MADE +from ai_company.observability.events.sandbox import ( + SANDBOX_CLEANUP, + SANDBOX_ENV_FILTERED, + SANDBOX_EXECUTE_FAILED, + SANDBOX_EXECUTE_START, + SANDBOX_EXECUTE_SUCCESS, + SANDBOX_EXECUTE_TIMEOUT, + SANDBOX_HEALTH_CHECK, + SANDBOX_KILL_FAILED, + SANDBOX_PATH_FALLBACK, + SANDBOX_SPAWN_FAILED, + SANDBOX_WORKSPACE_VIOLATION, +) from ai_company.observability.events.task import TASK_STATUS_CHANGED from ai_company.observability.events.template import ( TEMPLATE_RENDER_START, @@ -137,5 +150,18 @@ def test_git_events_exist(self) -> None: assert GIT_CLONE_URL_REJECTED == "git.clone.url_rejected" assert GIT_REF_INJECTION_BLOCKED == "git.ref.injection_blocked" + def test_sandbox_events_exist(self) -> None: + assert SANDBOX_EXECUTE_START == "sandbox.execute.start" + assert SANDBOX_EXECUTE_SUCCESS == "sandbox.execute.success" + assert SANDBOX_EXECUTE_FAILED == "sandbox.execute.failed" + assert SANDBOX_EXECUTE_TIMEOUT == "sandbox.execute.timeout" + assert SANDBOX_SPAWN_FAILED == "sandbox.spawn.failed" + assert SANDBOX_ENV_FILTERED == "sandbox.env.filtered" + assert SANDBOX_WORKSPACE_VIOLATION == "sandbox.workspace.violation" + assert SANDBOX_CLEANUP == "sandbox.cleanup" + assert SANDBOX_PATH_FALLBACK == "sandbox.path.fallback" + assert SANDBOX_HEALTH_CHECK == "sandbox.health_check" + assert SANDBOX_KILL_FAILED == "sandbox.kill.failed" + def test_tool_events_exist(self) -> None: assert TOOL_INVOKE_START == "tool.invoke.start" diff --git a/tests/unit/tools/git/test_git_sandbox_integration.py b/tests/unit/tools/git/test_git_sandbox_integration.py index cc97efc418..9c534ec382 100644 --- a/tests/unit/tools/git/test_git_sandbox_integration.py +++ b/tests/unit/tools/git/test_git_sandbox_integration.py @@ -7,6 +7,7 @@ from ai_company.tools.git_tools import ( GitBranchTool, + GitCloneTool, GitCommitTool, GitDiffTool, GitLogTool, @@ -60,6 +61,19 @@ async def test_commit_with_sandbox(self, git_repo: Path) -> None: ) assert not result.is_error + async def test_clone_with_sandbox(self, git_repo: Path) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + tool = GitCloneTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute( + arguments={ + "url": git_repo.as_uri(), + "directory": "cloned", + }, + ) + # file:// URLs are intentionally rejected + assert result.is_error + assert "Invalid clone URL" in result.content + class TestGitToolsWithoutSandbox: """Git tools work without sandbox (backward compat).""" @@ -115,6 +129,77 @@ async def test_sandbox_error_returns_error( assert "workspace violation" in result.content +class TestUnexpectedSandboxException: + """Non-SandboxError exceptions propagate (not swallowed).""" + + async def test_unexpected_exception_propagates( + self, + git_repo: Path, + ) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + sandbox.execute = AsyncMock( # type: ignore[method-assign] + side_effect=RuntimeError("unexpected bug"), + ) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) + with pytest.raises(RuntimeError, match="unexpected bug"): + await tool.execute(arguments={}) + + +class TestSandboxResultConversion: + """_sandbox_result_to_execution_result handles edge cases.""" + + async def test_nonzero_returncode_prefers_stderr( + self, + git_repo: Path, + ) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + sandbox.execute = AsyncMock( # type: ignore[method-assign] + return_value=SandboxResult( + stdout="stdout detail", + stderr="stderr detail", + returncode=1, + ), + ) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert result.is_error + assert "stderr detail" in result.content + + async def test_nonzero_returncode_falls_back_to_stdout( + self, + git_repo: Path, + ) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + sandbox.execute = AsyncMock( # type: ignore[method-assign] + return_value=SandboxResult( + stdout="stdout detail", + stderr="", + returncode=1, + ), + ) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert result.is_error + assert "stdout detail" in result.content + + async def test_nonzero_returncode_unknown_error_fallback( + self, + git_repo: Path, + ) -> None: + sandbox = SubprocessSandbox(workspace=git_repo) + sandbox.execute = AsyncMock( # type: ignore[method-assign] + return_value=SandboxResult( + stdout="", + stderr="", + returncode=1, + ), + ) + tool = GitStatusTool(workspace=git_repo, sandbox=sandbox) + result = await tool.execute(arguments={}) + assert result.is_error + assert "Unknown git error" in result.content + + class TestGitHardeningWithSandbox: """Git hardening env vars are passed via env_overrides.""" diff --git a/tests/unit/tools/sandbox/test_result.py b/tests/unit/tools/sandbox/test_result.py index 8344663091..7f3e4a248c 100644 --- a/tests/unit/tools/sandbox/test_result.py +++ b/tests/unit/tools/sandbox/test_result.py @@ -11,39 +11,29 @@ class TestSandboxResult: """SandboxResult is frozen with a computed ``success`` field.""" - def test_success_when_zero_returncode_no_timeout(self) -> None: - result = SandboxResult( - stdout="ok", - stderr="", - returncode=0, - ) - assert result.success is True - - def test_failure_when_nonzero_returncode(self) -> None: - result = SandboxResult( - stdout="", - stderr="error", - returncode=1, - ) - assert result.success is False - - def test_failure_when_timed_out(self) -> None: - result = SandboxResult( - stdout="", - stderr="timeout", - returncode=0, - timed_out=True, - ) - assert result.success is False - - def test_failure_when_both_nonzero_and_timed_out(self) -> None: + @pytest.mark.parametrize( + ("returncode", "timed_out", "expected_success"), + [ + (0, False, True), + (1, False, False), + (0, True, False), + (-1, True, False), + ], + ids=["zero-no-timeout", "nonzero", "timeout-zero", "timeout-neg"], + ) + def test_success_matrix( + self, + returncode: int, + timed_out: bool, + expected_success: bool, + ) -> None: result = SandboxResult( stdout="", stderr="", - returncode=-1, - timed_out=True, + returncode=returncode, + timed_out=timed_out, ) - assert result.success is False + assert result.success is expected_success def test_frozen(self) -> None: result = SandboxResult( diff --git a/tests/unit/tools/sandbox/test_subprocess_sandbox.py b/tests/unit/tools/sandbox/test_subprocess_sandbox.py index 84f82ac02f..70f4c49a02 100644 --- a/tests/unit/tools/sandbox/test_subprocess_sandbox.py +++ b/tests/unit/tools/sandbox/test_subprocess_sandbox.py @@ -107,6 +107,29 @@ def test_env_overrides_applied( ) assert env["GIT_TERMINAL_PROMPT"] == "0" + def test_env_overrides_bypass_denylist( + self, + sandbox_workspace: Path, + ) -> None: + """env_overrides bypass denylist by design (security contract).""" + config = SubprocessSandboxConfig( + env_allowlist=("HOME",), + env_denylist_patterns=("*KEY*",), + ) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + with patch.dict( + os.environ, + {"HOME": "/home/test"}, + clear=True, + ): + env = sandbox._build_filtered_env( + env_overrides={"API_KEY": "override_value"}, + ) + assert env["API_KEY"] == "override_value" + def test_lc_glob_matching( self, sandbox_workspace: Path, @@ -150,6 +173,47 @@ def test_restricted_path_filters_entries( env = sandbox._build_filtered_env() assert "suspicious" not in env.get("PATH", "").lower() + def test_restricted_path_rejects_prefix_spoofing( + self, + sandbox_workspace: Path, + ) -> None: + """PATH entries like /usr/bin-malicious are rejected.""" + config = SubprocessSandboxConfig(restricted_path=True) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + if os.name == "nt": + spoofed = r"C:\WINDOWS\system32;C:\WINDOWS-extra\bin" + else: + spoofed = "/usr/bin:/usr/bin-malicious" + with patch.dict( + os.environ, + {"PATH": spoofed}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert "malicious" not in env.get("PATH", "").lower() + assert "extra" not in env.get("PATH", "").lower() + + def test_path_fallback_when_no_entries_match( + self, + sandbox_workspace: Path, + ) -> None: + """PATH fallback uses safe directories that exist.""" + config = SubprocessSandboxConfig(restricted_path=True) + sandbox = SubprocessSandbox( + config=config, + workspace=sandbox_workspace, + ) + with patch.dict( + os.environ, + {"PATH": "/totally/fake/dir"}, + clear=True, + ): + env = sandbox._build_filtered_env() + assert "/totally/fake/dir" not in env.get("PATH", "") + # ── Workspace boundary ─────────────────────────────────────────── @@ -254,6 +318,26 @@ async def test_timeout_kills_process( assert result.timed_out assert not result.success + async def test_zero_timeout_kills_process( + self, + subprocess_sandbox: SubprocessSandbox, + ) -> None: + """timeout=0.0 is treated as immediate timeout, not default.""" + if os.name == "nt": + result = await subprocess_sandbox.execute( + command="cmd", + args=("/c", "ping", "-n", "10", "127.0.0.1"), + timeout=0.0, + ) + else: + result = await subprocess_sandbox.execute( + command="sleep", + args=("10",), + timeout=0.0, + ) + assert result.timed_out + assert not result.success + @pytest.mark.filterwarnings("ignore::ResourceWarning") async def test_start_failure( self,