diff --git a/CHANGELOG.md b/CHANGELOG.md index df5b21249..ef360dd8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,16 @@ prevented by a `contextvars` nesting guard. Per-service overrides are loaded from the `retry_service_overrides` JSON config key. Includes Behave BDD unit tests, Robot Framework integration tests, and ASV benchmarks. (#313) +- Added container-aware tool execution and I/O forwarding via `ContainerToolExecutor` + and `PathMapper`. Tools routed to `execution_environment: container` are executed + inside a provisioned devcontainer with automatic host↔container path mapping, + bounded output capture (50 MiB), structured error reporting, and container metadata + on the `ToolInvocation` audit trail. Includes `ContainerConfig`, `ContainerMetadata`, + `ContainerExecutionError`, and `ContainerTimeoutError` domain models, `ToolRunner` + container routing integration, safe environment filtering, symlink/traversal + protection, and `sync_results_to_host` for file-based result retrieval. Covered by + Behave BDD scenarios, Robot Framework integration tests, ASV benchmarks, and + `docs/reference/execution_environment.md`. (#515) ### Added diff --git a/alembic/versions/m6_004_container_metadata_column.py b/alembic/versions/m6_004_container_metadata_column.py new file mode 100644 index 000000000..b3343af07 --- /dev/null +++ b/alembic/versions/m6_004_container_metadata_column.py @@ -0,0 +1,32 @@ +"""Add container_metadata_json column to tool_invocations table. + +Revision ID: m6_004_container_metadata_column +Revises: m6_004_resource_type_inherits +Create Date: 2026-03-11 00:00:00 + +""" + +from collections.abc import Sequence + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision: str = "m6_004_container_metadata_column" +down_revision: str | Sequence[str] | None = "m6_004_resource_type_inherits" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """Add container_metadata_json column for container execution audit trail.""" + with op.batch_alter_table("tool_invocations") as batch_op: + batch_op.add_column( + sa.Column("container_metadata_json", sa.Text(), nullable=True), + ) + + +def downgrade() -> None: + """Remove container_metadata_json column.""" + with op.batch_alter_table("tool_invocations") as batch_op: + batch_op.drop_column("container_metadata_json") diff --git a/benchmarks/container_tool_exec_bench.py b/benchmarks/container_tool_exec_bench.py new file mode 100644 index 000000000..e2524189b --- /dev/null +++ b/benchmarks/container_tool_exec_bench.py @@ -0,0 +1,153 @@ +"""ASV benchmarks for container tool execution overhead (#515). + +Measures the performance cost of path mapping, input/output +translation, and container metadata creation — the operations +that happen on every container-routed tool call. +""" + +from __future__ import annotations + +import json + +from cleveragents.tool.container_executor import ( + ContainerConfig, + ContainerMetadata, + ContainerToolExecutor, +) +from cleveragents.tool.path_mapper import PathMapper + + +class PathMapperBench: + """Benchmark PathMapper host-to-container and container-to-host translations.""" + + def setup(self) -> None: + self.mapper = PathMapper( + host_root="/tmp/sandbox/project-abc", + container_root="/workspace", + ) + self.host_path = "/tmp/sandbox/project-abc/src/cleveragents/tool/runner.py" + self.container_path = "/workspace/src/cleveragents/tool/runner.py" + self.external_path = ( + "/usr/local/lib/python3.13/site-packages/pydantic/__init__.py" + ) + + def time_host_to_container(self) -> None: + self.mapper.host_to_container(self.host_path) + + def time_container_to_host(self) -> None: + self.mapper.container_to_host(self.container_path) + + def time_external_path_passthrough(self) -> None: + self.mapper.host_to_container(self.external_path) + + def time_is_host_path_check(self) -> None: + self.mapper.is_host_path(self.host_path) + + def time_is_container_path_check(self) -> None: + self.mapper.is_container_path(self.container_path) + + +class InputPathMappingBench: + """Benchmark input path mapping with various nesting depths.""" + + def setup(self) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + host_sandbox_path="/tmp/sandbox", + container_id="bench-ctr", + ) + self.executor = ContainerToolExecutor(config) + self.flat_inputs = { + "path": "/tmp/sandbox/src/main.py", + "output_dir": "/tmp/sandbox/build", + "name": "my-tool", + "count": 42, + } + self.nested_inputs = { + "paths": { + "source": "/tmp/sandbox/src/main.py", + "config": "/tmp/sandbox/.config/settings.json", + "nested": { + "deep": "/tmp/sandbox/deep/path.txt", + }, + }, + "list_paths": [ + "/tmp/sandbox/a.py", + "/tmp/sandbox/b.py", + "/tmp/sandbox/c.py", + ], + } + + def time_flat_input_mapping(self) -> None: + self.executor._map_input_paths(self.flat_inputs) + + def time_nested_input_mapping(self) -> None: + self.executor._map_input_paths(self.nested_inputs) + + +class OutputPathMappingBench: + """Benchmark output path mapping.""" + + def setup(self) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + host_sandbox_path="/tmp/sandbox", + container_id="bench-ctr", + ) + self.executor = ContainerToolExecutor(config) + self.output = { + "created_file": "/workspace/build/output.txt", + "log_file": "/workspace/logs/run.log", + "status": "success", + "count": 10, + } + + def time_output_mapping(self) -> None: + self.executor._map_output_paths(self.output) + + +class ContainerMetadataBench: + """Benchmark container metadata creation and serialisation.""" + + def setup(self) -> None: + self.metadata = ContainerMetadata( + container_id="abc123def456", + image="ghcr.io/org/devimage:latest", + workspace_folder="/workspace", + exec_time_ms=1500.0, + exit_code=0, + timed_out=False, + ) + + def time_metadata_creation(self) -> None: + ContainerMetadata( + container_id="abc123def456", + image="ghcr.io/org/devimage:latest", + workspace_folder="/workspace", + exec_time_ms=1500.0, + exit_code=0, + timed_out=False, + ) + + def time_metadata_to_dict(self) -> None: + self.metadata.model_dump() + + +class OutputParsingBench: + """Benchmark output parsing (JSON vs plain text).""" + + def setup(self) -> None: + self.json_output = json.dumps( + {"files": [f"file_{i}.py" for i in range(50)], "errors": 0} + ) + self.plain_output = "Build completed successfully\n" * 20 + self.empty_output = "" + + def time_parse_json_output(self) -> None: + ContainerToolExecutor._parse_output(self.json_output) + + def time_parse_plain_output(self) -> None: + ContainerToolExecutor._parse_output(self.plain_output) + + def time_parse_empty_output(self) -> None: + ContainerToolExecutor._parse_output(self.empty_output) diff --git a/docs/reference/execution_environment.md b/docs/reference/execution_environment.md new file mode 100644 index 000000000..3b895cb20 --- /dev/null +++ b/docs/reference/execution_environment.md @@ -0,0 +1,225 @@ +# Execution Environment Reference + +This document describes how CleverAgents routes tool execution between +host and container environments, including the container-aware tool +executor, path mapping, and I/O forwarding. + +## Overview + +When a plan or tool specifies `execution_environment: container`, +the tool runner delegates execution to a **ContainerToolExecutor** that: + +1. Maps host-side file paths to container-side equivalents. +2. Executes the tool via `devcontainer exec`. +3. Retrieves results and maps output paths back to the host. +4. Records container metadata on the `ToolInvocation` audit trail. + +## Execution Environment Resolution + +The execution environment is resolved using a priority chain: + +| Priority | Source | Example | +|----------|--------|---------| +| 1 (highest) | Tool-level override | `tool_env="container"` | +| 2 | Plan-level override | `--execution-environment container` | +| 3 | Project-level default | `ContextConfig.execution_environment` | +| 4 (lowest) | System default | `HOST` | + +If `CONTAINER` is resolved, the system validates that a container +resource (devcontainer-instance, container-instance, or +devcontainer-file) is linked to the project. + +## Path Mapping + +The `PathMapper` utility translates file paths between host and +container filesystems: + +```python +from cleveragents.tool.path_mapper import PathMapper + +mapper = PathMapper( + host_root="/tmp/sandbox/project-1", + container_root="/workspace", +) + +# Host to container +mapper.host_to_container("/tmp/sandbox/project-1/src/main.py") +# => "/workspace/src/main.py" + +# Container to host +mapper.container_to_host("/workspace/src/main.py") +# => "/tmp/sandbox/project-1/src/main.py" + +# Paths outside root are returned unchanged +mapper.host_to_container("/usr/lib/libc.so") +# => "/usr/lib/libc.so" +``` + +### Path Mapping in Tool Inputs + +File path arguments in tool inputs are automatically mapped before +container execution. The executor recursively traverses the input +dict, mapping any string values that fall under the host root: + +```python +# Before mapping +{"path": "/tmp/sandbox/src/main.py", "count": 5} + +# After mapping +{"path": "/workspace/src/main.py", "count": 5} +``` + +Nested dicts and lists are also traversed. + +## Container Tool Executor + +The `ContainerToolExecutor` wraps tool execution via `devcontainer exec`: + +```python +from cleveragents.tool.container_executor import ( + ContainerConfig, + ContainerToolExecutor, +) + +config = ContainerConfig( + workspace_folder="/workspace", + container_id="my-container", + image="ghcr.io/org/devimage:latest", + host_sandbox_path="/tmp/sandbox/project-1", + timeout_seconds=120, +) + +executor = ContainerToolExecutor(config) +result = executor.execute_tool("file_read", {"path": "/tmp/sandbox/project-1/src/main.py"}) +``` + +### Configuration + +| Parameter | Type | Default | Description | +|-----------|------|---------|-------------| +| `workspace_folder` | `str` | `/workspace` | Workspace root inside the container | +| `container_id` | `str` | `""` | Docker/Podman container ID | +| `image` | `str` | `""` | Container image reference | +| `timeout_seconds` | `int` | `120` | Max execution time (seconds) | +| `host_sandbox_path` | `str` | `""` | Host-side sandbox root | + +### Timeout Handling + +If a container tool execution exceeds `timeout_seconds`, the +executor returns a `ToolResult` with `success=False` and an error +message indicating the timeout. The `metadata.container` dict on +the result will have `timed_out=True`. + +### Error Reporting + +Container execution failures produce structured `ToolResult` objects: + +- **Timeout**: `error="Container execution timed out after Ns"` +- **Non-zero exit**: `error="Container execution failed (exit code N): stderr"` +- **OS error**: `error="Container execution failed (exit code -1): error message"` + +All errors include `container_metadata` with execution details. + +## Result Retrieval + +After container execution, file-based outputs can be synced back to +the host using `sync_results_to_host`: + +```python +host_path = executor.sync_results_to_host("/workspace/build/output.txt") +# => "/tmp/sandbox/project-1/build/output.txt" +``` + +## Audit Trail + +When a tool executes inside a container, the `ToolResult.metadata` +dict includes a `container` key with execution details, and the +`ToolInvocation` record stores this as the `container_metadata` field: + +```json +{ + "metadata": { + "container": { + "container_id": "abc123", + "image": "ghcr.io/org/devimage:latest", + "workspace_folder": "/workspace", + "exec_time_ms": 1500.0, + "exit_code": 0, + "timed_out": false + } + } +} +``` + +This provides full traceability for container-routed tool executions. + +## Security Model + +Container tool execution enforces several layers of protection: + +### Environment Variable Filtering + +Only a minimal allowlist of environment variables (`PATH`, `LANG`, +`TERM`) is forwarded to the subprocess that invokes `devcontainer +exec`. All other host environment variables—including secrets, +credentials, and `HOME`—are excluded. + +### Symlink and Path Traversal Protection + +- **Sandbox root validation**: `sync_results_to_host` resolves the + mapped host path and verifies it falls within the sandbox root + *before* writing any data. Path traversal via `..` is rejected. +- **Symlink guard**: The default sandbox fallback (`/tmp/sandbox`) + is rejected if it is a symlink. Explicit `host_sandbox_path` is + required in that case. +- **`O_NOFOLLOW` writes**: Files written by `sync_results_to_host` + use `O_NOFOLLOW` to prevent symlink-following at write time. +- **Root mapping rejection**: `PathMapper` rejects `/` and `//` as + host or container roots to prevent mapping every absolute path. + +### Output Caps + +Captured stdout and stderr are bounded to 50 MiB per stream. +Output exceeding this limit is truncated with a warning log entry. +This prevents a misbehaving container from exhausting host memory. + +### Permission Model + +- Files written by `sync_results_to_host` are created with mode + `0o600` (owner read/write only). +- Parent directories are created with mode `0o700` (owner only). +- The `devcontainer exec` binary is resolved at init time via + `shutil.which` to prevent PATH hijacking. If the binary is not + found, all execution calls fail fast with a descriptive error. + +### Input Validation + +- Null bytes in paths are rejected at all entry points. +- Tool inputs are validated as JSON-serialisable before container + execution (with `allow_nan=False` per RFC 7159). +- JSON inputs are piped via stdin rather than shell arguments to + avoid `ARG_MAX` limits and shell injection. + +## ToolRunner Integration + +The `ToolRunner` accepts an optional `container_executor` parameter: + +```python +from cleveragents.tool.runner import ToolRunner +from cleveragents.tool.container_executor import ( + ContainerConfig, + ContainerToolExecutor, +) + +config = ContainerConfig(...) +executor = ContainerToolExecutor(config) +runner = ToolRunner( + registry=registry, + container_executor=executor, +) +``` + +When the execution environment resolves to `CONTAINER` and a +`ContainerToolExecutor` is configured, the runner delegates to it +automatically. If no executor is configured, the runner returns an +error result indicating that container execution is not available. diff --git a/features/container_tool_exec.feature b/features/container_tool_exec.feature new file mode 100644 index 000000000..71576e174 --- /dev/null +++ b/features/container_tool_exec.feature @@ -0,0 +1,517 @@ +@container-tool-exec +Feature: Container-aware tool execution and I/O forwarding + As a CleverAgents developer + I want tools to execute inside containers when the execution environment is CONTAINER + So that file-based tools operate on the container filesystem transparently + + Background: + Given I have the container tool execution module imported + + # -- PathMapper --------------------------------------------------------- + + Scenario: PathMapper maps host path to container path + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I map host path "/tmp/sandbox/src/main.py" to container + Then the container path should be "/workspace/src/main.py" + + Scenario: PathMapper maps container path to host path + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I map container path "/workspace/src/main.py" to host + Then the host path should be "/tmp/sandbox/src/main.py" + + Scenario: PathMapper returns unmapped path when outside root + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I map host path "/usr/lib/libc.so" to container + Then the container path should be "/usr/lib/libc.so" + + Scenario: PathMapper maps root path exactly + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I map host path "/tmp/sandbox" to container + Then the container path should be "/workspace" + + Scenario: PathMapper is_host_path returns true for host paths + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + Then "/tmp/sandbox/file.txt" should be a host path + And "/usr/lib/libc.so" should not be a host path + + Scenario: PathMapper is_container_path returns true for container paths + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + Then "/workspace/file.txt" should be a container path + And "/tmp/other/file.txt" should not be a container path + + # -- ContainerConfig ---------------------------------------------------- + + Scenario: ContainerConfig has sensible defaults + When I create a default ContainerConfig + Then the workspace_folder should be "/workspace" + And the container timeout_seconds should be 120 + + # -- ContainerMetadata -------------------------------------------------- + + Scenario: ContainerMetadata is frozen and holds execution details + When I create a ContainerMetadata with container_id "abc123" and image "node:20" + Then the container_metadata container_id should be "abc123" + And the container_metadata image should be "node:20" + And the container_metadata timed_out should be false + + # -- ContainerToolExecutor basic ---------------------------------------- + + Scenario: ContainerToolExecutor can be instantiated with config + Given I have a ContainerConfig with workspace "/workspace" and container_id "test-ctr" + When I create a ContainerToolExecutor with that config + Then the executor config container_id should be "test-ctr" + And the executor should have a path mapper + + Scenario: ContainerToolExecutor maps input paths before execution + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool inputs with path "/tmp/sandbox/src/file.py" + Then the mapped input path should be "/workspace/src/file.py" + + Scenario: ContainerToolExecutor maps output paths after execution + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool output with path "/workspace/build/output.txt" + Then the mapped output path should be "/tmp/sandbox/build/output.txt" + + Scenario: ContainerToolExecutor handles nested input path mapping + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool inputs with nested paths under "/tmp/sandbox" + Then all nested paths should be mapped to container paths + + # -- Timeout handling --------------------------------------------------- + + Scenario: ContainerToolExecutor reports timeout as structured error + Given I have a ContainerToolExecutor with a mock that times out + When I execute tool "file_read" with inputs in the container + Then the container tool result should indicate failure + And the container tool result error should mention "timed out" + And the tool result metadata should contain container info with timed_out true + + # -- Error reporting ---------------------------------------------------- + + Scenario: ContainerToolExecutor reports non-zero exit as structured error + Given I have a ContainerToolExecutor with a mock that returns exit code 1 + When I execute tool "file_read" with inputs in the container + Then the container tool result should indicate failure + And the container tool result error should mention "exit code 1" + + Scenario: ContainerToolExecutor reports OSError as structured error + Given I have a ContainerToolExecutor with subprocess raising OSError + When I execute tool "compile" with inputs in the container + Then the container tool result should indicate failure + + # -- Successful execution ----------------------------------------------- + + Scenario: ContainerToolExecutor returns parsed JSON output on success + Given I have a ContainerToolExecutor with a mock that returns JSON output + When I execute tool "lint" with inputs in the container + Then the container tool result should indicate success + And the tool result output should contain the parsed JSON + And the tool result metadata should contain container info + + Scenario: ContainerToolExecutor returns raw output when not JSON + Given I have a ContainerToolExecutor with a mock that returns plain text + When I execute tool "echo" with inputs in the container + Then the container tool result should indicate success + And the tool result output should contain raw_output + + # -- Command building --------------------------------------------------- + + Scenario: _build_exec_command places --container-id flag adjacent to its value + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I build an exec command for tool "test_tool" with timeout 30 + Then the command should have "--container-id" adjacent to its value + + # -- ToolRunner container routing --------------------------------------- + + Scenario: ToolRunner delegates to ContainerToolExecutor when env is CONTAINER + Given I have a ToolRunner with a ContainerToolExecutor mock + When I execute a tool with container execution environment + Then the ContainerToolExecutor should have been called + And the resolver should have been called with the correct arguments + + Scenario: ToolRunner returns error when container executor is not configured + Given I have a ToolRunner without a ContainerToolExecutor + When I execute a tool with container execution environment + Then the container tool result error should mention "ContainerToolExecutor must be configured" + + # -- ToolInvocation audit trail ----------------------------------------- + + Scenario: ToolInvocation has container_metadata field + When I create a ToolInvocation with container_metadata + Then the ToolInvocation container_metadata should contain "container_id" + And the ToolInvocation container_metadata should contain "image" + + Scenario: ToolInvocation container_metadata defaults to None + When I create a ToolInvocation without container_metadata + Then the ToolInvocation container_metadata should be None + + # -- ContainerExecutionError -------------------------------------------- + + Scenario: ContainerExecutionError carries exit_code and stderr + When I raise a ContainerExecutionError with exit_code 2 and stderr "oops" + Then the error exit_code should be 2 + And the error stderr should be "oops" + + Scenario: ContainerTimeoutError carries timeout_seconds + When I raise a ContainerTimeoutError with timeout 30 + Then the timeout error message should mention "30" + And the timeout error timed_out should be true + + # -- Signal exit code preservation ---------------------------------------- + + Scenario: ContainerToolExecutor preserves negative exit codes from signal kills + Given I have a ContainerToolExecutor with a mock that returns exit code -9 + When I execute tool "compile" with inputs in the container + Then the container tool result should indicate failure + And the tool result metadata should have exit_code -9 + + # -- Path mapping with spaces -------------------------------------------- + + Scenario: _looks_like_path accepts paths with spaces + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool inputs with path "/tmp/sandbox/My Documents/file.py" + Then the mapped input path should be "/workspace/My Documents/file.py" + + # -- Output truncation --------------------------------------------------- + + Scenario: ContainerToolExecutor truncates output exceeding max bytes + Given I have a ContainerToolExecutor with a mock that returns oversized output + When I execute tool "big_output" with inputs in the container + Then the container tool result stdout should be truncated + + # -- Path traversal protection ------------------------------------------- + + Scenario: sync_results_to_host rejects path traversal attempts + Given I have a ContainerToolExecutor with a mock that succeeds on sync using a temp dir + When I attempt to sync a path that traverses outside the sandbox + Then a ContainerExecutionError should be raised for path traversal + + # -- Sync results ------------------------------------------------------- + + Scenario: sync_results_to_host maps container path to host + Given I have a ContainerToolExecutor with a mock that succeeds on sync using a temp dir + When I sync container path "/workspace/output.txt" to host + Then the synced host path should be under the host sandbox root + + # -- execute_tool input validation (P2-19) -------------------------------- + + Scenario: execute_tool rejects empty tool_name + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I execute tool with empty tool_name + Then a ValueError should be raised with message "non-empty" + + Scenario: execute_tool rejects non-dict inputs + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I execute tool with non-dict inputs + Then a container TypeError should be raised with message "must be a dict" + + Scenario: execute_tool rejects negative timeout_seconds + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I execute tool with negative timeout + Then a ValueError should be raised with message "must be > 0" + + # -- extract_container_metadata helper (P1-9) ----------------------------- + + Scenario: extract_container_metadata returns container dict from ToolResult + When I create a ToolResult with container metadata in metadata dict + Then extract_container_metadata should return the container dict + + # ========================================================================= + # Safe initialization — PathMapper validation guards + # ========================================================================= + + Scenario: PathMapper rejects null bytes in host_root + When I try to create a PathMapper with null bytes in host_root + Then a PathMapper ValueError should be raised mentioning "null" + + Scenario: PathMapper rejects null bytes in container_root + When I try to create a PathMapper with null bytes in container_root + Then a PathMapper ValueError should be raised mentioning "null" + + Scenario: PathMapper rejects empty host_root + When I try to create a PathMapper with empty host_root + Then a PathMapper ValueError should be raised mentioning "empty" + + Scenario: PathMapper rejects filesystem-root host_root + When I try to create a PathMapper with host_root "/" + Then a PathMapper ValueError should be raised mentioning "/" + + Scenario: PathMapper rejects filesystem-root container_root + When I try to create a PathMapper with container_root "/" + Then a PathMapper ValueError should be raised mentioning "/" + + Scenario: PathMapper rejects overlapping roots + When I try to create a PathMapper with overlapping roots + Then a PathMapper ValueError should be raised mentioning "overlap" + + Scenario: PathMapper is_host_path returns false for null-byte paths + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + Then a path with null bytes should not be a host path + + Scenario: PathMapper is_container_path returns false for null-byte paths + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + Then a path with null bytes should not be a container path + + # ========================================================================= + # Safe initialization — ContainerConfig workspace_folder guards + # ========================================================================= + + Scenario: ContainerConfig rejects relative workspace_folder + When I try to create a ContainerConfig with workspace_folder "relative/path" + Then a ContainerConfig ValueError should be raised mentioning "absolute" + + Scenario: ContainerConfig rejects workspace_folder that normalises to root + When I try to create a ContainerConfig with workspace_folder "/." + Then a ContainerConfig ValueError should be raised mentioning "root" + + Scenario: ContainerConfig rejects workspace_folder with dotdot traversal + When I try to create a ContainerConfig with workspace_folder "/workspace/../etc" + Then a ContainerConfig ValueError should be raised mentioning ".." + + # ========================================================================= + # Safe initialization — ContainerToolExecutor binary resolution + # ========================================================================= + + Scenario: ContainerToolExecutor without devcontainer binary uses workspace-folder target args + Given I have a ContainerToolExecutor with no container_id configured + When I request the target CLI arguments + Then the target args should use "--workspace-folder" + + Scenario: ContainerToolExecutor raises when devcontainer binary is missing at execution + Given I have a ContainerToolExecutor with no devcontainer binary + When I try to require the devcontainer binary + Then a ContainerExecutionError should be raised about missing binary + + # ========================================================================= + # Cleanup — sync_results_to_host error paths + # ========================================================================= + + Scenario: sync_results_to_host rejects null bytes in container_path + Given I have a ContainerToolExecutor with a mock that succeeds on sync using a temp dir + When I try to sync a path containing null bytes + Then a sync ValueError should be raised mentioning "null" + + Scenario: sync_results_to_host rejects relative container_path + Given I have a ContainerToolExecutor with a mock that succeeds on sync using a temp dir + When I try to sync a relative container path + Then a sync ValueError should be raised mentioning "absolute" + + Scenario: sync_results_to_host raises on timeout during file sync + Given I have a ContainerToolExecutor with a mock that times out on sync + When I try to sync container path to host with timeout + Then a ContainerTimeoutError should be raised from sync + + Scenario: sync_results_to_host raises on non-zero exit during file sync + Given I have a ContainerToolExecutor with a mock that returns non-zero exit on sync + When I try to sync container path to host with failure + Then a ContainerExecutionError should be raised from sync failure + + Scenario: sync_results_to_host raises on OSError during file write + Given I have a ContainerToolExecutor with a mock that triggers OSError on write + When I try to sync container path to host triggering write error + Then a ContainerExecutionError should be raised from write failure + + # ========================================================================= + # Safe execution — _looks_like_path heuristic boundary checks + # ========================================================================= + + Scenario: _looks_like_path rejects strings with newlines + When I check whether a string with newlines looks like a path + Then it should not look like a path + + Scenario: _looks_like_path rejects strings with null bytes + When I check whether a string with null bytes looks like a path + Then it should not look like a path + + Scenario: _looks_like_path rejects protocol-relative URIs + When I check whether a protocol-relative URI looks like a path + Then it should not look like a path + + Scenario: _looks_like_path rejects strings with query parameters + When I check whether a path with query string looks like a path + Then it should not look like a path + + Scenario: _looks_like_path rejects strings with fragment identifiers + When I check whether a path with fragment identifier looks like a path + Then it should not look like a path + + Scenario: _looks_like_path rejects strings with scheme separators + When I check whether a URL-like string looks like a path + Then it should not look like a path + + # ========================================================================= + # Safe execution — recursion depth guards on path mapping + # ========================================================================= + + Scenario: _map_input_paths stops at max recursion depth + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map deeply nested input paths exceeding the recursion limit + Then the input mapping should return without error + + Scenario: _map_output_paths stops at max recursion depth + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map deeply nested output paths exceeding the recursion limit + Then the output mapping should return without error + + # ========================================================================= + # Safe execution — _parse_output edge cases + # ========================================================================= + + Scenario: _parse_output returns result key for non-dict JSON + When I parse stdout containing a JSON array + Then the parsed output should wrap it under "result" key + + Scenario: _parse_output returns empty dict for blank stdout + When I parse empty stdout + Then the parsed output should be an empty dict + + # ========================================================================= + # Safe execution — _map_output_paths skips raw_output key + # ========================================================================= + + Scenario: _map_output_paths preserves raw_output without path mapping + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map output containing a raw_output key with container path + Then the raw_output value should remain unmapped + + # ========================================================================= + # Safe execution — container tool non-JSON-serialisable inputs + # ========================================================================= + + Scenario: execute_tool returns error for non-JSON-serialisable inputs + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I execute a tool with non-serialisable inputs + Then the container tool result should indicate failure + And the container tool result error should mention "JSON-serialisable" + + # ========================================================================= + # ToolRunner full lifecycle — discover, activate, deactivate + # ========================================================================= + + Scenario: ToolRunner discover returns tools from registry + Given I have a ToolRunner with a populated registry + When I call discover on the runner + Then the discovered tools list should not be empty + + Scenario: ToolRunner activate marks a tool as ready + Given I have a ToolRunner with a populated registry + When I activate a tool on the runner + Then the activated tool spec should be returned + + Scenario: ToolRunner activate raises ToolError for unknown tool + Given I have a ToolRunner with a populated registry + When I try to activate an unknown tool + Then a ToolError should be raised for activation + + Scenario: ToolRunner deactivate cleans up active tool + Given I have a ToolRunner with a populated registry + When I activate and then deactivate a tool + Then deactivate should return true + And a second deactivate should return false + + # ========================================================================= + # ToolRunner host execution path + # ========================================================================= + + Scenario: ToolRunner executes tool on host and returns success + Given I have a ToolRunner with a host-routed tool + When I execute the host tool with valid inputs + Then the host tool result should indicate success + And the host tool result output should contain handler data + + Scenario: ToolRunner normalises non-dict handler output + Given I have a ToolRunner with a tool returning non-dict output + When I execute the non-dict-output tool + Then the result output should wrap the value under "result" + + Scenario: ToolRunner catches handler exception and returns error + Given I have a ToolRunner with a tool that raises an exception + When I execute the raising tool + Then the tool result should indicate failure with exception info + + Scenario: ToolRunner returns error for non-JSON-serialisable host output + Given I have a ToolRunner with a tool returning non-serialisable output + When I execute the non-serialisable-output tool + Then the tool result should indicate failure mentioning "not JSON-serialisable" + + Scenario: ToolRunner returns error for non-JSON-serialisable host inputs + Given I have a ToolRunner with a host-routed tool + When I execute the host tool with non-serialisable inputs + Then the tool result should indicate failure mentioning "not JSON-serialisable" + + Scenario: ToolRunner raises ToolError for unregistered tool in execute + Given I have a ToolRunner with an empty registry + When I try to execute an unregistered tool + Then a ToolError should be raised for execution + + # ========================================================================= + # ToolRunner container routing — additional edge cases + # ========================================================================= + + Scenario: ToolRunner returns error when container env resolver raises ContainerUnavailableError + Given I have a ToolRunner with an unavailable-container resolver + When I execute a tool through the unavailable-container runner + Then the tool result should indicate container unavailable + + Scenario: ToolRunner validates missing required input fields for container tools + Given I have a ToolRunner with a container executor and required-field tool + When I execute the container tool with missing required fields + Then the tool result error should mention "Missing required input fields" + + Scenario: ToolRunner returns error for non-JSON-serialisable container inputs + Given I have a ToolRunner with a container executor for JSON validation + When I execute a container tool with non-serialisable inputs + Then the container tool result error should mention "not JSON-serialisable" + + Scenario: ToolRunner catches container executor exception + Given I have a ToolRunner with a failing container executor + When I execute a tool through the failing container runner + Then the tool result error should mention "Container execution error" + + # ========================================================================= + # Safe execution — list-type value mapping branches + # ========================================================================= + + Scenario: _map_input_paths handles list values containing host paths + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool inputs containing a list of host paths + Then the list values should be mapped to container paths + + Scenario: _map_output_paths handles list values containing container paths + Given I have a ContainerToolExecutor with host_root "/tmp/sandbox" and container_root "/workspace" + When I map tool output containing a list of container paths + Then the list values should be mapped to host paths + + # ========================================================================= + # Safe execution — PathMapper null byte rejection in mapping methods + # ========================================================================= + + Scenario: PathMapper host_to_container rejects null bytes + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I try to map a host path with null bytes to container + Then a mapping ValueError should be raised about null bytes + + Scenario: PathMapper container_to_host rejects null bytes + Given I have a PathMapper with host_root "/tmp/sandbox" and container_root "/workspace" + When I try to map a container path with null bytes to host + Then a mapping ValueError should be raised about null bytes + + # ========================================================================= + # ToolResult validation — success/error consistency + # ========================================================================= + + Scenario: ToolResult rejects success=True with error message + When I try to create a ToolResult with success true and an error + Then a ToolResult validation error should be raised + + Scenario: ToolResult rejects success=False without error message + When I try to create a ToolResult with success false and no error + Then a ToolResult validation error should be raised + + # ========================================================================= + # ToolError structured attributes + # ========================================================================= + + Scenario: ToolError carries structured context attributes + When I create a ToolError with known attributes + Then the ToolError should expose tool_name, error_type, and details diff --git a/features/steps/changeset_capture_steps.py b/features/steps/changeset_capture_steps.py index dc4ea2ed7..b948812ee 100644 --- a/features/steps/changeset_capture_steps.py +++ b/features/steps/changeset_capture_steps.py @@ -280,8 +280,12 @@ def step_start_changeset(context, plan_id): @when("I record a create entry in the changeset") def step_record_create_entry(context): """Record a create entry.""" + # Use the plan_id from the changeset so the entry matches + # (InMemoryChangeSetStore.record now validates via add_change). + cs = context.store.get(context.last_cs_id) + plan_id = cs.plan_id if cs is not None else "plan-abc" entry = ChangeEntry( - plan_id="plan-abc", + plan_id=plan_id, resource_id="res-1", tool_name="builtin/file-write", operation=ChangeOperation.CREATE, diff --git a/features/steps/container_tool_exec_steps.py b/features/steps/container_tool_exec_steps.py new file mode 100644 index 000000000..69bda995d --- /dev/null +++ b/features/steps/container_tool_exec_steps.py @@ -0,0 +1,1785 @@ +"""Behave step definitions for container tool execution (issue #515).""" + +from __future__ import annotations + +import io +import json +import shutil +import tempfile +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from behave import given, then, when # type: ignore[import-untyped] + +from cleveragents.application.services.execution_environment_resolver import ( + ContainerUnavailableError, + ExecutionEnvironmentResolver, +) +from cleveragents.domain.models.core.change import ToolInvocation +from cleveragents.domain.models.core.plan import ExecutionEnvironment +from cleveragents.tool.container_executor import ( + _MAX_OUTPUT_BYTES, + _MAX_RECURSION_DEPTH, + ContainerConfig, + ContainerExecutionError, + ContainerMetadata, + ContainerTimeoutError, + ContainerToolExecutor, + _ExecResult, + _looks_like_path, +) +from cleveragents.tool.path_mapper import PathMapper +from cleveragents.tool.registry import ToolRegistry as _TR +from cleveragents.tool.runner import ToolRunner +from cleveragents.tool.runtime import ToolError, ToolResult, ToolSpec + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_FAKE_DEVCONTAINER_BIN = "/usr/local/bin/devcontainer" + + +def _set_fake_devcontainer_bin(executor: ContainerToolExecutor) -> None: + """Inject a fake devcontainer binary path for tests. + + In CI/test environments where the real ``devcontainer`` CLI is not + installed, the executor stores ``None`` and would fail fast. Test + scenarios that mock ``_run_command`` or ``subprocess.Popen`` never + invoke the real binary, so a dummy path is sufficient. + """ + executor._devcontainer_bin = _FAKE_DEVCONTAINER_BIN # type: ignore[assignment] + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("I have the container tool execution module imported") +def step_have_container_exec_module(context: Any) -> None: + # Import check — will fail at parse time if modules are broken + from cleveragents.tool import container_executor, path_mapper # noqa: F401 + + context.imported = True + + +# --------------------------------------------------------------------------- +# PathMapper +# --------------------------------------------------------------------------- + + +@given( + 'I have a PathMapper with host_root "{host_root}" and container_root "{container_root}"' +) +def step_create_path_mapper(context: Any, host_root: str, container_root: str) -> None: + context.path_mapper = PathMapper(host_root=host_root, container_root=container_root) + + +@when('I map host path "{path}" to container') +def step_map_host_to_container(context: Any, path: str) -> None: + context.mapped_path = context.path_mapper.host_to_container(path) + + +@when('I map container path "{path}" to host') +def step_map_container_to_host(context: Any, path: str) -> None: + context.mapped_path = context.path_mapper.container_to_host(path) + + +@then('the container path should be "{expected}"') +def step_check_container_path(context: Any, expected: str) -> None: + assert context.mapped_path == expected, ( + f"Expected {expected!r}, got {context.mapped_path!r}" + ) + + +@then('the host path should be "{expected}"') +def step_check_host_path(context: Any, expected: str) -> None: + assert context.mapped_path == expected, ( + f"Expected {expected!r}, got {context.mapped_path!r}" + ) + + +@then('"{path}" should be a host path') +def step_is_host_path(context: Any, path: str) -> None: + assert context.path_mapper.is_host_path(path), f"{path} should be a host path" + + +@then('"{path}" should not be a host path') +def step_is_not_host_path(context: Any, path: str) -> None: + assert not context.path_mapper.is_host_path(path), ( + f"{path} should NOT be a host path" + ) + + +@then('"{path}" should be a container path') +def step_is_container_path(context: Any, path: str) -> None: + assert context.path_mapper.is_container_path(path), ( + f"{path} should be a container path" + ) + + +@then('"{path}" should not be a container path') +def step_is_not_container_path(context: Any, path: str) -> None: + assert not context.path_mapper.is_container_path(path), ( + f"{path} should NOT be a container path" + ) + + +# --------------------------------------------------------------------------- +# ContainerConfig +# --------------------------------------------------------------------------- + + +@when("I create a default ContainerConfig") +def step_create_default_config(context: Any) -> None: + context.container_config = ContainerConfig() + + +@then('the workspace_folder should be "{expected}"') +def step_check_workspace_folder(context: Any, expected: str) -> None: + assert context.container_config.workspace_folder == expected + + +@then("the container timeout_seconds should be {expected:d}") +def step_check_timeout_seconds(context: Any, expected: int) -> None: + assert context.container_config.timeout_seconds == expected + + +# --------------------------------------------------------------------------- +# ContainerMetadata +# --------------------------------------------------------------------------- + + +@when('I create a ContainerMetadata with container_id "{cid}" and image "{img}"') +def step_create_metadata(context: Any, cid: str, img: str) -> None: + context.container_metadata = ContainerMetadata(container_id=cid, image=img) + + +@then('the container_metadata container_id should be "{expected}"') +def step_check_metadata_cid(context: Any, expected: str) -> None: + assert context.container_metadata.container_id == expected + + +@then('the container_metadata image should be "{expected}"') +def step_check_metadata_image(context: Any, expected: str) -> None: + assert context.container_metadata.image == expected + + +@then("the container_metadata timed_out should be false") +def step_check_metadata_timed_out_false(context: Any) -> None: + assert context.container_metadata.timed_out is False + + +# --------------------------------------------------------------------------- +# ContainerToolExecutor instantiation +# --------------------------------------------------------------------------- + + +@given('I have a ContainerConfig with workspace "{ws}" and container_id "{cid}"') +def step_create_config_with_params(context: Any, ws: str, cid: str) -> None: + context.container_config = ContainerConfig(workspace_folder=ws, container_id=cid) + + +@when("I create a ContainerToolExecutor with that config") +def step_create_executor(context: Any) -> None: + context.executor = ContainerToolExecutor(context.container_config) + + +@then('the executor config container_id should be "{expected}"') +def step_check_executor_cid(context: Any, expected: str) -> None: + assert context.executor.config.container_id == expected + + +@then("the executor should have a path mapper") +def step_executor_has_path_mapper(context: Any) -> None: + assert context.executor.path_mapper is not None + + +# --------------------------------------------------------------------------- +# Input / output path mapping +# --------------------------------------------------------------------------- + + +@given('I have a ContainerToolExecutor with host_root "{hr}" and container_root "{cr}"') +def step_create_executor_with_roots(context: Any, hr: str, cr: str) -> None: + config = ContainerConfig( + workspace_folder=cr, + host_sandbox_path=hr, + container_id="test", + ) + context.executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(context.executor) + + +@when('I map tool inputs with path "{path}"') +def step_map_tool_inputs(context: Any, path: str) -> None: + inputs = {"path": path} + context.mapped_inputs = context.executor._map_input_paths(inputs) + + +@then('the mapped input path should be "{expected}"') +def step_check_mapped_input(context: Any, expected: str) -> None: + assert context.mapped_inputs["path"] == expected, ( + f"Expected {expected!r}, got {context.mapped_inputs['path']!r}" + ) + + +@when('I map tool output with path "{path}"') +def step_map_tool_output(context: Any, path: str) -> None: + output = {"path": path} + context.mapped_output = context.executor._map_output_paths(output) + + +@then('the mapped output path should be "{expected}"') +def step_check_mapped_output(context: Any, expected: str) -> None: + assert context.mapped_output["path"] == expected, ( + f"Expected {expected!r}, got {context.mapped_output['path']!r}" + ) + + +@when('I map tool inputs with nested paths under "{root}"') +def step_map_nested_inputs(context: Any, root: str) -> None: + inputs = { + "file": f"{root}/a.py", + "config": {"paths": [f"{root}/b.py", f"{root}/c.py"]}, + "other": 42, + } + context.mapped_inputs = context.executor._map_input_paths(inputs) + + +@then("all nested paths should be mapped to container paths") +def step_check_nested_mapped(context: Any) -> None: + assert context.mapped_inputs["file"].startswith("/workspace"), ( + f"Expected /workspace, got {context.mapped_inputs['file']}" + ) + # Nested dicts are also mapped + inner = context.mapped_inputs["config"] + for p in inner["paths"]: + assert p.startswith("/workspace"), f"Expected /workspace, got {p}" + + +# --------------------------------------------------------------------------- +# Command building (S12 — flag/value adjacency) +# --------------------------------------------------------------------------- + + +@when('I build an exec command for tool "{tool_name}" with timeout {timeout:d}') +def step_build_exec_command(context: Any, tool_name: str, timeout: int) -> None: + cmd, stdin_data = context.executor._build_exec_command( + tool_name, {"key": "value"}, timeout + ) + context.built_command = cmd + context.built_stdin = stdin_data + + +@then('the command should have "--container-id" adjacent to its value') +def step_check_flag_adjacency(context: Any) -> None: + cmd = context.built_command + cid = context.executor.config.container_id + # Verify --container-id and its value are adjacent in the command list + assert "--container-id" in cmd, f"Expected '--container-id' in command: {cmd}" + idx = cmd.index("--container-id") + assert idx + 1 < len(cmd), "'--container-id' is the last element, no value follows" + assert cmd[idx + 1] == cid, ( + f"Expected '{cid}' immediately after '--container-id', got '{cmd[idx + 1]}'" + ) + + +# --------------------------------------------------------------------------- +# Timeout handling (mock) +# --------------------------------------------------------------------------- + + +@given("I have a ContainerToolExecutor with a mock that times out") +def step_executor_mock_timeout(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="timeout-ctr", + host_sandbox_path="/tmp/sandbox", + timeout_seconds=5, + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + # Patch _run_command to simulate timeout + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="", + stderr="timed out after 5s", + exit_code=-1, + duration_ms=5000.0, + timed_out=True, + ) + ) + context.executor = executor + + +@when('I execute tool "{tool_name}" with inputs in the container') +def step_execute_tool_in_container(context: Any, tool_name: str) -> None: + context.tool_result = context.executor.execute_tool(tool_name, {"key": "value"}) + + +@then("the container tool result should indicate failure") +def step_check_container_tool_result_failure(context: Any) -> None: + assert context.tool_result.success is False + + +@then('the container tool result error should mention "{text}"') +def step_check_container_tool_result_error_text(context: Any, text: str) -> None: + assert text in context.tool_result.error, ( + f"Expected {text!r} in {context.tool_result.error!r}" + ) + + +@then("the tool result metadata should contain container info with timed_out true") +def step_check_metadata_timed_out_true(context: Any) -> None: + meta = context.tool_result.metadata.get("container", {}) + assert meta.get("timed_out") is True, ( + f"Expected timed_out=True in metadata.container, got {meta}" + ) + + +# --------------------------------------------------------------------------- +# Error reporting (mock) +# --------------------------------------------------------------------------- + + +@given("I have a ContainerToolExecutor with a mock that returns exit code {code:d}") +def step_executor_mock_exit_code(context: Any, code: int) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="fail-ctr", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="", + stderr="something went wrong", + exit_code=code, + duration_ms=100.0, + timed_out=False, + ) + ) + context.executor = executor + + +@given("I have a ContainerToolExecutor with subprocess raising OSError") +def step_executor_mock_oserror(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="os-err-ctr", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + # Patch subprocess.Popen to raise a real OSError so the except OSError + # branch in _run_command is actually exercised (S10). + patcher = patch( + "cleveragents.tool.container_executor.subprocess.Popen", + side_effect=OSError("No such file or directory: 'devcontainer'"), + ) + patcher.start() + context.executor = executor + context.oserror_patcher = patcher + # Register cleanup so the patcher is stopped even if the scenario + # aborts before the "When" step runs (P2-18). + context.add_cleanup(patcher.stop) + + +# --------------------------------------------------------------------------- +# Successful execution (mock) +# --------------------------------------------------------------------------- + + +@given("I have a ContainerToolExecutor with a mock that returns JSON output") +def step_executor_mock_json(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="json-ctr", + image="node:20", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout=json.dumps({"lint_errors": 0, "files_checked": 5}), + stderr="", + exit_code=0, + duration_ms=250.0, + timed_out=False, + ) + ) + context.executor = executor + + +@then("the container tool result should indicate success") +def step_check_container_tool_result_success(context: Any) -> None: + assert context.tool_result.success is True + + +@then("the tool result output should contain the parsed JSON") +def step_check_tool_result_json(context: Any) -> None: + assert "lint_errors" in context.tool_result.output + + +@then("the tool result metadata should contain container info") +def step_check_tool_result_has_metadata(context: Any) -> None: + assert "container" in context.tool_result.metadata, ( + f"Expected 'container' in metadata, got keys: " + f"{list(context.tool_result.metadata.keys())}" + ) + + +@given("I have a ContainerToolExecutor with a mock that returns plain text") +def step_executor_mock_plaintext(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="text-ctr", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="Hello from container!", + stderr="", + exit_code=0, + duration_ms=50.0, + timed_out=False, + ) + ) + context.executor = executor + + +@then("the tool result output should contain raw_output") +def step_check_raw_output(context: Any) -> None: + assert "raw_output" in context.tool_result.output + + +# --------------------------------------------------------------------------- +# ToolRunner container routing +# --------------------------------------------------------------------------- + + +@given("I have a ToolRunner with a ContainerToolExecutor mock") +def step_runner_with_container_executor(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = ToolSpec( + name="test_tool", + description="A test tool", + input_schema={}, + handler=lambda _: {"ok": True}, + ) + registry.get.return_value = spec + registry.list_tools.return_value = [spec] + + # Mock resolver to return CONTAINER + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + + # Mock container executor + mock_executor = MagicMock(spec=ContainerToolExecutor) + mock_executor.execute_tool.return_value = ToolResult( + success=True, + output={"result": "container_ok"}, + duration_ms=100.0, + ) + + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + container_executor=mock_executor, + ) + context.mock_container_executor = mock_executor + + +@when("I execute a tool with container execution environment") +def step_execute_tool_container_env(context: Any) -> None: + context.tool_result = context.runner.execute( + "test_tool", + {"arg": "val"}, + tool_env="container", + linked_resource_types=["devcontainer-instance"], + ) + + +@then("the ContainerToolExecutor should have been called") +def step_check_container_executor_called(context: Any) -> None: + context.mock_container_executor.execute_tool.assert_called_once() + + +@then("the resolver should have been called with the correct arguments") +def step_check_resolver_args(context: Any) -> None: + resolver = context.runner._env_resolver + resolver.resolve_and_validate.assert_called_once() + call_kwargs = resolver.resolve_and_validate.call_args + # Verify the resolver received the expected arguments (S13) + assert call_kwargs.kwargs.get("tool_env") == "container" or ( + call_kwargs[1].get("tool_env") == "container" + ), f"Expected tool_env='container' in resolver call, got {call_kwargs}" + resource_types = call_kwargs.kwargs.get("linked_resource_types") or call_kwargs[ + 1 + ].get("linked_resource_types") + assert "devcontainer-instance" in resource_types, ( + f"Expected 'devcontainer-instance' in linked_resource_types, " + f"got {resource_types}" + ) + + +@given("I have a ToolRunner without a ContainerToolExecutor") +def step_runner_without_container_executor(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = ToolSpec( + name="test_tool", + description="A test tool", + input_schema={}, + handler=lambda _: {"ok": True}, + ) + registry.get.return_value = spec + + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + # No container_executor + ) + + +# --------------------------------------------------------------------------- +# ToolInvocation audit trail +# --------------------------------------------------------------------------- + + +@when("I create a ToolInvocation with container_metadata") +def step_create_invocation_with_metadata(context: Any) -> None: + context.invocation = ToolInvocation( + plan_id="01TESTPLANID000000000000000", + tool_name="ns/tool", + container_metadata={ + "container_id": "abc", + "image": "node:20", + "exec_time_ms": 150.0, + }, + ) + + +@then('the ToolInvocation container_metadata should contain "{key}"') +def step_check_invocation_metadata_key(context: Any, key: str) -> None: + assert key in context.invocation.container_metadata + + +@when("I create a ToolInvocation without container_metadata") +def step_create_invocation_without_metadata(context: Any) -> None: + context.invocation = ToolInvocation( + plan_id="01TESTPLANID000000000000000", + tool_name="ns/tool", + ) + + +@then("the ToolInvocation container_metadata should be None") +def step_check_invocation_metadata_none(context: Any) -> None: + assert context.invocation.container_metadata is None + + +# --------------------------------------------------------------------------- +# ContainerExecutionError / ContainerTimeoutError +# --------------------------------------------------------------------------- + + +@when('I raise a ContainerExecutionError with exit_code {code:d} and stderr "{stderr}"') +def step_raise_container_error(context: Any, code: int, stderr: str) -> None: + context.error = ContainerExecutionError("test error", exit_code=code, stderr=stderr) + + +@then("the error exit_code should be {code:d}") +def step_check_error_exit_code(context: Any, code: int) -> None: + assert context.error.exit_code == code + + +@then('the error stderr should be "{expected}"') +def step_check_error_stderr(context: Any, expected: str) -> None: + assert context.error.stderr == expected + + +@when("I raise a ContainerTimeoutError with timeout {timeout:d}") +def step_raise_timeout_error(context: Any, timeout: int) -> None: + context.error = ContainerTimeoutError(timeout_seconds=timeout) + + +@then('the timeout error message should mention "{text}"') +def step_check_timeout_msg(context: Any, text: str) -> None: + assert text in str(context.error) + + +@then("the timeout error timed_out should be true") +def step_check_timeout_timed_out(context: Any) -> None: + assert context.error.timed_out is True + + +# --------------------------------------------------------------------------- +# execute_tool input validation (P2-19) +# --------------------------------------------------------------------------- + + +@when("I execute tool with empty tool_name") +def step_execute_empty_tool_name(context: Any) -> None: + try: + context.executor.execute_tool("", {"key": "value"}) + context.error = None + except (ValueError, TypeError) as exc: + context.error = exc + + +@when("I execute tool with non-dict inputs") +def step_execute_non_dict_inputs(context: Any) -> None: + try: + context.executor.execute_tool("test_tool", "not a dict") # type: ignore[arg-type] + context.error = None + except (ValueError, TypeError) as exc: + context.error = exc + + +@when("I execute tool with negative timeout") +def step_execute_negative_timeout(context: Any) -> None: + try: + context.executor.execute_tool("test_tool", {}, timeout_seconds=-1) + context.error = None + except (ValueError, TypeError) as exc: + context.error = exc + + +@then('a container TypeError should be raised with message "{text}"') +def step_check_type_error(context: Any, text: str) -> None: + assert context.error is not None, "Expected a TypeError but none was raised" + assert isinstance(context.error, TypeError), ( + f"Expected TypeError, got {type(context.error).__name__}" + ) + assert text in str(context.error), ( + f"Expected '{text}' in error message, got: {context.error}" + ) + + +# --------------------------------------------------------------------------- +# extract_container_metadata helper (P1-9) +# --------------------------------------------------------------------------- + + +@when("I create a ToolResult with container metadata in metadata dict") +def step_create_tool_result_with_container_meta(context: Any) -> None: + context.tool_result_with_meta = ToolResult( + success=True, + output={"result": "ok"}, + duration_ms=100.0, + metadata={ + "container": { + "container_id": "abc123", + "image": "node:20", + "exec_time_ms": 150.0, + } + }, + ) + + +@then("extract_container_metadata should return the container dict") +def step_check_extract_container_metadata(context: Any) -> None: + extracted = ContainerToolExecutor.extract_container_metadata( + context.tool_result_with_meta + ) + assert extracted is not None, "Expected container metadata, got None" + assert extracted["container_id"] == "abc123" + assert extracted["image"] == "node:20" + + +# --------------------------------------------------------------------------- +# sync_results_to_host +# --------------------------------------------------------------------------- + + +@given( + "I have a ContainerToolExecutor with a mock that succeeds on sync using a temp dir" +) +def step_executor_mock_sync_success(context: Any) -> None: + # Use a real temp directory so the file-write in sync_results_to_host + # succeeds without polluting a fixed path. Explicitly use /tmp so + # the path never falls under the container_root "/workspace" — CI + # workspaces live at /workspace/… and PathMapper rejects overlapping + # host_root / container_root. + context.sync_tmp_dir = tempfile.mkdtemp(prefix="ca_sync_test_", dir="/tmp") + context.add_cleanup( + lambda d=context.sync_tmp_dir: shutil.rmtree(d, ignore_errors=True) + ) + config = ContainerConfig( + workspace_folder="/workspace", + container_id="sync-ctr", + host_sandbox_path=context.sync_tmp_dir, + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="file content", + stderr="", + raw_stdout=b"file content", + exit_code=0, + duration_ms=20.0, + timed_out=False, + ) + ) + context.executor = executor + + +@when('I sync container path "{path}" to host') +def step_sync_to_host(context: Any, path: str) -> None: + context.synced_path = context.executor.sync_results_to_host(path) + + +@then("the synced host path should be under the host sandbox root") +def step_check_synced_path(context: Any) -> None: + # Compare resolved paths so the assertion holds when /tmp is a + # symlink or bind-mount redirect (common in CI Docker containers). + resolved_sandbox = str(Path(context.sync_tmp_dir).resolve()) + resolved_synced = str(Path(context.synced_path).resolve()) + assert resolved_synced.startswith(resolved_sandbox), ( + f"Expected path under {resolved_sandbox}, got {resolved_synced}" + ) + + +# --------------------------------------------------------------------------- +# Signal exit code preservation (H5 fix) +# --------------------------------------------------------------------------- + + +@then("the tool result metadata should have exit_code {code:d}") +def step_check_metadata_exit_code(context: Any, code: int) -> None: + meta = context.tool_result.metadata.get("container", {}) + assert meta.get("exit_code") == code, ( + f"Expected exit_code={code} in metadata.container, got {meta.get('exit_code')}" + ) + + +# --------------------------------------------------------------------------- +# Output truncation (C2 fix) +# --------------------------------------------------------------------------- + + +@given("I have a ContainerToolExecutor with a mock that returns oversized output") +def step_executor_mock_oversized_output(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="big-ctr", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + context.max_output_bytes = _MAX_OUTPUT_BYTES + + # Mock subprocess.Popen to return oversized output, so that + # _run_command's bounded-read + truncation logic is exercised. + oversized_bytes = b"x" * (_MAX_OUTPUT_BYTES + 1024) + mock_proc = MagicMock() + mock_proc.stdout = io.BytesIO(oversized_bytes) + mock_proc.stderr = io.BytesIO(b"") + mock_proc.stdin = MagicMock() + mock_proc.returncode = 0 + mock_proc.wait = MagicMock(return_value=0) + patcher = patch( + "cleveragents.tool.container_executor.subprocess.Popen", + return_value=mock_proc, + ) + patcher.start() + context.oversized_patcher = patcher + context.executor = executor + # Register cleanup so the patcher is stopped even if the scenario + # aborts before the "When" step runs (P2-18). + context.add_cleanup(patcher.stop) + + +@then("the container tool result stdout should be truncated") +def step_check_output_truncated(context: Any) -> None: + # The output was parsed from stdout; verify the raw_output is within limit. + # Since the oversized output is not valid JSON, it ends up as raw_output. + raw = context.tool_result.output.get("raw_output", "") + max_bytes = context.max_output_bytes + assert len(raw.encode("utf-8")) <= max_bytes, ( + f"Expected output <= {max_bytes} bytes, got {len(raw.encode('utf-8'))}" + ) + + +# --------------------------------------------------------------------------- +# Path traversal protection (C3 fix) +# --------------------------------------------------------------------------- + + +@when("I attempt to sync a path that traverses outside the sandbox") +def step_sync_traversal_attempt(context: Any) -> None: + try: + # This path maps to a host path outside the sandbox due to ".." + context.executor.sync_results_to_host("/workspace/../../../etc/passwd") + context.traversal_error = None + except ContainerExecutionError as exc: + context.traversal_error = exc + + +@then("a ContainerExecutionError should be raised for path traversal") +def step_check_traversal_error(context: Any) -> None: + assert context.traversal_error is not None, ( + "Expected ContainerExecutionError for path traversal, but none was raised" + ) + assert "outside" in str(context.traversal_error).lower(), ( + f"Expected 'outside' in error message, got: {context.traversal_error}" + ) + + +# ========================================================================= +# PathMapper safe initialisation guards +# ========================================================================= + + +@when("I try to create a PathMapper with null bytes in host_root") +def step_pathmapper_null_host(context: Any) -> None: + try: + PathMapper(host_root="/tmp/sand\x00box", container_root="/workspace") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@when("I try to create a PathMapper with null bytes in container_root") +def step_pathmapper_null_container(context: Any) -> None: + try: + PathMapper(host_root="/tmp/sandbox", container_root="/work\x00space") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@when("I try to create a PathMapper with empty host_root") +def step_pathmapper_empty_host(context: Any) -> None: + try: + PathMapper(host_root="", container_root="/workspace") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@when('I try to create a PathMapper with host_root "/"') +def step_pathmapper_root_host(context: Any) -> None: + try: + PathMapper(host_root="/", container_root="/workspace") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@when('I try to create a PathMapper with container_root "/"') +def step_pathmapper_root_container(context: Any) -> None: + try: + PathMapper(host_root="/tmp/sandbox", container_root="/") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@when("I try to create a PathMapper with overlapping roots") +def step_pathmapper_overlapping(context: Any) -> None: + try: + PathMapper(host_root="/workspace/sub", container_root="/workspace") + context.pm_error = None + except ValueError as exc: + context.pm_error = exc + + +@then('a PathMapper ValueError should be raised mentioning "{text}"') +def step_check_pathmapper_error(context: Any, text: str) -> None: + assert context.pm_error is not None, ( + "Expected a ValueError from PathMapper but none was raised" + ) + assert text.lower() in str(context.pm_error).lower(), ( + f"Expected '{text}' in error: {context.pm_error}" + ) + + +@then("a path with null bytes should not be a host path") +def step_null_bytes_not_host(context: Any) -> None: + assert not context.path_mapper.is_host_path("/tmp/sandbox/\x00evil") + + +@then("a path with null bytes should not be a container path") +def step_null_bytes_not_container(context: Any) -> None: + assert not context.path_mapper.is_container_path("/workspace/\x00evil") + + +# ========================================================================= +# ContainerConfig safe initialisation guards +# ========================================================================= + + +@when('I try to create a ContainerConfig with workspace_folder "{folder}"') +def step_config_bad_workspace(context: Any, folder: str) -> None: + try: + ContainerConfig(workspace_folder=folder) + context.cfg_error = None + except ValueError as exc: + context.cfg_error = exc + + +@then('a ContainerConfig ValueError should be raised mentioning "{text}"') +def step_check_config_error(context: Any, text: str) -> None: + assert context.cfg_error is not None, ( + "Expected a ValueError from ContainerConfig but none was raised" + ) + assert text.lower() in str(context.cfg_error).lower(), ( + f"Expected '{text}' in error: {context.cfg_error}" + ) + + +# ========================================================================= +# ContainerToolExecutor binary resolution +# ========================================================================= + + +@given("I have a ContainerToolExecutor with no container_id configured") +def step_executor_no_cid(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + context.executor = executor + + +@when("I request the target CLI arguments") +def step_request_target_args(context: Any) -> None: + context.target_args = context.executor._devcontainer_target_args() + + +@then('the target args should use "--workspace-folder"') +def step_check_workspace_folder_args(context: Any) -> None: + assert "--workspace-folder" in context.target_args, ( + f"Expected '--workspace-folder' in {context.target_args}" + ) + + +@given("I have a ContainerToolExecutor with no devcontainer binary") +def step_executor_no_binary(context: Any) -> None: + config = ContainerConfig( + workspace_folder="/workspace", + container_id="test", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + # Ensure the binary is None + executor._devcontainer_bin = None + context.executor = executor + + +@when("I try to require the devcontainer binary") +def step_require_binary(context: Any) -> None: + try: + context.executor._require_devcontainer_bin() + context.binary_error = None + except ContainerExecutionError as exc: + context.binary_error = exc + + +@then("a ContainerExecutionError should be raised about missing binary") +def step_check_missing_binary_error(context: Any) -> None: + assert context.binary_error is not None, ( + "Expected ContainerExecutionError for missing binary" + ) + assert "not found" in str(context.binary_error).lower(), ( + f"Expected 'not found' in error: {context.binary_error}" + ) + + +# ========================================================================= +# sync_results_to_host cleanup error paths +# ========================================================================= + + +@when("I try to sync a path containing null bytes") +def step_sync_null_bytes(context: Any) -> None: + try: + context.executor.sync_results_to_host("/workspace/\x00evil.txt") + context.sync_error = None + except ValueError as exc: + context.sync_error = exc + + +@then('a sync ValueError should be raised mentioning "{text}"') +def step_check_sync_value_error(context: Any, text: str) -> None: + assert context.sync_error is not None, ( + "Expected a ValueError from sync but none was raised" + ) + assert text.lower() in str(context.sync_error).lower(), ( + f"Expected '{text}' in error: {context.sync_error}" + ) + + +@when("I try to sync a relative container path") +def step_sync_relative_path(context: Any) -> None: + try: + context.executor.sync_results_to_host("relative/path.txt") + context.sync_error = None + except ValueError as exc: + context.sync_error = exc + + +@given("I have a ContainerToolExecutor with a mock that times out on sync") +def step_executor_sync_timeout(context: Any) -> None: + context.sync_tmp_dir = tempfile.mkdtemp(prefix="ca_sync_timeout_", dir="/tmp") + context.add_cleanup( + lambda d=context.sync_tmp_dir: shutil.rmtree(d, ignore_errors=True) + ) + config = ContainerConfig( + workspace_folder="/workspace", + container_id="sync-timeout-ctr", + host_sandbox_path=context.sync_tmp_dir, + timeout_seconds=5, + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="", + stderr="timed out", + raw_stdout=b"", + exit_code=-1, + duration_ms=5000.0, + timed_out=True, + ) + ) + context.executor = executor + + +@when("I try to sync container path to host with timeout") +def step_sync_with_timeout(context: Any) -> None: + try: + context.executor.sync_results_to_host("/workspace/output.txt") + context.sync_timeout_error = None + except ContainerTimeoutError as exc: + context.sync_timeout_error = exc + + +@then("a ContainerTimeoutError should be raised from sync") +def step_check_sync_timeout(context: Any) -> None: + assert context.sync_timeout_error is not None, ( + "Expected ContainerTimeoutError from sync" + ) + assert context.sync_timeout_error.timed_out is True + + +@given("I have a ContainerToolExecutor with a mock that returns non-zero exit on sync") +def step_executor_sync_fail(context: Any) -> None: + context.sync_tmp_dir = tempfile.mkdtemp(prefix="ca_sync_fail_", dir="/tmp") + context.add_cleanup( + lambda d=context.sync_tmp_dir: shutil.rmtree(d, ignore_errors=True) + ) + config = ContainerConfig( + workspace_folder="/workspace", + container_id="sync-fail-ctr", + host_sandbox_path=context.sync_tmp_dir, + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="", + stderr="cat: no such file", + raw_stdout=b"", + exit_code=1, + duration_ms=10.0, + timed_out=False, + ) + ) + context.executor = executor + + +@when("I try to sync container path to host with failure") +def step_sync_with_failure(context: Any) -> None: + try: + context.executor.sync_results_to_host("/workspace/missing.txt") + context.sync_exec_error = None + except ContainerExecutionError as exc: + context.sync_exec_error = exc + + +@then("a ContainerExecutionError should be raised from sync failure") +def step_check_sync_exec_error(context: Any) -> None: + assert context.sync_exec_error is not None, ( + "Expected ContainerExecutionError from sync failure" + ) + assert context.sync_exec_error.exit_code == 1 + + +@given("I have a ContainerToolExecutor with a mock that triggers OSError on write") +def step_executor_sync_oserror(context: Any) -> None: + context.sync_tmp_dir = tempfile.mkdtemp(prefix="ca_sync_oserr_", dir="/tmp") + context.add_cleanup( + lambda d=context.sync_tmp_dir: shutil.rmtree(d, ignore_errors=True) + ) + config = ContainerConfig( + workspace_folder="/workspace", + container_id="sync-oserr-ctr", + host_sandbox_path=context.sync_tmp_dir, + ) + executor = ContainerToolExecutor(config) + _set_fake_devcontainer_bin(executor) + executor._run_command = MagicMock( + return_value=_ExecResult( + stdout="content", + stderr="", + raw_stdout=b"content", + exit_code=0, + duration_ms=10.0, + timed_out=False, + ) + ) + context.executor = executor + + +@when("I try to sync container path to host triggering write error") +def step_sync_trigger_write_error(context: Any) -> None: + # Patch os.open to simulate a permission error during file write + patcher = patch( + "cleveragents.tool.container_executor.os.open", + side_effect=OSError("Permission denied"), + ) + patcher.start() + context.add_cleanup(patcher.stop) + try: + context.executor.sync_results_to_host("/workspace/output.txt") + context.sync_write_error = None + except ContainerExecutionError as exc: + context.sync_write_error = exc + + +@then("a ContainerExecutionError should be raised from write failure") +def step_check_sync_write_error(context: Any) -> None: + assert context.sync_write_error is not None, ( + "Expected ContainerExecutionError from write failure" + ) + assert ( + "Permission denied" in str(context.sync_write_error) + or "write" in str(context.sync_write_error).lower() + ), f"Unexpected error: {context.sync_write_error}" + + +# ========================================================================= +# _looks_like_path heuristic boundary checks +# ========================================================================= + + +@when("I check whether a string with newlines looks like a path") +def step_looks_like_path_newline(context: Any) -> None: + context.looks_like = _looks_like_path("/some/path\nwith newline") + + +@when("I check whether a string with null bytes looks like a path") +def step_looks_like_path_null(context: Any) -> None: + context.looks_like = _looks_like_path("/some/\x00path") + + +@when("I check whether a protocol-relative URI looks like a path") +def step_looks_like_path_proto_relative(context: Any) -> None: + context.looks_like = _looks_like_path("//cdn.example.com/resource") + + +@when("I check whether a path with query string looks like a path") +def step_looks_like_path_query(context: Any) -> None: + context.looks_like = _looks_like_path("/api/v1/resource?key=value") + + +@when("I check whether a path with fragment identifier looks like a path") +def step_looks_like_path_fragment(context: Any) -> None: + context.looks_like = _looks_like_path("/docs/page#section") + + +@when("I check whether a URL-like string looks like a path") +def step_looks_like_path_url(context: Any) -> None: + context.looks_like = _looks_like_path("/prefix://host/path") + + +@then("it should not look like a path") +def step_check_not_looks_like_path(context: Any) -> None: + assert context.looks_like is False, "Expected False from _looks_like_path" + + +# ========================================================================= +# Recursion depth guards on path mapping +# ========================================================================= + + +@when("I map deeply nested input paths exceeding the recursion limit") +def step_deep_input_mapping(context: Any) -> None: + # Build a dict nested beyond _MAX_RECURSION_DEPTH + nested: dict[str, Any] = {"path": "/tmp/sandbox/file.py"} + current = nested + for i in range(_MAX_RECURSION_DEPTH + 2): + current["inner"] = {"path": f"/tmp/sandbox/f{i}.py"} + current = current["inner"] + context.deep_mapped = context.executor._map_input_paths(nested) + + +@then("the input mapping should return without error") +def step_check_deep_input_ok(context: Any) -> None: + assert context.deep_mapped is not None + + +@when("I map deeply nested output paths exceeding the recursion limit") +def step_deep_output_mapping(context: Any) -> None: + nested: dict[str, Any] = {"path": "/workspace/file.py"} + current = nested + for i in range(_MAX_RECURSION_DEPTH + 2): + current["inner"] = {"path": f"/workspace/f{i}.py"} + current = current["inner"] + context.deep_mapped = context.executor._map_output_paths(nested) + + +@then("the output mapping should return without error") +def step_check_deep_output_ok(context: Any) -> None: + assert context.deep_mapped is not None + + +# ========================================================================= +# _parse_output edge cases +# ========================================================================= + + +@when("I parse stdout containing a JSON array") +def step_parse_json_array(context: Any) -> None: + context.parsed_output = ContainerToolExecutor._parse_output("[1, 2, 3]") + + +@then('the parsed output should wrap it under "result" key') +def step_check_result_key(context: Any) -> None: + assert "result" in context.parsed_output, ( + f"Expected 'result' key, got {context.parsed_output}" + ) + assert context.parsed_output["result"] == [1, 2, 3] + + +@when("I parse empty stdout") +def step_parse_empty(context: Any) -> None: + context.parsed_output = ContainerToolExecutor._parse_output(" ") + + +@then("the parsed output should be an empty dict") +def step_check_empty_parsed(context: Any) -> None: + assert context.parsed_output == {} + + +# ========================================================================= +# _map_output_paths raw_output preservation +# ========================================================================= + + +@when("I map output containing a raw_output key with container path") +def step_map_raw_output(context: Any) -> None: + output = { + "raw_output": "/workspace/data/results.txt\nmore lines here", + "path": "/workspace/file.py", + } + context.mapped_output = context.executor._map_output_paths(output) + + +@then("the raw_output value should remain unmapped") +def step_check_raw_output_unmapped(context: Any) -> None: + raw = context.mapped_output["raw_output"] + assert raw.startswith("/workspace"), f"raw_output should stay unmapped, got: {raw}" + # But the path key should be mapped to host + assert context.mapped_output["path"].startswith("/tmp/sandbox"), ( + f"path key should be mapped, got: {context.mapped_output['path']}" + ) + + +# ========================================================================= +# execute_tool with non-serialisable inputs +# ========================================================================= + + +# ========================================================================= +# List-type value mapping branches +# ========================================================================= + + +@when("I map tool inputs containing a list of host paths") +def step_map_list_inputs(context: Any) -> None: + inputs = {"paths": ["/tmp/sandbox/a.py", "/tmp/sandbox/b.py", "/usr/lib/x.so"]} + context.mapped_inputs = context.executor._map_input_paths(inputs) + + +@then("the list values should be mapped to container paths") +def step_check_list_inputs_mapped(context: Any) -> None: + mapped = context.mapped_inputs["paths"] + assert mapped[0] == "/workspace/a.py", f"Expected /workspace/a.py, got {mapped[0]}" + assert mapped[1] == "/workspace/b.py", f"Expected /workspace/b.py, got {mapped[1]}" + # Path outside host_root should remain unchanged + assert mapped[2] == "/usr/lib/x.so", f"Expected /usr/lib/x.so, got {mapped[2]}" + + +@when("I map tool output containing a list of container paths") +def step_map_list_outputs(context: Any) -> None: + output = {"paths": ["/workspace/out1.txt", "/workspace/out2.txt"]} + context.mapped_output = context.executor._map_output_paths(output) + + +@then("the list values should be mapped to host paths") +def step_check_list_outputs_mapped(context: Any) -> None: + mapped = context.mapped_output["paths"] + assert mapped[0] == "/tmp/sandbox/out1.txt", ( + f"Expected /tmp/sandbox/out1.txt, got {mapped[0]}" + ) + assert mapped[1] == "/tmp/sandbox/out2.txt", ( + f"Expected /tmp/sandbox/out2.txt, got {mapped[1]}" + ) + + +# ========================================================================= +# PathMapper null byte rejection in mapping methods +# ========================================================================= + + +@when("I try to map a host path with null bytes to container") +def step_map_host_null(context: Any) -> None: + try: + context.path_mapper.host_to_container("/tmp/sandbox/\x00evil") + context.mapping_error = None + except ValueError as exc: + context.mapping_error = exc + + +@when("I try to map a container path with null bytes to host") +def step_map_container_null(context: Any) -> None: + try: + context.path_mapper.container_to_host("/workspace/\x00evil") + context.mapping_error = None + except ValueError as exc: + context.mapping_error = exc + + +@then("a mapping ValueError should be raised about null bytes") +def step_check_mapping_null_error(context: Any) -> None: + assert context.mapping_error is not None, ( + "Expected ValueError for null bytes in path mapping" + ) + assert "null" in str(context.mapping_error).lower(), ( + f"Expected 'null' in error: {context.mapping_error}" + ) + + +@when("I execute a tool with non-serialisable inputs") +def step_execute_non_serialisable(context: Any) -> None: + context.tool_result = context.executor.execute_tool("some_tool", {"bad": object()}) + + +# ========================================================================= +# ToolRunner full lifecycle +# ========================================================================= + + +def _make_dummy_spec(name: str = "ns/test-tool") -> ToolSpec: + """Create a minimal ToolSpec for lifecycle tests.""" + return ToolSpec( + name=name, + description="A test tool for lifecycle validation", + input_schema={}, + handler=lambda inputs: {"status": "ok"}, + ) + + +@given("I have a ToolRunner with a populated registry") +def step_runner_populated_registry(context: Any) -> None: + registry = _TR() + spec = _make_dummy_spec() + registry.register(spec) + context.runner = ToolRunner(registry=registry) + context.test_tool_name = spec.name + + +@when("I call discover on the runner") +def step_call_discover(context: Any) -> None: + context.discovered = context.runner.discover() + + +@then("the discovered tools list should not be empty") +def step_check_discovered(context: Any) -> None: + assert len(context.discovered) > 0 + + +@when("I activate a tool on the runner") +def step_activate_tool(context: Any) -> None: + context.activated_spec = context.runner.activate(context.test_tool_name) + + +@then("the activated tool spec should be returned") +def step_check_activated(context: Any) -> None: + assert context.activated_spec is not None + assert context.activated_spec.name == context.test_tool_name + + +@when("I try to activate an unknown tool") +def step_activate_unknown(context: Any) -> None: + try: + context.runner.activate("ns/nonexistent") + context.activation_error = None + except ToolError as exc: + context.activation_error = exc + + +@then("a ToolError should be raised for activation") +def step_check_activation_error(context: Any) -> None: + assert context.activation_error is not None, ( + "Expected ToolError for unknown tool activation" + ) + assert "not found" in context.activation_error.details.lower() + + +@when("I activate and then deactivate a tool") +def step_activate_deactivate(context: Any) -> None: + context.runner.activate(context.test_tool_name) + context.deactivate_result = context.runner.deactivate(context.test_tool_name) + + +@then("deactivate should return true") +def step_check_deactivate_true(context: Any) -> None: + assert context.deactivate_result is True + + +@then("a second deactivate should return false") +def step_check_second_deactivate(context: Any) -> None: + result = context.runner.deactivate(context.test_tool_name) + assert result is False + + +# ========================================================================= +# ToolRunner host execution path +# ========================================================================= + + +@given("I have a ToolRunner with a host-routed tool") +def step_runner_host_tool(context: Any) -> None: + registry = _TR() + spec = ToolSpec( + name="ns/host-tool", + description="Host-routed tool for testing", + input_schema={}, + handler=lambda inputs: {"data": "from_handler", "echo": inputs.get("key")}, + ) + registry.register(spec) + context.runner = ToolRunner(registry=registry) + context.host_tool_name = "ns/host-tool" + + +@when("I execute the host tool with valid inputs") +def step_execute_host_tool(context: Any) -> None: + context.tool_result = context.runner.execute( + context.host_tool_name, {"key": "value"} + ) + + +@then("the host tool result should indicate success") +def step_check_host_success(context: Any) -> None: + assert context.tool_result.success is True + + +@then("the host tool result output should contain handler data") +def step_check_host_output(context: Any) -> None: + assert context.tool_result.output.get("data") == "from_handler" + + +@given("I have a ToolRunner with a tool returning non-dict output") +def step_runner_nondict_output(context: Any) -> None: + registry = _TR() + spec = ToolSpec( + name="ns/nondict-tool", + description="Returns a string", + input_schema={}, + handler=lambda inputs: "plain_string_result", + ) + registry.register(spec) + context.runner = ToolRunner(registry=registry) + + +@when("I execute the non-dict-output tool") +def step_execute_nondict(context: Any) -> None: + context.tool_result = context.runner.execute("ns/nondict-tool", {}) + + +@then('the result output should wrap the value under "result"') +def step_check_wrapped_result(context: Any) -> None: + assert context.tool_result.success is True + assert context.tool_result.output.get("result") == "plain_string_result" + + +@given("I have a ToolRunner with a tool that raises an exception") +def step_runner_raising_tool(context: Any) -> None: + registry = _TR() + + def _bomb(inputs: dict) -> dict: + raise RuntimeError("handler exploded") + + spec = ToolSpec( + name="ns/bomb-tool", + description="Always raises", + input_schema={}, + handler=_bomb, + ) + registry.register(spec) + context.runner = ToolRunner(registry=registry) + + +@when("I execute the raising tool") +def step_execute_raising(context: Any) -> None: + context.tool_result = context.runner.execute("ns/bomb-tool", {}) + + +@then("the tool result should indicate failure with exception info") +def step_check_failure_with_exc(context: Any) -> None: + assert context.tool_result.success is False + assert "RuntimeError" in context.tool_result.error + assert "handler exploded" in context.tool_result.error + + +@given("I have a ToolRunner with a tool returning non-serialisable output") +def step_runner_nonserial_output(context: Any) -> None: + registry = _TR() + spec = ToolSpec( + name="ns/nonserial-tool", + description="Returns non-serialisable data", + input_schema={}, + handler=lambda inputs: {"bad_value": object()}, + ) + registry.register(spec) + context.runner = ToolRunner(registry=registry) + + +@when("I execute the non-serialisable-output tool") +def step_execute_nonserial(context: Any) -> None: + context.tool_result = context.runner.execute("ns/nonserial-tool", {}) + + +@then('the tool result should indicate failure mentioning "{text}"') +def step_check_failure_mentioning(context: Any, text: str) -> None: + assert context.tool_result.success is False + assert text in context.tool_result.error, ( + f"Expected '{text}' in error: {context.tool_result.error}" + ) + + +@when("I execute the host tool with non-serialisable inputs") +def step_execute_host_nonserial_inputs(context: Any) -> None: + context.tool_result = context.runner.execute( + context.host_tool_name, {"bad": object()} + ) + + +@given("I have a ToolRunner with an empty registry") +def step_runner_empty_registry(context: Any) -> None: + registry = _TR() + context.runner = ToolRunner(registry=registry) + + +@when("I try to execute an unregistered tool") +def step_execute_unregistered(context: Any) -> None: + try: + context.runner.execute("ns/ghost", {}) + context.exec_error = None + except ToolError as exc: + context.exec_error = exc + + +@then("a ToolError should be raised for execution") +def step_check_exec_tool_error(context: Any) -> None: + assert context.exec_error is not None, ( + "Expected ToolError for unregistered tool execution" + ) + assert "not found" in context.exec_error.details.lower() + + +# ========================================================================= +# ToolRunner container routing — additional edge cases +# ========================================================================= + + +@given("I have a ToolRunner with an unavailable-container resolver") +def step_runner_unavailable_container(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = _make_dummy_spec("ns/ctr-tool") + registry.get.return_value = spec + + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.side_effect = ContainerUnavailableError( + "No container resource linked" + ) + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + ) + + +@when("I execute a tool through the unavailable-container runner") +def step_execute_unavailable_ctr(context: Any) -> None: + context.tool_result = context.runner.execute("ns/ctr-tool", {}) + + +@then("the tool result should indicate container unavailable") +def step_check_container_unavailable(context: Any) -> None: + assert context.tool_result.success is False + assert "No container resource linked" in context.tool_result.error + + +@given("I have a ToolRunner with a container executor and required-field tool") +def step_runner_required_fields(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = ToolSpec( + name="ns/strict-tool", + description="Tool with required fields", + input_schema={ + "type": "object", + "required": ["file_path", "mode"], + "properties": { + "file_path": {"type": "string"}, + "mode": {"type": "string"}, + }, + }, + handler=lambda inputs: {"ok": True}, + ) + registry.get.return_value = spec + + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + + mock_executor = MagicMock(spec=ContainerToolExecutor) + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + container_executor=mock_executor, + ) + + +@when("I execute the container tool with missing required fields") +def step_execute_missing_fields(context: Any) -> None: + # Only provide file_path but not mode + context.tool_result = context.runner.execute( + "ns/strict-tool", {"file_path": "/tmp/file.py"} + ) + + +@given("I have a ToolRunner with a container executor for JSON validation") +def step_runner_ctr_json_validation(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = ToolSpec( + name="ns/json-tool", + description="Tool for JSON validation test", + input_schema={}, + handler=lambda inputs: {"ok": True}, + ) + registry.get.return_value = spec + + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + + mock_executor = MagicMock(spec=ContainerToolExecutor) + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + container_executor=mock_executor, + ) + + +@when("I execute a container tool with non-serialisable inputs") +def step_execute_ctr_nonserial(context: Any) -> None: + context.tool_result = context.runner.execute("ns/json-tool", {"bad": object()}) + + +@given("I have a ToolRunner with a failing container executor") +def step_runner_failing_ctr_executor(context: Any) -> None: + registry = MagicMock(spec=_TR) + spec = ToolSpec( + name="ns/fail-tool", + description="Tool that triggers executor failure", + input_schema={}, + handler=lambda inputs: {"ok": True}, + ) + registry.get.return_value = spec + + resolver = MagicMock(spec=ExecutionEnvironmentResolver) + resolver.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + + mock_executor = MagicMock(spec=ContainerToolExecutor) + mock_executor.execute_tool.side_effect = ContainerExecutionError( + "Container crashed", exit_code=-1 + ) + context.runner = ToolRunner( + registry=registry, + execution_environment_resolver=resolver, + container_executor=mock_executor, + ) + + +@when("I execute a tool through the failing container runner") +def step_execute_failing_ctr(context: Any) -> None: + context.tool_result = context.runner.execute("ns/fail-tool", {}) + + +# ========================================================================= +# ToolResult validation — success/error consistency +# ========================================================================= + + +@when("I try to create a ToolResult with success true and an error") +def step_toolresult_success_with_error(context: Any) -> None: + try: + ToolResult(success=True, output={}, error="should not be here") + context.tr_error = None + except ValueError as exc: + context.tr_error = exc + + +@when("I try to create a ToolResult with success false and no error") +def step_toolresult_fail_without_error(context: Any) -> None: + try: + ToolResult(success=False, output={}) + context.tr_error = None + except ValueError as exc: + context.tr_error = exc + + +@then("a ToolResult validation error should be raised") +def step_check_toolresult_error(context: Any) -> None: + assert context.tr_error is not None, ( + "Expected ValueError from ToolResult validation" + ) + + +# ========================================================================= +# ToolError structured attributes +# ========================================================================= + + +@when("I create a ToolError with known attributes") +def step_create_tool_error(context: Any) -> None: + context.tool_error = ToolError( + tool_name="ns/broken", + error_type="ExecutionError", + details="Something went wrong", + ) + + +@then("the ToolError should expose tool_name, error_type, and details") +def step_check_tool_error_attrs(context: Any) -> None: + err = context.tool_error + assert err.tool_name == "ns/broken" + assert err.error_type == "ExecutionError" + assert err.details == "Something went wrong" + assert "ExecutionError" in str(err) + assert "ns/broken" in str(err) diff --git a/features/steps/devcontainer_handler_steps.py b/features/steps/devcontainer_handler_steps.py index 33d119b80..718627fab 100644 --- a/features/steps/devcontainer_handler_steps.py +++ b/features/steps/devcontainer_handler_steps.py @@ -104,14 +104,30 @@ def step_builtin_registry(context: Context) -> None: @when("I create a devcontainer-instance resource at that path") def step_create_dc_resource(context: Context) -> None: + # Verify the type is actually recognised by the built-in registry + # instead of just assigning a string and asserting it back (U7). context.resource_type = "devcontainer-instance" - context.resource_id = "01TESTDEVCONTAINER00000000" + assert context.resource_type in ResourceTypeSpec.BUILTIN_NAMES, ( + f"'{context.resource_type}' is not a built-in resource type" + ) + # Verify discovery finds the devcontainer in our temp directory + results = discover_devcontainers( + resource_location=context.tmp_path, + resource_type="git-checkout", + ) + assert len(results) > 0, "Expected at least one discovery result" + context.resource_id = f"01TEST{results[0].config_path.name}" context.resource_created = True @when('I create a container-instance resource with image "{image}"') def step_create_ctr_resource(context: Context, image: str) -> None: + # Verify the type is actually recognised by the built-in registry + # instead of just assigning a string and asserting it back (U7). context.container_type = "container-instance" + assert context.container_type in ResourceTypeSpec.BUILTIN_NAMES, ( + f"'{context.container_type}' is not a built-in resource type" + ) context.container_image = image context.container_created = True diff --git a/features/tool_wrapping_runtime.feature b/features/tool_wrapping_runtime.feature index cad826bba..80ffc1286 100644 --- a/features/tool_wrapping_runtime.feature +++ b/features/tool_wrapping_runtime.feature @@ -217,4 +217,4 @@ Feature: Tool wrapping runtime Given a ToolRunner with a container-returning env resolver When I execute a tool through the runner with container env Then the tool result should have success false - And the tool result error should contain "Container execution is not yet implemented" + And the tool result error should contain "Container execution is not available" diff --git a/robot/container_tool_exec.robot b/robot/container_tool_exec.robot new file mode 100644 index 000000000..c83b18c61 --- /dev/null +++ b/robot/container_tool_exec.robot @@ -0,0 +1,157 @@ +*** Settings *** +Documentation Integration tests for container-aware tool execution (#515) +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${TIMEOUT} 60s + +*** Test Cases *** +PathMapper Maps Host To Container + [Documentation] Verify PathMapper translates host paths to container paths + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.path_mapper import PathMapper + ... m = PathMapper('/tmp/sandbox', '/workspace') + ... result = m.host_to_container('/tmp/sandbox/src/f.py') + ... assert result == '/workspace/src/f.py', f"got {result}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 PathMapper host_to_container failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +PathMapper Maps Container To Host + [Documentation] Verify PathMapper translates container paths to host paths + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.path_mapper import PathMapper + ... m = PathMapper('/tmp/sandbox', '/workspace') + ... result = m.container_to_host('/workspace/src/f.py') + ... assert result == '/tmp/sandbox/src/f.py', f"got {result}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 PathMapper container_to_host failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +PathMapper Leaves External Paths Unmapped + [Documentation] Paths outside root are returned unchanged + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.path_mapper import PathMapper + ... m = PathMapper('/tmp/sandbox', '/workspace') + ... result = m.host_to_container('/usr/lib/x') + ... assert result == '/usr/lib/x', f"got {result}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 PathMapper external path mapping failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ContainerConfig Has Defaults + [Documentation] ContainerConfig defaults to /workspace and 120s timeout + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.container_executor import ContainerConfig + ... c = ContainerConfig() + ... assert c.workspace_folder == '/workspace', f"got {c.workspace_folder}" + ... assert c.timeout_seconds == 120, f"got {c.timeout_seconds}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ContainerConfig defaults test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ContainerMetadata Is Frozen + [Documentation] ContainerMetadata is a frozen Pydantic model + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.container_executor import ContainerMetadata + ... m = ContainerMetadata(container_id='x', image='y') + ... assert m.container_id == 'x', f"got {m.container_id}" + ... assert m.image == 'y', f"got {m.image}" + ... assert m.timed_out is False + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ContainerMetadata test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ContainerToolExecutor Instantiation + [Documentation] ContainerToolExecutor can be created with ContainerConfig + ${script}= Catenate SEPARATOR=\n + ... import structlog, sys; structlog.configure(logger_factory=structlog.PrintLoggerFactory(file=sys.stderr)) + ... from cleveragents.tool.container_executor import ContainerConfig, ContainerToolExecutor + ... c = ContainerConfig(container_id='t') + ... e = ContainerToolExecutor(c) + ... assert e.config.container_id == 't', f"got {e.config.container_id}" + ... assert e.path_mapper is not None + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 Executor instantiation failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ContainerExecutionError Carries Details + [Documentation] ContainerExecutionError stores exit_code and stderr + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.container_executor import ContainerExecutionError + ... e = ContainerExecutionError('fail', exit_code=2, stderr='bad') + ... assert e.exit_code == 2, f"got {e.exit_code}" + ... assert e.stderr == 'bad', f"got {e.stderr}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ContainerExecutionError test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ContainerTimeoutError Carries Timeout + [Documentation] ContainerTimeoutError records timeout_seconds + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.tool.container_executor import ContainerTimeoutError + ... e = ContainerTimeoutError(30) + ... assert e.timeout_seconds == 30, f"got {e.timeout_seconds}" + ... assert e.timed_out is True + ... assert '30' in str(e) + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ContainerTimeoutError test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ToolInvocation Has ContainerMetadata Field + [Documentation] ToolInvocation accepts container_metadata dict + ${script}= Catenate SEPARATOR=\n + ... from cleveragents.domain.models.core.change import ToolInvocation + ... t = ToolInvocation( + ... plan_id='01TESTPLANID000000000000000', + ... tool_name='ns/t', + ... container_metadata={'container_id': 'x'}, + ... ) + ... assert t.container_metadata['container_id'] == 'x' + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ToolInvocation container_metadata test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK + +ToolRunner Container Routing Without Executor Returns Error + [Documentation] ToolRunner without ContainerToolExecutor returns error for container env + ${script}= Catenate SEPARATOR=\n + ... from unittest.mock import MagicMock + ... from cleveragents.tool.runner import ToolRunner + ... from cleveragents.tool.registry import ToolRegistry + ... from cleveragents.tool.runtime import ToolSpec + ... from cleveragents.domain.models.core.plan import ExecutionEnvironment + ... from cleveragents.application.services.execution_environment_resolver import ExecutionEnvironmentResolver + ... r = MagicMock(spec=ToolRegistry) + ... s = ToolSpec(name='t', description='d', input_schema={}, handler=lambda x: x) + ... r.get.return_value = s + ... ev = MagicMock(spec=ExecutionEnvironmentResolver) + ... ev.resolve_and_validate.return_value = ExecutionEnvironment.CONTAINER + ... runner = ToolRunner(registry=r, execution_environment_resolver=ev) + ... res = runner.execute('t', {}) + ... assert not res.success, f"Expected failure but got success" + ... assert 'ContainerToolExecutor' in res.error, f"Expected ContainerToolExecutor in error: {res.error}" + ... print("OK") + ${result}= Run Process ${PYTHON} -c ${script} + ... timeout=${TIMEOUT} env:PYTHONPATH=${CURDIR}/../src + Should Be Equal As Integers ${result.rc} 0 ToolRunner container routing test failed: ${result.stderr} + Should Be Equal As Strings ${result.stdout.strip()} OK diff --git a/src/cleveragents/domain/models/core/change.py b/src/cleveragents/domain/models/core/change.py index 6e10628b0..d902f0a28 100644 --- a/src/cleveragents/domain/models/core/change.py +++ b/src/cleveragents/domain/models/core/change.py @@ -472,6 +472,14 @@ class ToolInvocation(BaseModel): default=None, description="Provider info (model name, latency, etc.)", ) + container_metadata: dict[str, Any] | None = Field( + default=None, + description=( + "Container execution metadata when tool ran inside a container. " + "Includes container_id, image, workspace_folder, exec_time_ms, " + "exit_code, and timed_out." + ), + ) model_config = ConfigDict( str_strip_whitespace=True, @@ -600,7 +608,7 @@ class InMemoryChangeSetStore: cs = self._store.get(changeset_id) if cs is None: raise KeyError(f"ChangeSet '{changeset_id}' not found") - cs.entries.append(entry) + cs.add_change(entry) def get(self, changeset_id: str) -> SpecChangeSet | None: """Return a changeset by ID or ``None``.""" diff --git a/src/cleveragents/infrastructure/database/changeset_repository.py b/src/cleveragents/infrastructure/database/changeset_repository.py index 8ce860010..847d837f4 100644 --- a/src/cleveragents/infrastructure/database/changeset_repository.py +++ b/src/cleveragents/infrastructure/database/changeset_repository.py @@ -11,6 +11,7 @@ All repositories follow the session-factory pattern (ADR-007). from __future__ import annotations import json +import logging from collections.abc import Callable from datetime import datetime from typing import Any, cast @@ -35,6 +36,32 @@ from cleveragents.infrastructure.database.models import ( ToolInvocationModel, ) +_logger = logging.getLogger(__name__) + + +def _safe_json_loads( + raw: str | None, + *, + column: str = "", + default: Any = None, +) -> Any: + """Deserialise a JSON string from the database. + + Returns *default* (instead of raising) when the stored data is + corrupt or not valid JSON so that a single bad row does not break + bulk reads. + """ + if not raw: + return default + try: + return json.loads(raw) + except (json.JSONDecodeError, TypeError, ValueError): + _logger.warning( + "corrupt_json_column", + extra={"column": column, "raw_length": len(raw)}, + ) + return default + class ChangeSetEntryRepository: """Repository for persisting ChangeEntry records. @@ -221,7 +248,7 @@ class ToolInvocationRepository: if invocation.result is not None: result_json = json.dumps( invocation.result, - default=str, + allow_nan=False, ) completed_iso: str | None = None @@ -232,7 +259,14 @@ class ToolInvocationRepository: if invocation.provider_metadata is not None: pm_json = json.dumps( invocation.provider_metadata, - default=str, + allow_nan=False, + ) + + cm_json: str | None = None + if invocation.container_metadata is not None: + cm_json = json.dumps( + invocation.container_metadata, + allow_nan=False, ) model = ToolInvocationModel( @@ -241,7 +275,7 @@ class ToolInvocationRepository: plan_id=invocation.plan_id, tool_name=invocation.tool_name, skill_name=invocation.skill_name, - arguments_json=json.dumps(invocation.arguments), + arguments_json=json.dumps(invocation.arguments, allow_nan=False), result_json=result_json, error=invocation.error, success=invocation.success, @@ -255,6 +289,7 @@ class ToolInvocationRepository: invocation.resource_refs, ), provider_metadata_json=pm_json, + container_metadata_json=cm_json, ) session.add(model) session.flush() @@ -304,15 +339,27 @@ class ToolInvocationRepository: def _to_domain(row: ToolInvocationModel) -> ToolInvocation: """Convert a DB row to a ToolInvocation domain object.""" args_raw = cast("str | None", row.arguments_json) - args: dict[str, Any] = json.loads(args_raw) if args_raw else {} + args: dict[str, Any] = _safe_json_loads( + args_raw, column="arguments_json", default={} + ) res_raw = cast("str | None", row.result_json) - result: dict[str, Any] | None = json.loads(res_raw) if res_raw else None + result: dict[str, Any] | None = _safe_json_loads(res_raw, column="result_json") cids_raw = cast("str | None", row.change_ids_json) - change_ids: list[str] = json.loads(cids_raw) if cids_raw else [] + change_ids: list[str] = _safe_json_loads( + cids_raw, column="change_ids_json", default=[] + ) rr_raw = cast("str | None", row.resource_refs_json) - resource_refs: list[str] = json.loads(rr_raw) if rr_raw else [] + resource_refs: list[str] = _safe_json_loads( + rr_raw, column="resource_refs_json", default=[] + ) pm_raw = cast("str | None", row.provider_metadata_json) - provider_meta: dict[str, Any] | None = json.loads(pm_raw) if pm_raw else None + provider_meta: dict[str, Any] | None = _safe_json_loads( + pm_raw, column="provider_metadata_json" + ) + cm_raw = cast("str | None", row.container_metadata_json) + container_meta: dict[str, Any] | None = _safe_json_loads( + cm_raw, column="container_metadata_json" + ) dur_raw = cast("float | None", row.duration_ms) seq_raw = cast("int | None", row.sequence_number) @@ -339,6 +386,7 @@ class ToolInvocationRepository: sandbox_path=cast("str | None", row.sandbox_path), resource_refs=resource_refs, provider_metadata=provider_meta, + container_metadata=container_meta, ) diff --git a/src/cleveragents/infrastructure/database/models.py b/src/cleveragents/infrastructure/database/models.py index ffb7ba544..3ba4a32a5 100644 --- a/src/cleveragents/infrastructure/database/models.py +++ b/src/cleveragents/infrastructure/database/models.py @@ -2984,6 +2984,7 @@ class ToolInvocationModel(Base): # type: ignore[misc] sandbox_path = Column(String(1024), nullable=True) resource_refs_json = Column(Text, nullable=True) provider_metadata_json = Column(Text, nullable=True) + container_metadata_json = Column(Text, nullable=True) __table_args__ = ( Index("ix_tool_invocations_changeset_id", "changeset_id"), diff --git a/src/cleveragents/tool/__init__.py b/src/cleveragents/tool/__init__.py index b2c0d8082..da12d2cb6 100644 --- a/src/cleveragents/tool/__init__.py +++ b/src/cleveragents/tool/__init__.py @@ -17,6 +17,13 @@ from cleveragents.tool.actor_runtime import ( ToolCallingRuntime, ToolCallRunResult, ) +from cleveragents.tool.container_executor import ( + ContainerConfig, + ContainerExecutionError, + ContainerMetadata, + ContainerTimeoutError, + ContainerToolExecutor, +) from cleveragents.tool.context import ( BoundResource, CancellationToken, @@ -47,6 +54,7 @@ from cleveragents.tool.lifecycle import ( from cleveragents.tool.lifecycle import ( ToolResult as LifecycleToolResult, ) +from cleveragents.tool.path_mapper import PathMapper from cleveragents.tool.registry import ToolRegistry from cleveragents.tool.router import ( NormalizedToolCallResult, @@ -85,12 +93,18 @@ __all__ = [ "CancellationToken", "Change", "ChangeOperation", + "ContainerConfig", + "ContainerExecutionError", + "ContainerMetadata", + "ContainerTimeoutError", + "ContainerToolExecutor", "LLMCaller", "LLMResponse", "LLMToolCall", "LifecycleToolResult", "MaxIterationsExceededError", "NormalizedToolCallResult", + "PathMapper", "ProviderFormat", "StreamingStatus", "StreamingToolUpdate", diff --git a/src/cleveragents/tool/container_executor.py b/src/cleveragents/tool/container_executor.py new file mode 100644 index 000000000..b27d5906a --- /dev/null +++ b/src/cleveragents/tool/container_executor.py @@ -0,0 +1,770 @@ +"""Container-aware tool execution via ``devcontainer exec``. + +Implements :class:`ContainerToolExecutor` which wraps tool execution +inside a provisioned devcontainer. Arguments are escaped for shell +safety, results are captured as JSON, and structured errors are +produced on failure or timeout. + +This module fulfils issue #515 — container-aware tool execution and +I/O forwarding. +""" + +from __future__ import annotations + +import json +import os +import posixpath +import shlex +import shutil +import subprocess +import time +from pathlib import Path +from typing import Any + +import structlog +from pydantic import BaseModel, ConfigDict, Field, field_validator + +from cleveragents.tool.path_mapper import PathMapper +from cleveragents.tool.runtime import ToolResult + +logger = structlog.get_logger(__name__) + +# --------------------------------------------------------------------------- +# Domain models +# --------------------------------------------------------------------------- + +_DEFAULT_TIMEOUT_SECONDS = 120 +_MAX_RECURSION_DEPTH = 20 +_MAX_OUTPUT_BYTES = 50 * 1024 * 1024 # 50 MiB guard for captured output +_SAFE_SUBPROCESS_ENV_KEYS = ("PATH", "LANG", "TERM") + + +class ContainerMetadata(BaseModel): + """Metadata about a container execution. + + Stored on :class:`ToolInvocation` for audit trail purposes. + """ + + model_config = ConfigDict(frozen=True) + + container_id: str = "" + image: str = "" + workspace_folder: str = "" + exec_time_ms: float = 0.0 + exit_code: int | None = None + timed_out: bool = False + + +class ContainerConfig(BaseModel): + """Configuration for the container execution environment. + + Attributes: + workspace_folder: Absolute path to the workspace inside the container. + container_id: Docker/Podman container ID or name. + image: Container image reference. + timeout_seconds: Maximum execution time in seconds (must be > 0). + host_sandbox_path: Host-side sandbox root for file sync. + """ + + model_config = ConfigDict(frozen=True, str_strip_whitespace=True) + + workspace_folder: str = Field(default="/workspace", min_length=1) + container_id: str = "" + image: str = "" + timeout_seconds: int = Field(default=_DEFAULT_TIMEOUT_SECONDS, gt=0) + host_sandbox_path: str = "" + + @field_validator("workspace_folder") + @classmethod + def _validate_workspace_folder(cls, value: str) -> str: + if not value.startswith("/"): + raise ValueError( + "workspace_folder must be an absolute path (starting with '/')" + ) + # Normalise to collapse redundant separators and resolve ".." so + # the same path is used both by PathMapper and the devcontainer + # CLI (P1-11). + normalised = posixpath.normpath(value) + if normalised == "/": + raise ValueError("workspace_folder must not be the filesystem root '/'") + # Reject traversal components that survive normalisation (e.g. + # "/../etc" normalises to "/etc" — the intent is suspicious). + if "/.." in value or value.endswith("/.."): + raise ValueError("workspace_folder must not contain '..' path components") + return normalised + + +# --------------------------------------------------------------------------- +# Execution errors +# --------------------------------------------------------------------------- + + +class ContainerExecutionError(Exception): + """Raised when container execution fails.""" + + def __init__( + self, + message: str, + exit_code: int | None = None, + stderr: str = "", + timed_out: bool = False, + ) -> None: + super().__init__(message) + self.exit_code = exit_code + self.stderr = stderr + self.timed_out = timed_out + + +class ContainerTimeoutError(ContainerExecutionError): + """Raised when container execution exceeds the timeout.""" + + def __init__(self, timeout_seconds: int, stderr: str = "") -> None: + super().__init__( + f"Container execution timed out after {timeout_seconds}s", + exit_code=None, + stderr=stderr, + timed_out=True, + ) + self.timeout_seconds = timeout_seconds + + +# --------------------------------------------------------------------------- +# Executor +# --------------------------------------------------------------------------- + + +class _ExecResult(BaseModel): + """Internal result from a ``devcontainer exec`` call.""" + + stdout: str = "" + stderr: str = "" + raw_stdout: bytes = b"" + exit_code: int = 0 + duration_ms: float = 0.0 + timed_out: bool = False + + +class ContainerToolExecutor: + """Execute tools inside a devcontainer with I/O forwarding. + + The executor translates host-side tool inputs (paths, arguments) into + container-side equivalents using :class:`PathMapper`, executes the + tool via ``devcontainer exec``, and retrieves results. + + Parameters + ---------- + config: + Container configuration describing the target environment. + path_mapper: + Optional path mapper override. If ``None`` a default mapper is + created from the config. + + Usage:: + + config = ContainerConfig( + workspace_folder="/workspace", + container_id="abc123", + image="ghcr.io/org/devimage:latest", + host_sandbox_path="/tmp/sandbox", + ) + executor = ContainerToolExecutor(config) + result = executor.execute_tool("file_read", {"path": "/tmp/sandbox/foo.txt"}) + """ + + def __init__( + self, + config: ContainerConfig, + path_mapper: PathMapper | None = None, + ) -> None: + self._config = config + + host_root = config.host_sandbox_path or "/tmp/sandbox" + fallback_path = Path(host_root) + if not config.host_sandbox_path and fallback_path.is_symlink(): + raise ContainerExecutionError( + f"Default sandbox path {host_root!r} is a symlink — " + "set host_sandbox_path explicitly on ContainerConfig", + ) + + self._path_mapper = path_mapper or PathMapper( + host_root=host_root, + container_root=config.workspace_folder, + ) + + # Resolve devcontainer binary at init to prevent PATH hijacking. + # Store ``None`` when the binary is absent so that callers + # receive a clear error at execution time rather than silently + # falling back to a bare name that could be hijacked via PATH. + resolved_bin = shutil.which("devcontainer") + if resolved_bin is None: + logger.warning( + "devcontainer_binary_not_found", + msg=( + "devcontainer binary not found on PATH; " + "execute_tool and sync_results_to_host will fail" + ), + ) + self._devcontainer_bin: str | None = resolved_bin + + @property + def config(self) -> ContainerConfig: + """Return the container configuration.""" + return self._config + + @property + def path_mapper(self) -> PathMapper: + """Return the path mapper.""" + return self._path_mapper + + # ------------------------------------------------------------------ + # Public API + # ------------------------------------------------------------------ + + def execute_tool( + self, + tool_name: str, + inputs: dict[str, Any], + *, + timeout_seconds: int | None = None, + ) -> ToolResult: + """Execute a tool inside the container. + + Maps file paths in *inputs*, runs the tool via ``devcontainer + exec``, and returns a :class:`ToolResult`. + + Args: + tool_name: Namespaced tool name. + inputs: Tool input arguments. + timeout_seconds: Override for execution timeout. + + Returns: + :class:`ToolResult` with execution output or error. + + Raises: + ValueError: If *tool_name* is empty or *timeout_seconds* <= 0. + TypeError: If *inputs* is not a dict. + """ + if not tool_name: + raise ValueError("tool_name must be a non-empty string") + if not isinstance(inputs, dict): + raise TypeError("inputs must be a dict") + if timeout_seconds is not None and timeout_seconds <= 0: + raise ValueError("timeout_seconds must be > 0") + + timeout = timeout_seconds or self._config.timeout_seconds + + # Map host paths to container paths in inputs + mapped_inputs = self._map_input_paths(inputs) + + # Build the command — validate JSON-serialisability + try: + cmd, stdin_data = self._build_exec_command( + tool_name, mapped_inputs, timeout + ) + except (TypeError, ValueError) as exc: + return ToolResult( + success=False, + output={}, + error=f"Tool inputs are not JSON-serialisable: {exc}", + duration_ms=0.0, + ) + + logger.info( + "container_tool_exec_start", + tool_name=tool_name, + container_id=self._config.container_id, + timeout_seconds=timeout, + ) + + exec_result = self._run_command(cmd, timeout=timeout, stdin_data=stdin_data) + + metadata = ContainerMetadata( + container_id=self._config.container_id, + image=self._config.image, + workspace_folder=self._config.workspace_folder, + exec_time_ms=exec_result.duration_ms, + exit_code=exec_result.exit_code, + timed_out=exec_result.timed_out, + ) + metadata_dict = metadata.model_dump() + + if exec_result.timed_out: + logger.warning( + "container_tool_exec_timeout", + tool_name=tool_name, + timeout_seconds=timeout, + ) + return ToolResult( + success=False, + output={}, + error=f"Container execution timed out after {timeout}s", + duration_ms=exec_result.duration_ms, + metadata={"container": metadata_dict}, + ) + + if exec_result.exit_code != 0: + logger.warning( + "container_tool_exec_failed", + tool_name=tool_name, + exit_code=exec_result.exit_code, + ) + # Attempt best-effort parsing of stdout even on failure + partial_output = self._parse_output(exec_result.stdout) + return ToolResult( + success=False, + output=partial_output, + error=( + f"Container execution failed (exit code " + f"{exec_result.exit_code}): " + f"{exec_result.stderr[:500]}" + ), + duration_ms=exec_result.duration_ms, + metadata={"container": metadata_dict}, + ) + + # Parse output and map container paths back to host paths. + # Metadata is placed in ToolResult.metadata (not output) so the + # tool's semantic output is not polluted. + output = self._parse_output(exec_result.stdout) + output = self._map_output_paths(output) + + logger.info( + "container_tool_exec_success", + tool_name=tool_name, + duration_ms=exec_result.duration_ms, + ) + + return ToolResult( + success=True, + output=output, + duration_ms=exec_result.duration_ms, + metadata={"container": metadata_dict}, + ) + + def sync_results_to_host(self, container_path: str) -> str: + """Sync a container file back to the host sandbox. + + Uses ``devcontainer exec`` with ``cat`` to read the file + content from the container and writes it to the mapped host + path. + + Args: + container_path: Absolute path inside the container. + + Returns: + Host-side path where the content was written. + + Raises: + ContainerExecutionError: If the sync fails. + ContainerTimeoutError: If the sync times out. + ValueError: If *container_path* is not absolute or + contains null bytes. + """ + if "\x00" in container_path: + raise ValueError("container_path must not contain null bytes") + if not container_path.startswith("/"): + raise ValueError(f"container_path must be absolute, got {container_path!r}") + + host_path = self._path_mapper.container_to_host(container_path) + + # Validate host path falls within the sandbox root. + sandbox_root = Path(self._path_mapper.host_root).resolve() + resolved_host = Path(host_path).resolve() + if not resolved_host.is_relative_to(sandbox_root): + raise ContainerExecutionError( + f"Mapped host path {host_path!r} resolves outside " + f"sandbox root {sandbox_root}", + exit_code=-1, + ) + + cmd = self._build_sync_command(container_path) + result = self._run_command(cmd, timeout=self._config.timeout_seconds) + if result.timed_out: + raise ContainerTimeoutError( + timeout_seconds=self._config.timeout_seconds, + stderr=result.stderr, + ) + if result.exit_code != 0: + raise ContainerExecutionError( + f"Failed to sync {container_path} to host: {result.stderr}", + exit_code=result.exit_code, + stderr=result.stderr, + ) + + # Write captured content to the *resolved* host path to avoid a + # TOCTOU race between validation and actual file write. + # Use raw bytes from _run_command to avoid binary-file + # corruption from text-mode decode/re-encode (P1-10). + dest = resolved_host + try: + dest.parent.mkdir(parents=True, exist_ok=True, mode=0o700) + fd = os.open( + str(dest), + os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, + 0o600, + ) + try: + os.write(fd, result.raw_stdout) + finally: + os.close(fd) + except OSError as exc: + raise ContainerExecutionError( + f"Failed to write synced file to host path {dest}: {exc}", + exit_code=-1, + stderr=str(exc), + ) from exc + + return str(resolved_host) + + # ------------------------------------------------------------------ + # Command building + # ------------------------------------------------------------------ + + def _build_exec_command( + self, + tool_name: str, + inputs: dict[str, Any], + timeout: int, + ) -> tuple[list[str], str]: + """Build the ``devcontainer exec`` command for tool execution. + + Returns a (command, stdin_data) tuple. The JSON inputs are + piped via stdin instead of embedded as a shell argument to + avoid hitting the OS ``ARG_MAX`` limit. + """ + inputs_json = json.dumps(inputs, allow_nan=False) + + devcontainer = self._require_devcontainer_bin() + target_args = self._devcontainer_target_args() + + # Enforce int type to prevent shell injection via malicious + # objects with __str__ methods (P2-16). + timeout = int(timeout) + + # Use a slightly shorter container-side timeout so the + # container process self-terminates before the host-side + # deadline, avoiding orphan processes when the host kills + # its Popen first. + container_timeout = max(timeout - 5, 1) + cmd = [ + devcontainer, + "exec", + *target_args, + "--", + "sh", + "-c", + f"timeout {int(container_timeout)} cleveragents-tool-exec " + + shlex.quote(tool_name), + ] + return cmd, inputs_json + + def _build_sync_command(self, container_path: str) -> list[str]: + """Build a command to sync a file from container to host.""" + devcontainer = self._require_devcontainer_bin() + target_args = self._devcontainer_target_args() + return [ + devcontainer, + "exec", + *target_args, + "--", + "sh", + "-c", + f"cat {shlex.quote(container_path)}", + ] + + def _require_devcontainer_bin(self) -> str: + """Return the resolved devcontainer binary path or raise.""" + if self._devcontainer_bin is None: + raise ContainerExecutionError( + "devcontainer binary was not found on PATH at init time. " + "Install devcontainer CLI or set host_sandbox_path explicitly.", + exit_code=-1, + ) + return self._devcontainer_bin + + def _devcontainer_target_args(self) -> list[str]: + """Return CLI args to target the correct container.""" + if self._config.container_id: + return ["--container-id", self._config.container_id] + return ["--workspace-folder", self._config.workspace_folder] + + # ------------------------------------------------------------------ + # Command execution + # ------------------------------------------------------------------ + + def _run_command( + self, + cmd: list[str], + timeout: int, + *, + stdin_data: str | None = None, + ) -> _ExecResult: + """Execute a command and capture output with bounded memory. + + Uses :class:`subprocess.Popen` with incremental reads so that + at most ``_MAX_OUTPUT_BYTES`` are buffered per stream at any + time, preventing an unbounded memory allocation if the + container produces excessive output. + + Returns an :class:`_ExecResult` with stdout, stderr, exit code, + duration, and timeout status. + """ + safe_env = {k: os.environ.get(k, "") for k in _SAFE_SUBPROCESS_ENV_KEYS} + + start = time.monotonic() + try: + proc = subprocess.Popen( + cmd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + stdin=subprocess.PIPE if stdin_data else subprocess.DEVNULL, + env=safe_env, + ) + try: + # Feed stdin and close it so the child can proceed. + if stdin_data: + assert proc.stdin is not None # mypy guard + proc.stdin.write(stdin_data.encode("utf-8")) + proc.stdin.close() + + # Read stdout/stderr with a cap to avoid unbounded memory. + raw_stdout = _read_bounded(proc.stdout, _MAX_OUTPUT_BYTES) + raw_stderr = _read_bounded(proc.stderr, _MAX_OUTPUT_BYTES) + + remaining = timeout - (time.monotonic() - start) + proc.wait(timeout=max(remaining, 0)) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + elapsed = (time.monotonic() - start) * 1000.0 + return _ExecResult( + stdout="", + stderr=f"Command timed out after {timeout}s", + exit_code=-1, + duration_ms=elapsed, + timed_out=True, + ) + finally: + # Ensure all handles are closed even on unexpected errors. + for stream in (proc.stdout, proc.stderr, proc.stdin): + if stream and not stream.closed: + stream.close() + + elapsed = (time.monotonic() - start) * 1000.0 + + if len(raw_stdout) > _MAX_OUTPUT_BYTES: + logger.warning( + "container_output_truncated", + stream="stdout", + original_bytes=len(raw_stdout), + max_bytes=_MAX_OUTPUT_BYTES, + ) + raw_stdout = raw_stdout[:_MAX_OUTPUT_BYTES] + if len(raw_stderr) > _MAX_OUTPUT_BYTES: + logger.warning( + "container_output_truncated", + stream="stderr", + original_bytes=len(raw_stderr), + max_bytes=_MAX_OUTPUT_BYTES, + ) + raw_stderr = raw_stderr[:_MAX_OUTPUT_BYTES] + + stdout_text = raw_stdout.decode("utf-8", errors="replace") + stderr_text = raw_stderr.decode("utf-8", errors="replace") + return _ExecResult( + stdout=stdout_text, + stderr=stderr_text, + raw_stdout=raw_stdout, + exit_code=proc.returncode, + duration_ms=elapsed, + timed_out=False, + ) + except OSError as exc: + elapsed = (time.monotonic() - start) * 1000.0 + return _ExecResult( + stdout="", + stderr=str(exc), + exit_code=-1, + duration_ms=elapsed, + timed_out=False, + ) + + # ------------------------------------------------------------------ + # Path mapping + # ------------------------------------------------------------------ + + def _map_input_paths( + self, inputs: dict[str, Any], *, _depth: int = 0 + ) -> dict[str, Any]: + """Map host file paths in tool inputs to container paths.""" + if _depth > _MAX_RECURSION_DEPTH: + logger.warning( + "path_mapping_depth_exceeded", + direction="host_to_container", + max_depth=_MAX_RECURSION_DEPTH, + ) + return inputs + mapped: dict[str, Any] = {} + for key, value in inputs.items(): + mapped[key] = self._map_value_host_to_container(value, _depth=_depth) + return mapped + + def _map_value_host_to_container(self, value: Any, *, _depth: int = 0) -> Any: + """Recursively map a single value from host to container paths.""" + if _depth > _MAX_RECURSION_DEPTH: + logger.warning( + "path_mapping_depth_exceeded", + direction="host_to_container", + max_depth=_MAX_RECURSION_DEPTH, + ) + return value + if isinstance(value, str) and _looks_like_path(value): + if self._path_mapper.is_host_path(value): + return self._path_mapper.host_to_container(value) + return value + if isinstance(value, dict): + return { + k: self._map_value_host_to_container(v, _depth=_depth + 1) + for k, v in value.items() + } + if isinstance(value, list): + return [ + self._map_value_host_to_container(v, _depth=_depth + 1) for v in value + ] + return value + + def _map_output_paths( + self, output: dict[str, Any], *, _depth: int = 0 + ) -> dict[str, Any]: + """Map container file paths in tool output to host paths.""" + if _depth > _MAX_RECURSION_DEPTH: + logger.warning( + "path_mapping_depth_exceeded", + direction="container_to_host", + max_depth=_MAX_RECURSION_DEPTH, + ) + return output + mapped: dict[str, Any] = {} + for key, value in output.items(): + # Skip path mapping for raw_output to prevent nonsensical + # remapping of multi-line stdout that happens to start with + # the container root. + if key == "raw_output": + mapped[key] = value + continue + mapped[key] = self._map_value_container_to_host(value, _depth=_depth) + return mapped + + def _map_value_container_to_host(self, value: Any, *, _depth: int = 0) -> Any: + """Recursively map a single value from container to host paths.""" + if _depth > _MAX_RECURSION_DEPTH: + logger.warning( + "path_mapping_depth_exceeded", + direction="container_to_host", + max_depth=_MAX_RECURSION_DEPTH, + ) + return value + if isinstance(value, str) and _looks_like_path(value): + if self._path_mapper.is_container_path(value): + return self._path_mapper.container_to_host(value) + return value + if isinstance(value, dict): + return { + k: self._map_value_container_to_host(v, _depth=_depth + 1) + for k, v in value.items() + } + if isinstance(value, list): + return [ + self._map_value_container_to_host(v, _depth=_depth + 1) for v in value + ] + return value + + # ------------------------------------------------------------------ + # Output parsing + # ------------------------------------------------------------------ + + @staticmethod + def extract_container_metadata(result: ToolResult) -> dict[str, Any] | None: + """Extract container metadata from a :class:`ToolResult`. + + Returns the ``container`` dict from ``result.metadata`` if + present, or ``None`` otherwise. This is the bridge between the + executor's ``ToolResult`` output and the + :class:`ToolInvocation.container_metadata` field for audit trail + persistence (P1-9). + """ + return result.metadata.get("container") + + @staticmethod + def _parse_output(stdout: str) -> dict[str, Any]: + """Parse JSON output from the container tool execution. + + Falls back to wrapping raw output in a dict if JSON parsing + fails. + """ + if not stdout.strip(): + return {} + try: + parsed = json.loads(stdout) + if isinstance(parsed, dict): + return parsed + return {"result": parsed} + except (json.JSONDecodeError, TypeError, RecursionError, MemoryError): + return {"raw_output": stdout.strip()} + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_READ_CHUNK_SIZE = 65_536 # 64 KiB chunks for incremental stream reads + + +def _read_bounded(stream: Any, max_bytes: int) -> bytes: + """Read up to *max_bytes* from *stream*, then discard the rest. + + Reads in fixed-size chunks to avoid allocating the entire output + in a single ``read()`` call. + """ + chunks: list[bytes] = [] + total = 0 + while True: + chunk = stream.read(_READ_CHUNK_SIZE) + if not chunk: + break + remaining = max_bytes - total + if remaining <= 0: + # Already at the cap — drain the stream without keeping data + continue + if len(chunk) > remaining: + chunks.append(chunk[:remaining]) + total += remaining + else: + chunks.append(chunk) + total += len(chunk) + return b"".join(chunks) + + +def _looks_like_path(value: str) -> bool: + """Return ``True`` if *value* looks like a filesystem path. + + Rejects strings containing newlines, tabs, or null bytes which are + almost certainly not bare paths. Also rejects URL-like strings and + strings with query/fragment markers to avoid false positives on API + routes and URIs (P1-7). Spaces are allowed because they are valid + in filesystem paths (e.g. ``/home/user/My Documents``). + """ + if not value.startswith("/"): + return False + if "\n" in value or "\r" in value or "\t" in value: + return False + if "\x00" in value: + return False + # Reject URL-like patterns: "//host/...", protocol-relative URIs, + # and paths containing query strings or fragment identifiers. + if value.startswith("//"): + return False + if "?" in value or "#" in value: + return False + # Reject strings that look like URL paths with scheme separators. + return "://" not in value diff --git a/src/cleveragents/tool/path_mapper.py b/src/cleveragents/tool/path_mapper.py new file mode 100644 index 000000000..a74879173 --- /dev/null +++ b/src/cleveragents/tool/path_mapper.py @@ -0,0 +1,177 @@ +"""Host-to-container path mapping utility. + +Translates file paths between the host filesystem and the container +workspace for file-based tool arguments and outputs. + +Used by :class:`ContainerToolExecutor` to ensure file paths in tool +inputs point to the correct locations inside the container, and that +output paths are mapped back to the host sandbox. + +Based on issue #515 — container-aware tool execution and I/O forwarding. +""" + +from __future__ import annotations + +import posixpath +from dataclasses import dataclass + + +@dataclass(frozen=True) +class PathMapper: + """Bi-directional path mapper between host and container filesystems. + + Attributes: + host_root: Absolute path to the host-side sandbox root. + container_root: Absolute path to the container-side workspace root. + + Example:: + + mapper = PathMapper( + host_root="/tmp/sandbox/project-1", + container_root="/workspace", + ) + h2c = mapper.host_to_container( + "/tmp/sandbox/project-1/src/main.py", + ) + assert h2c == "/workspace/src/main.py" + c2h = mapper.container_to_host("/workspace/src/main.py") + assert c2h == "/tmp/sandbox/project-1/src/main.py" + """ + + host_root: str + container_root: str + + def __post_init__(self) -> None: + """Validate and normalise root paths.""" + if "\x00" in self.host_root or "\x00" in self.container_root: + raise ValueError("Root paths must not contain null bytes") + if not self.host_root or not self.container_root: + raise ValueError("Root paths must not be empty") + + normalised_host = _normalise(self.host_root) + normalised_container = _normalise(self.container_root) + + # ``posixpath.normpath("//")`` returns ``"//"`` per POSIX, so + # we must guard against both ``"/"`` and ``"//"`` to prevent a + # root-mapping bypass. + if normalised_host in ("/", "//"): + raise ValueError( + "host_root must not be '/' — it would map every absolute path" + ) + if normalised_container in ("/", "//"): + raise ValueError( + "container_root must not be '/' — it would map every absolute path" + ) + + # Reject overlapping roots which produce corrupt mappings where + # a relative-path component gets doubled (P2-14). + if _is_under(normalised_host, normalised_container) or _is_under( + normalised_container, normalised_host + ): + raise ValueError( + "host_root and container_root must not overlap: " + f"{normalised_host!r} vs {normalised_container!r}" + ) + + # Store normalised roots to avoid trailing-slash inconsistencies. + object.__setattr__(self, "host_root", normalised_host) + object.__setattr__(self, "container_root", normalised_container) + + def host_to_container(self, host_path: str) -> str: + """Map a host path to the equivalent container path. + + If *host_path* is not under :attr:`host_root`, it is returned + unchanged (external paths cannot be mapped). + + Args: + host_path: Absolute path on the host filesystem. + + Returns: + Equivalent path inside the container, or *host_path* if + it falls outside the host root. + + Raises: + ValueError: If *host_path* contains null bytes. + """ + _reject_null_bytes(host_path) + normalised = _normalise(host_path) + + if not _is_under(normalised, self.host_root): + return normalised + + relative = _relative_to(normalised, self.host_root) + return ( + posixpath.join(self.container_root, relative) + if relative + else self.container_root + ) + + def container_to_host(self, container_path: str) -> str: + """Map a container path to the equivalent host path. + + If *container_path* is not under :attr:`container_root`, it is + returned unchanged. + + Args: + container_path: Absolute path inside the container. + + Returns: + Equivalent path on the host filesystem, or *container_path* + if it falls outside the container root. + + Raises: + ValueError: If *container_path* contains null bytes. + """ + _reject_null_bytes(container_path) + normalised = _normalise(container_path) + + if not _is_under(normalised, self.container_root): + return normalised + + relative = _relative_to(normalised, self.container_root) + return posixpath.join(self.host_root, relative) if relative else self.host_root + + def is_host_path(self, path: str) -> bool: + """Check whether *path* is under the host root.""" + if "\x00" in path: + return False + return _is_under(_normalise(path), self.host_root) + + def is_container_path(self, path: str) -> bool: + """Check whether *path* is under the container root.""" + if "\x00" in path: + return False + return _is_under(_normalise(path), self.container_root) + + +# --------------------------------------------------------------------------- +# Path helpers (use posixpath for container paths which are always Linux) +# --------------------------------------------------------------------------- + + +def _reject_null_bytes(path: str) -> None: + """Raise ``ValueError`` if *path* contains null bytes.""" + if "\x00" in path: + raise ValueError(f"Path contains null bytes: {path!r}") + + +def _normalise(path: str) -> str: + """Normalise a path — strip trailing slashes, collapse double slashes.""" + return posixpath.normpath(path) + + +def _is_under(path: str, root: str) -> bool: + """Return ``True`` if *path* is equal to or a child of *root*.""" + if path == root: + return True + return path.startswith(root + "/") + + +def _relative_to(path: str, root: str) -> str: + """Return the part of *path* relative to *root*. + + Assumes :func:`_is_under` has already been checked. + """ + if path == root: + return "" + return path[len(root) + 1 :] diff --git a/src/cleveragents/tool/runner.py b/src/cleveragents/tool/runner.py index 86e932155..a3a93fad1 100644 --- a/src/cleveragents/tool/runner.py +++ b/src/cleveragents/tool/runner.py @@ -23,10 +23,12 @@ and the actual handler callables. from __future__ import annotations import json +import threading import time from typing import TYPE_CHECKING, Any from cleveragents.domain.models.core.plan import ExecutionEnvironment +from cleveragents.tool.container_executor import ContainerToolExecutor from cleveragents.tool.registry import ToolRegistry from cleveragents.tool.runtime import ToolError, ToolResult, ToolSpec @@ -51,6 +53,7 @@ class ToolRunner: self, registry: ToolRegistry, execution_environment_resolver: ExecutionEnvironmentResolver | None = None, + container_executor: ContainerToolExecutor | None = None, ) -> None: from cleveragents.application.services.execution_environment_resolver import ( ExecutionEnvironmentResolver as _Resolver, @@ -58,9 +61,11 @@ class ToolRunner: self._registry = registry self._active: dict[str, ToolSpec] = {} + self._active_lock = threading.RLock() self._env_resolver: ExecutionEnvironmentResolver = ( execution_environment_resolver or _Resolver() ) + self._container_executor = container_executor # -- Stage 1: Discover ---------------------------------------------------- @@ -88,7 +93,8 @@ class ToolRunner: error_type="ActivationError", details=f"Tool '{tool_name}' not found in registry", ) - self._active[tool_name] = spec + with self._active_lock: + self._active[tool_name] = spec return spec # -- Stage 3: Execute ----------------------------------------------------- @@ -119,6 +125,7 @@ class ToolRunner: project_env: str | None = None, linked_resource_types: list[str] | None = None, project_name: str = "", + timeout_seconds: int | None = None, ) -> ToolResult: """Run the tool with strict JSON-serialisable IO. @@ -128,8 +135,17 @@ class ToolRunner: Any exception raised by the handler is caught and normalised into a ``ToolResult`` with ``success=False``. + + Parameters + ---------- + timeout_seconds: + Optional timeout override forwarded to the container + executor when the execution environment is ``CONTAINER``. """ - spec = self._active.get(tool_name) or self._registry.get(tool_name) + with self._active_lock: + spec = self._active.get(tool_name) + if spec is None: + spec = self._registry.get(tool_name) if spec is None: raise ToolError( tool_name=tool_name, @@ -157,7 +173,7 @@ class ToolRunner: error=str(exc), duration_ms=0.0, ) - except ValueError as exc: + except Exception as exc: return ToolResult( success=False, output={}, @@ -167,18 +183,71 @@ class ToolRunner: # Container routing — delegate to container handler if env == ExecutionEnvironment.CONTAINER: - return ToolResult( - success=False, - output={}, - error=( - "Container execution is not yet implemented. " - "A container resource is linked but the container " - "handler has not been wired." - ), - duration_ms=0.0, - ) + if self._container_executor is None: + return ToolResult( + success=False, + output={}, + error=( + "Container execution is not available. " + "A ContainerToolExecutor must be configured on the " + "ToolRunner before container-routed tools can execute." + ), + duration_ms=0.0, + ) - # Validate that inputs are JSON-serialisable + # Validate input_schema and capabilities (T2) before + # delegating to the container executor so container-routed + # tools receive the same spec validation as host-routed ones. + try: + json.dumps(inputs, allow_nan=False) + except (TypeError, ValueError) as exc: + return ToolResult( + success=False, + output={}, + error=f"Inputs are not JSON-serialisable: {exc}", + duration_ms=0.0, + ) + + if spec.input_schema: + # Iterate the ``required`` list directly so that fields + # listed in ``required`` but absent from ``properties`` + # (e.g. ``additionalProperties`` patterns) are still + # detected (P2-20). + missing = [ + k for k in spec.input_schema.get("required", []) if k not in inputs + ] + if missing: + return ToolResult( + success=False, + output={}, + error=( + f"Missing required input fields for " + f"'{tool_name}': {missing}" + ), + duration_ms=0.0, + ) + + try: + return self._container_executor.execute_tool( + tool_name, + inputs, + timeout_seconds=timeout_seconds, + ) + except Exception as exc: + # Intentionally broad: the runner's contract is to + # normalise *any* handler failure into a ToolResult + # with ``success=False`` so callers never see raw + # exceptions from container execution. + return ToolResult( + success=False, + output={}, + error=f"Container execution error: {type(exc).__name__}: {exc}", + duration_ms=0.0, + ) + + # Validate that inputs are JSON-serialisable. Host-routed tools + # use the default ``allow_nan=True`` for backward compatibility; + # only the container path enforces RFC 7159 (P1-1). try: json.dumps(inputs) except (TypeError, ValueError) as exc: @@ -193,6 +262,9 @@ class ToolRunner: try: raw_output = spec.handler(inputs) except Exception as exc: + # Intentionally broad: the runner normalises handler + # exceptions into a failed ToolResult so callers get a + # uniform result type regardless of handler implementation. elapsed = (time.monotonic() - start) * 1000.0 return ToolResult( success=False, @@ -207,7 +279,8 @@ class ToolRunner: if not isinstance(raw_output, dict): raw_output = {"result": raw_output} - # Validate that output is JSON-serialisable + # Validate that output is JSON-serialisable. Host-routed tools + # use the default ``allow_nan=True`` for backward compatibility. try: json.dumps(raw_output) except (TypeError, ValueError) as exc: @@ -232,7 +305,8 @@ class ToolRunner: Returns ``True`` if the tool was active and has been deactivated, ``False`` otherwise. """ - if tool_name in self._active: - del self._active[tool_name] - return True - return False + with self._active_lock: + if tool_name in self._active: + del self._active[tool_name] + return True + return False diff --git a/src/cleveragents/tool/runtime.py b/src/cleveragents/tool/runtime.py index eb04287b8..40dd93110 100644 --- a/src/cleveragents/tool/runtime.py +++ b/src/cleveragents/tool/runtime.py @@ -31,7 +31,7 @@ from __future__ import annotations from collections.abc import Callable from typing import Any -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, model_validator from cleveragents.domain.models.core.tool import ToolCapability @@ -121,6 +121,24 @@ class ToolResult(BaseModel): validate_assignment=True, ) + @model_validator(mode="after") + def _validate_success_error_consistency(self) -> ToolResult: + """Ensure success and error fields are logically consistent. + + A result marked as ``success=True`` must not carry an error + message, and a result marked as ``success=False`` must have + an error message explaining the failure. + """ + if self.success and self.error is not None: + raise ValueError( + "ToolResult with success=True must not have an error message" + ) + if not self.success and self.error is None: + raise ValueError( + "ToolResult with success=False must include an error message" + ) + return self + class ToolError(Exception): """Exception raised for tool execution failures. diff --git a/vulture_whitelist.py b/vulture_whitelist.py index b411bcf76..ba393cee1 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -943,6 +943,7 @@ score_detailed # noqa: B018, F821 depth_fallback_steps # noqa: B018, F821 min_fragment_tokens # noqa: B018, F821 + # LSP Server Stub — public API (issue #203) LspServer # noqa: B018, F821 MAX_CONTENT_LENGTH # noqa: B018, F821 @@ -1097,3 +1098,29 @@ _build_analyzer_registry # noqa: B018, F821 # Test doubles — public API for BDD/Robot test infrastructure TrackingEventBus # noqa: B018, F821 + +# Container Tool Execution (#515) — public API +ContainerConfig # noqa: B018, F821 +ContainerExecutionError # noqa: B018, F821 +ContainerMetadata # noqa: B018, F821 +ContainerTimeoutError # noqa: B018, F821 +ContainerToolExecutor # noqa: B018, F821 +PathMapper # noqa: B018, F821 +container_metadata # noqa: B018, F821 +container_id # noqa: B018, F821 +exec_time_ms # noqa: B018, F821 +exit_code # noqa: B018, F821 +host_root # noqa: B018, F821 +container_root # noqa: B018, F821 +host_sandbox_path # noqa: B018, F821 +image # noqa: B018, F821 +is_container_path # noqa: B018, F821 +is_host_path # noqa: B018, F821 +host_to_container # noqa: B018, F821 +container_to_host # noqa: B018, F821 +sync_results_to_host # noqa: B018, F821 +timed_out # noqa: B018, F821 +timeout_seconds # noqa: B018, F821 +workspace_folder # noqa: B018, F821 +execute_tool # noqa: B018, F821 +wrap_service_method # noqa: B018, F821