diff --git a/CHANGELOG.md b/CHANGELOG.md index eb19eabbd..d32928830 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -243,6 +243,14 @@ resource types with `DevcontainerHandler` and auto-discovery logic that scans for `.devcontainer/` directories when `git-checkout` or `fs-directory` resources are linked. Includes CLI support, Behave/Robot/ASV tests, and reference documentation. (#511) +- Added full lifecycle management for `devcontainer-instance` resources: lazy activation + on first tool target via `DevcontainerHandler.resolve()`, health checking with periodic + liveness probes, manual `agents resource stop`/`agents resource rebuild` CLI commands + (rebuild passes `--reset-container` to force container recreation), and automatic + session-scoped cleanup on session close or plan completion. Lifecycle state tracked via + `ContainerLifecycleTracker` with six states (`detected`/`building`/`running`/`stopping`/ + `stopped`/`failed`) and validated transitions. Includes Behave BDD scenarios, Robot + Framework integration tests, ASV benchmarks, and reference documentation. (#514) - Added skeleton compressor service (`SkeletonCompressorService`) for ACMS context inheritance. Compresses parent plan context fragments by `skeleton_ratio` (0.0–1.0) for propagation to child plans. Persists `SkeletonMetadata` (ratio, token counts, source decision IDs) on the diff --git a/benchmarks/devcontainer_lifecycle_bench.py b/benchmarks/devcontainer_lifecycle_bench.py new file mode 100644 index 000000000..b9bf9ec83 --- /dev/null +++ b/benchmarks/devcontainer_lifecycle_bench.py @@ -0,0 +1,219 @@ +"""ASV benchmarks for devcontainer lifecycle activation latency. + +Measures the performance of: +- ContainerLifecycleState transition validation +- Lifecycle tracker construction and transition +- Lazy activation with mock runner (no real subprocess) +- JSON output parsing from devcontainer up +- Lifecycle registry operations (get/set/list) + +Based on issue #514: Devcontainer lifecycle management. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path + +try: + from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, + validate_transition, + ) + from cleveragents.resource.handlers.devcontainer import ( + _parse_devcontainer_up_output, + activate_container, + clear_lifecycle_registry, + get_lifecycle_tracker, + list_active_containers, + set_lifecycle_tracker, + ) +except ModuleNotFoundError: + sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) + from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, + validate_transition, + ) + from cleveragents.resource.handlers.devcontainer import ( + _parse_devcontainer_up_output, + activate_container, + clear_lifecycle_registry, + get_lifecycle_tracker, + list_active_containers, + set_lifecycle_tracker, + ) + + +class _MockResult: + """Minimal mock result for benchmark subprocess calls.""" + + def __init__(self) -> None: + self.returncode = 0 + self.stdout = json.dumps( + { + "outcome": "success", + # F3 fix: use valid 20-char hex ID to pass S1 validation + "containerId": "aabbccddee0011223344", + "remoteWorkspaceFolder": "/ws", + } + ) + self.stderr = "" + + +def _mock_runner(args: list[str], **kwargs: object) -> _MockResult: + """Zero-overhead mock subprocess runner.""" + return _MockResult() + + +class TimeTransitionValidation: + """Benchmark validate_transition throughput.""" + + timeout = 10 + + def time_valid_transition(self) -> None: + """Time valid transition check (1000 iterations).""" + for _ in range(1000): + validate_transition( + ContainerLifecycleState.DETECTED, + ContainerLifecycleState.BUILDING, + ) + + def time_invalid_transition(self) -> None: + """Time invalid transition check (1000 iterations).""" + for _ in range(1000): + validate_transition( + ContainerLifecycleState.DETECTED, + ContainerLifecycleState.RUNNING, + ) + + +class TimeTrackerConstruction: + """Benchmark lifecycle tracker creation.""" + + timeout = 10 + + def time_tracker_construction(self) -> None: + """Time ContainerLifecycleTracker instantiation.""" + for i in range(1000): + ContainerLifecycleTracker( + resource_id=f"01BENCH{i:020d}", + ) + + +class TimeTransitionState: + """Benchmark state transition with history recording.""" + + timeout = 30 + + def time_single_transition(self) -> None: + """Time a single state transition.""" + for _ in range(1000): + tracker = ContainerLifecycleTracker( + resource_id="01BENCHTRACKER0000000001", + ) + transition_state( + tracker, + ContainerLifecycleState.BUILDING, + reason="benchmark", + ) + + +class TimeActivationLatency: + """Benchmark lazy activation with mock runner.""" + + timeout = 30 + params: list[int] = [1, 10, 50] + param_names: list[str] = ["num_activations"] + + def setup(self, num_activations: int) -> None: + """Clear registry before each timing iteration (R17 fix). + + Without this, trackers from prior iterations accumulate in + ``_lifecycle_registry`` and subsequent activate calls fail + with invalid-transition errors (``running`` → ``building``). + """ + clear_lifecycle_registry() + + def time_activation(self, num_activations: int) -> None: + """Time container activation with mock runner. + + R8-F6 fix: clear the registry after the loop to stop health + check threads spawned by each activation (F3 auto-start). + Without this, ``num_activations`` daemon threads accumulate + during the timing call. + """ + for i in range(num_activations): + activate_container( + f"01BENCHACTIVATE{i:011d}", + "/workspace", + run_command=_mock_runner, + ) + clear_lifecycle_registry() + + def teardown(self, num_activations: int) -> None: + """Clear registry after each timing iteration (safety net).""" + clear_lifecycle_registry() + + +class TimeJsonParsing: + """Benchmark devcontainer up JSON output parsing.""" + + timeout = 10 + + def setup(self) -> None: + """Create sample JSON output.""" + self.valid_json = json.dumps( + { + "outcome": "success", + "containerId": "aabbccddee0011223344", + "remoteWorkspaceFolder": "/workspaces/project", + } + ) + self.invalid_json = "not valid json {{" + + def time_parse_valid_json(self) -> None: + """Time parsing valid JSON output.""" + for _ in range(1000): + _parse_devcontainer_up_output(self.valid_json) + + def time_parse_invalid_json(self) -> None: + """Time parsing invalid JSON output.""" + for _ in range(1000): + _parse_devcontainer_up_output(self.invalid_json) + + +class TimeRegistryOperations: + """Benchmark lifecycle registry get/set/list operations.""" + + timeout = 30 + + def setup(self) -> None: + """Pre-populate registry with trackers.""" + clear_lifecycle_registry() + for i in range(100): + tracker = ContainerLifecycleTracker( + resource_id=f"01BENCHREG{i:016d}", + current_state=ContainerLifecycleState.RUNNING + if i % 2 == 0 + else ContainerLifecycleState.DETECTED, + ) + set_lifecycle_tracker(tracker) + + def teardown(self) -> None: + """Clear registry.""" + clear_lifecycle_registry() + + def time_get_tracker(self) -> None: + """Time registry lookup.""" + for i in range(1000): + get_lifecycle_tracker(f"01BENCHREG{(i % 100):016d}") + + def time_list_active(self) -> None: + """Time listing active containers.""" + for _ in range(100): + list_active_containers() diff --git a/docs/reference/devcontainer_resources.md b/docs/reference/devcontainer_resources.md index 5d7c6b123..f6f8e5741 100644 --- a/docs/reference/devcontainer_resources.md +++ b/docs/reference/devcontainer_resources.md @@ -17,7 +17,7 @@ A container execution environment defined by a | Property | Value | |-------------------|-----------------------| | Kind | physical | -| Sandbox strategy | snapshot | +| Sandbox strategy | none (container provides isolation) | | User-addable | yes | | Parent types | git-checkout, fs-directory | | Child types | devcontainer-file | @@ -51,22 +51,30 @@ inherits. Represents a generic container execution environment. | Property | Value | |-------------------|-----------------------| | Kind | physical | -| Sandbox strategy | snapshot | +| Sandbox strategy | none (container provides isolation) | | User-addable | yes | | Parent types | git-checkout, fs-directory | | Child types | (none) | | Handler | `DevcontainerHandler` | -## Auto-Discovery +## Auto-Discovery (Planned) -When a `git-checkout` or `fs-directory` resource is linked to a -project, the auto-discovery hook scans for devcontainer configurations -in the following locations (relative to the resource root): +> **Not yet wired (F31/F23):** The discovery module +> (`discover_devcontainers()`) exists and is tested in isolation, but +> is **not** invoked during `project link-resource` or any other +> production code path. Auto-discovery will be wired in a follow-up PR. +> For now, devcontainer-instance resources must be added manually via +> `agents resource add`. + +When wired, linking a `git-checkout` or `fs-directory` resource to a +project will trigger an auto-discovery hook that scans for devcontainer +configurations in the following locations (relative to the resource +root): 1. `.devcontainer/devcontainer.json` 2. `.devcontainer.json` (root-level) -### Discovery Process +### Discovery Process (Planned) 1. **Trigger**: Linking a `git-checkout` or `fs-directory` resource. 2. **Scan**: The discovery module checks for configuration files at the @@ -85,6 +93,99 @@ Devcontainers use lazy activation: the container is only built and started when first needed by a plan execution. The discovery phase only registers the resource metadata. +## Lifecycle Management + +Devcontainer-instance resources support a full lifecycle state +machine with six states and validated transitions. + +### State Diagram + +``` +detected ──► building ──► running ──► stopping ──► stopped + │ │ + ▼ │ + failed ◄───────────────────────────────┘ + │ ▲ + └──► building │ + (retry) (rebuild) + │ + stopped ───────┘ +``` + +### States + +| State | Description | +|------------|---------------------------------------------------| +| `detected` | Container not yet started (default for new) | +| `building` | Container is being activated (`devcontainer up`) | +| `running` | Container is running and healthy | +| `stopping` | Container is being stopped | +| `stopped` | Container has been stopped cleanly | +| `failed` | Container is in an error state | + +### Valid Transitions + +| From | To | Trigger | +|-----------|---------------------|-----------------------| +| detected | building | lazy activation | +| building | running | `devcontainer up` OK | +| building | failed | `devcontainer up` fail| +| running | stopping | stop command | +| stopping | stopped | container stopped | +| stopping | failed | stop failed | +| stopped | building | rebuild | +| failed | building | retry | + +### Lazy Activation Flow + +When a tool targets a devcontainer-instance that is in `detected` +state: + +1. Transition to `building`. +2. Run `devcontainer up --workspace-folder `. +3. Parse JSON output for `containerId` and `remoteWorkspaceFolder`. +4. On success: transition to `running`. +5. On failure: transition to `failed`. + +### Health Checking + +> **Implementation-ahead-of-spec (SPEC-3):** Health checking and +> session-scoped cleanup are implemented but not yet formally specified +> in `docs/specification.md` for container resources (the spec only +> defines health checking for LSP servers). These features fulfil +> Issue #514 acceptance criteria and will be added to the specification +> in a follow-up PR. + +Active containers receive periodic health probes. Health checking +starts automatically when a container transitions to `running` state +via `activate_container`. + +- **Command**: `devcontainer exec ... echo ping` +- **Default interval**: 30 seconds (configurable via + `health_check_interval` on the lifecycle tracker). +- **On failure**: transitions to `failed` via `stopping`. +- Health checks run as background daemon threads. +- Health checks are stopped automatically before container stop. + +### Session/Plan Cleanup + +When a session closes or a plan completes/fails/is-cancelled, active +devcontainer-instances **scoped to that session or plan** are stopped +automatically. Each `ContainerLifecycleTracker` carries an optional +`session_id` field, set during activation, used to scope cleanup. + +Cleanup is wired into the following hooks: + +- `AcpLocalFacade._handle_session_close` — session close +- `PlanLifecycleService.complete_apply` — plan success +- `PlanLifecycleService.fail_apply` — plan apply failure +- `PlanLifecycleService.fail_execute` — plan execute failure +- `PlanLifecycleService.cancel_plan` — plan cancellation + +```python +CleanupService.stop_active_devcontainers(session_id="...") +``` + ## CLI Usage ```bash @@ -105,8 +206,53 @@ agents resource tree local/my-repo # Inspect resource with tree view agents resource inspect local/my-repo --tree + +# Stop an active devcontainer +agents resource stop local/my-dc + +# Stop without confirmation prompt (for scripts) +agents resource stop local/my-dc --yes + +# Rebuild a stopped or errored devcontainer +agents resource rebuild local/my-dc --yes ``` +### Stop Command + +```bash +agents resource stop [--yes|-y] +``` + +Transitions an active container through `running → stopping → stopped`. +Only `devcontainer-instance` resources may be stopped (F19 fix: +`container-instance` was removed because it has no lifecycle tracker +wiring). Prompts for confirmation unless `--yes` (`-y`) is passed. +If the container is already in a terminal state (`stopped` or +`failed`), the stop is a no-op and returns immediately. + +### Rebuild Command + +```bash +agents resource rebuild [--yes|-y] +``` + +Transitions a stopped or errored container through +`stopped/failed → building → running`. Runs `devcontainer up` to +rebuild the container. Only `devcontainer-instance` resources may +be rebuilt (not generic `container-instance`). Prompts for +confirmation unless `--yes` (`-y`) is passed. + +## Known Limitations + +| Limitation | Detail | Planned Fix | +|------------|--------|-------------| +| In-memory lifecycle state (F20) | Lifecycle state is tracked in an in-memory registry (`_lifecycle_registry` dict), **not** persisted on the resource model or database. State is lost on process restart. CLI `stop`/`rebuild` in a new process cannot see trackers from a previous process. | M7+: Persist `activation_state` on the resource model per the spec field definition, hydrating trackers on CLI operations. | +| Registry eviction | Terminal-state trackers (stopped/failed) are evicted when count exceeds 200 (`_MAX_TERMINAL_TRACKERS`). Eviction runs automatically after bulk cleanup via `stop_all_active_containers`. Long-running processes with many container cycles may lose historical tracker data. | Acceptable for MVP; persisted state in M7+ eliminates this. | +| Sandbox strategy (F22/F25) | Specification uses `container_snapshot`; `SandboxFactory` does not yet implement `snapshot`. Handler now uses `SandboxStrategy.NONE` — the container itself provides isolation. | Implement `container_snapshot` in `SandboxFactory` and switch handler back to it. | +| Hardcoded `docker stop` (F21) | `stop_container()` calls `docker stop` directly. Devcontainer CLI can target Podman or other engines; the devcontainer CLI does not yet offer a `stop` subcommand. | Detect the container engine from resource properties or devcontainer CLI config and dispatch to the appropriate stop command. | +| Auto-discovery not wired (F23) | Documentation describes auto-discovery on `project link-resource`, but no code path invokes `discover_devcontainers()` during resource linking. | Wire the discovery hook into the resource linking flow. | +| Container execution stubbed (F24) | `ToolRunner` returns an error for `ExecutionEnvironment.CONTAINER`, so lazy activation cannot trigger via tool use in production. Lazy activation currently works through `DevcontainerHandler.resolve()` on plan sandbox resolution. | Full container execution support is scoped for a follow-up PR (#616). | + ## Architecture The devcontainer integration follows ADR-043 and builds on the @@ -115,9 +261,11 @@ resource type inheritance model defined in ADR-042. ### Handler `DevcontainerHandler` extends `BaseResourceHandler` and uses the -`snapshot` sandbox strategy. It handles both `devcontainer-instance` -and `devcontainer-file` resource types through the common base class -resolve logic. +`none` sandbox strategy (the container itself provides isolation). +For `devcontainer-instance` resources, the handler overrides +`resolve()` to trigger lazy activation via `activate_container()` +when the resource is in a non-running state. It handles both +`devcontainer-instance` and `devcontainer-file` resource types. ### Discovery Module diff --git a/docs/specification.md b/docs/specification.md index ea14ef534..4d0b93847 100644 --- a/docs/specification.md +++ b/docs/specification.md @@ -316,6 +316,8 @@ The following standards are integrated into the architecture: agents resource tree [(--depth|-d) <N>] [(--type|-t) <TYPE>] <RESOURCE> agents resource link-child <PARENT> <CHILD> agents resource unlink-child [--yes|-y] <PARENT> <CHILD> +agents resource stop <NAME> +agents resource rebuild <NAME> agents plan list [--phase <PHASE>] [--state <STATE>] [--project <PROJECT>] [--action <ACTION>] [<REGEX>] @@ -12008,6 +12010,76 @@ Remove a manual parent/child link between two resources. Auto-discovered links c - Child resource unlinked ``` +##### agents resource stop + +
agents resource stop <NAME> [--yes | -y]
+ +**Purpose** +Stop an active `devcontainer-instance` or `container-instance` resource. Transitions the container through `running → stopping → stopped`. Only container-typed resources may be stopped; attempting to stop other resource types produces an error. + +**Arguments** + +- ``: Resource name or ULID of the devcontainer to stop. + +**Options** + +- `--yes` / `-y`: Skip the confirmation prompt (useful in scripts and automation). + +**Examples** + +=== "Rich" + + 

+    $ agents resource stop local/my-dc
+
+    Stopping container local/my-dc (state: running)...
+    Stopped: local/my-dc
+
+ +=== "Plain" + + ``` + $ agents resource stop local/my-dc + + Stopping container local/my-dc (state: running)... + Stopped: local/my-dc + ``` + +##### agents resource rebuild + +
agents resource rebuild <NAME> [--yes | -y]
+ +**Purpose** +Rebuild a stopped or failed `devcontainer-instance` resource. Transitions the container through `stopped/failed → building → running` by invoking `devcontainer up` with the resource's workspace location. Only `devcontainer-instance` resources may be rebuilt since the rebuild process invokes the devcontainer CLI; generic `container-instance` resources do not support rebuild. + +**Arguments** + +- ``: Resource name or ULID of the devcontainer to rebuild. + +**Options** + +- `--yes` / `-y`: Skip the confirmation prompt (useful in scripts and automation). + +**Examples** + +=== "Rich" + + 

+    $ agents resource rebuild local/my-dc
+
+    Rebuilding local/my-dc...
+    Rebuilt: local/my-dc
+
+ +=== "Plain" + + ``` + $ agents resource rebuild local/my-dc + + Rebuilding local/my-dc... + Rebuilt: local/my-dc + ``` + #### agents plan !!! info "Purpose" @@ -23497,8 +23569,8 @@ Example: description: "Path to .devcontainer/devcontainer.json or .devcontainer/ directory" handler: - class: DevcontainerInstanceHandler - module: cleveragents.resources.handlers.devcontainer + class: DevcontainerHandler + module: cleveragents.resource.handlers.devcontainer ##### Built-in Resource Types @@ -24849,7 +24921,7 @@ Every resource type provides a handler that implements this interface: | `container-runtime` | `ContainerRuntimeHandler` | ✓ | ✗ | ✗ | `none` | | `container-image` | `ContainerImageHandler` | ✓ | ✗ | ✗ | `none` | | `container-instance` | `ContainerInstanceHandler` | ✓ | ✓ | ✓ | `snapshot` | -| `devcontainer-instance` (inherits `container-instance`) | `DevcontainerInstanceHandler` | ✓ | ✓ | ✓ | `snapshot` (inherited) | +| `devcontainer-instance` (inherits `container-instance`) | `DevcontainerHandler` | ✓ | ✓ | ✓ | `snapshot` (inherited) | | `container-mount`, `container-exec-env`, `container-port` | `ContainerChildHandler` | ✓ | varies | ✗ | (inherits) | | `container-volume` | `ContainerVolumeHandler` | ✓ | ✓ | ✓ | `snapshot` | | `container-network` | `ContainerNetworkHandler` | ✓ | ✗ | ✗ | `none` | @@ -32928,7 +33000,7 @@ A custom resource type for Subversion repositories: handler: class: SVNHandler - module: cleveragents.resources.handlers.svn + module: cleveragents.resource.handlers.svn config: svn_binary: "svn" trust_server_cert: true @@ -32993,7 +33065,7 @@ A resource type for Amazon S3 buckets with prefix-based child discovery: handler: class: S3BucketHandler - module: cleveragents.resources.handlers.s3 + module: cleveragents.resource.handlers.s3 config: max_object_size: 104857600 # 100 MB default_acl: "private" @@ -33071,7 +33143,7 @@ A resource type for relational databases: handler: class: DatabaseHandler - module: cleveragents.resources.handlers.database + module: cleveragents.resource.handlers.database config: connection_pool_size: 5 statement_timeout: 30000 # 30 seconds @@ -33112,7 +33184,7 @@ A virtual resource type that links equivalent physical files across repositories handler: class: VirtualConfigFileHandler - module: cleveragents.resources.handlers.virtual_config + module: cleveragents.resource.handlers.virtual_config config: track_content_drift: true @@ -33171,7 +33243,7 @@ A resource type for Docker container registries with full auto-discovery and lif handler: class: DockerRegistryHandler - module: cleveragents.resources.handlers.docker + module: cleveragents.resource.handlers.docker config: api_version: "v2" page_size: 100 @@ -33226,12 +33298,12 @@ This example demonstrates the `devcontainer-instance` type, which inherits from required: false description: "Command(s) to run after container start, from devcontainer.json postStartCommand" activation_state: - type: "enum(detected, building, running, stopped, failed)" + type: "enum(detected, building, running, stopping, stopped, failed)" required: true default: "detected" - description: "Lifecycle state. Starts as 'detected' (lazy); transitions to 'building' then 'running' on first access." + description: "Lifecycle state. Starts as 'detected' (lazy); transitions to 'building' then 'running' on first access. The 'stopping' state tracks in-progress shutdown before reaching 'stopped'." -handler: "DevcontainerInstanceHandler" +handler: "DevcontainerHandler" sandbox_strategy: "container_snapshot" # Overrides the parent's sandbox_strategy; devcontainers use container snapshots diff --git a/features/devcontainer_activation.feature b/features/devcontainer_activation.feature new file mode 100644 index 000000000..6299fea7b --- /dev/null +++ b/features/devcontainer_activation.feature @@ -0,0 +1,217 @@ +Feature: Devcontainer Activation, Stop and Rebuild + As a CleverAgents developer + I want devcontainers to be lazily activated and manually stopped or rebuilt + So that containers start on demand and can be controlled explicitly + + # ── Lazy activation ──────────────────────────────────────── + + Scenario: Lazy activation succeeds on inactive container + Given a mock devcontainer CLI runner + And the runner configured for successful activation + When I activate container "01TESTLIFECYCLE0000000010" at "/workspace/project" + Then the container state should be "running" + And the container ID should be "aabbccddee0011223344" + And the workspace path should be "/workspaces/project" + And the runner should have received a devcontainer up call + + Scenario: Lazy activation transitions to error on failure + Given a mock devcontainer CLI runner + And the runner configured for failed activation + When I attempt to activate container "01TESTLIFECYCLE0000000011" at "/workspace/project" + Then the activation should raise RuntimeError + And the container state for "01TESTLIFECYCLE0000000011" should be "failed" + + # ── Manual stop ──────────────────────────────────────────── + + Scenario: Stop transitions active container to stopped + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000020" with container ID "ctr-123" + When I stop container "01TESTLIFECYCLE0000000020" + Then the container state for "01TESTLIFECYCLE0000000020" should be "stopped" + And the runner should have received a docker stop call + + # ── Rebuild ──────────────────────────────────────────────── + + Scenario: Rebuild transitions stopped container to active + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And a stopped container "01TESTLIFECYCLE0000000030" + When I rebuild container "01TESTLIFECYCLE0000000030" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000030" should be "running" + + Scenario: Rebuild transitions error container to active + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And an errored container "01TESTLIFECYCLE0000000031" + When I rebuild container "01TESTLIFECYCLE0000000031" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000031" should be "running" + + # ── Argument validation ──────────────────────────────────── + + Scenario: Activate rejects empty resource_id + When I attempt to activate a container with empty resource_id + Then the activation should raise ValueError with "resource_id" + + Scenario: Activate rejects empty workspace_folder + When I attempt to activate a container with empty workspace_folder + Then the activation should raise ValueError with "workspace_folder" + + Scenario: Stop rejects empty resource_id + When I attempt to stop a container with empty resource_id + Then the stop should raise ValueError with "resource_id" + + # ── Activate on already-running container ──────────────────── + + Scenario: Activate on already-running container raises ValueError + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And an active container "01TESTLIFECYCLE0000000140" with container ID "ctr-dup" + When I attempt to activate container "01TESTLIFECYCLE0000000140" at "/workspace" + Then the activation should raise ValueError with "Invalid lifecycle transition" + + # ── Stop container with no container_id ───────────────────── + + Scenario: Stop container with no container_id skips docker stop + Given a mock devcontainer CLI runner + And an active container with no container_id "01TESTLIFECYCLE0000000150" + When I stop container "01TESTLIFECYCLE0000000150" + Then the container state for "01TESTLIFECYCLE0000000150" should be "stopped" + And the runner should not have received a docker stop call + + # ── Activation fails when container_id is missing ──────────── + + Scenario: Activation fails when devcontainer up returns no container_id + Given a mock devcontainer CLI runner + And the runner configured for activation with no container_id + When I attempt to activate container "01TESTLIFECYCLE0000000160" at "/workspace" + Then the activation should raise RuntimeError + And the container state for "01TESTLIFECYCLE0000000160" should be "failed" + + # ── Direct rebuild_container coverage ─────────────────────── + + Scenario: rebuild_container directly stops then reactivates + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And a stopped container "01TESTLIFECYCLE0000000090" + When I call rebuild_container for "01TESTLIFECYCLE0000000090" at "/workspace" + Then the container state for "01TESTLIFECYCLE0000000090" should be "running" + + Scenario: rebuild_container rejects empty resource_id + When I call rebuild_container with empty resource_id + Then the rebuild should raise ValueError with "resource_id" + + Scenario: rebuild_container rejects empty workspace_folder + When I call rebuild_container with empty workspace_folder + Then the rebuild should raise ValueError with "workspace_folder" + + # ── Stop container error path coverage ───────────────────── + + Scenario: stop_container transitions to error when docker stop raises + Given a mock devcontainer CLI runner + And the runner configured to raise exception on stop + And an active container "01TESTLIFECYCLE0000000110" with container ID "ctr-sf" + When I attempt to stop container "01TESTLIFECYCLE0000000110" + Then the stop should raise RuntimeError + And the container state for "01TESTLIFECYCLE0000000110" should be "failed" + + # ── Activate generic exception path coverage ─────────────── + + Scenario: activate_container transitions to error on generic exception + Given a mock devcontainer CLI runner + And the runner configured to raise generic exception on up + When I attempt to activate container "01TESTLIFECYCLE0000000120" at "/workspace" + Then the activation should raise RuntimeError + And the container state for "01TESTLIFECYCLE0000000120" should be "failed" + + # ── Stop container non-zero return code (F2) ─────────────── + + Scenario: stop_container transitions to error when docker stop returns non-zero + Given a mock devcontainer CLI runner + And the runner configured for failed docker stop + And an active container "01TESTLIFECYCLE0000000170" with container ID "ctr-rc1" + When I attempt to stop container "01TESTLIFECYCLE0000000170" + Then the stop should raise RuntimeError + And the container state for "01TESTLIFECYCLE0000000170" should be "failed" + + # ── R20: workspace_folder path validation ────────────────── + + Scenario: Activate rejects relative workspace_folder + When I attempt to activate a container with relative workspace_folder + Then the activation should raise ValueError with "absolute path" + + # ── S2: Activation timeout transitions to failed ─────────── + + Scenario: Activation timeout transitions container to failed state + Given a mock devcontainer CLI runner that times out + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000350" + When I attempt to activate the timed-out container + Then the activation should raise RuntimeError with "timed out" + And the tracker "01TESTLIFECYCLE0000000350" should be in "failed" state + + # ── F4: Orphaned container cleanup on timeout ────────────── + + Scenario: Activation timeout attempts to stop orphaned containers + Given a mock devcontainer CLI runner that times out with orphan containers + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000360" + When I attempt to activate the timed-out container with orphan cleanup + Then the activation should raise RuntimeError with "timed out" + And the runner should have attempted orphan container cleanup + + # ── F6: R10 fix explicit assertion (container_id clearing) ─ + + Scenario: Stopped container has container_id cleared + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000380" with container ID "ctr-r10" + When I stop container "01TESTLIFECYCLE0000000380" + Then the container state for "01TESTLIFECYCLE0000000380" should be "stopped" + And the container_id for "01TESTLIFECYCLE0000000380" should be None + And the workspace_path for "01TESTLIFECYCLE0000000380" should be None + + Scenario: Failed activation clears container_id + Given a mock devcontainer CLI runner + And the runner configured for failed activation + When I attempt to activate container "01TESTLIFECYCLE0000000381" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000381" should be "failed" + And the container_id for "01TESTLIFECYCLE0000000381" should be None + + # ── R8-F2: stop_container early return on terminal state ──── + + Scenario: stop_container returns early when container already stopped + Given a mock devcontainer CLI runner + And a stopped container "01TESTLIFECYCLE0000000660" + When I stop container "01TESTLIFECYCLE0000000660" expecting early return + Then the container state for "01TESTLIFECYCLE0000000660" should be "stopped" + And the runner should not have received a docker stop call + + Scenario: stop_container returns early when container already failed + Given a mock devcontainer CLI runner + And an errored container "01TESTLIFECYCLE0000000670" + When I stop container "01TESTLIFECYCLE0000000670" expecting early return + Then the container state for "01TESTLIFECYCLE0000000670" should be "failed" + And the runner should not have received a docker stop call + + # ── F15: host_workspace_path stored on activation ─────────── + + Scenario: Activation stores host_workspace_path for health probes + Given a mock devcontainer CLI runner + And the runner configured for successful activation + When I activate container "01TESTLIFECYCLE0000000800" at "/workspace/project" + Then the container state should be "running" + And the host_workspace_path for "01TESTLIFECYCLE0000000800" should be "/workspace/project" + + # ── F10: rebuild passes --reset-container flag ────────────── + + Scenario: Rebuild passes --reset-container to devcontainer up + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And a stopped container "01TESTLIFECYCLE0000000810" + When I call rebuild_container for "01TESTLIFECYCLE0000000810" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000810" should be "running" + And the devcontainer up call should include "--reset-container" + + Scenario: Normal activation does not pass --reset-container + Given a mock devcontainer CLI runner + And the runner configured for successful activation + When I activate container "01TESTLIFECYCLE0000000811" at "/workspace/project" + Then the container state should be "running" + And the devcontainer up call should not include "--reset-container" diff --git a/features/devcontainer_cleanup.feature b/features/devcontainer_cleanup.feature new file mode 100644 index 000000000..3c9a98376 --- /dev/null +++ b/features/devcontainer_cleanup.feature @@ -0,0 +1,237 @@ +Feature: Devcontainer Session Cleanup and CLI Commands + As a CleverAgents developer + I want session-scoped container cleanup and CLI stop/rebuild commands + So that containers are cleaned up when sessions end and can be managed via CLI + + # ── Session cleanup ──────────────────────────────────────── + + Scenario: Active containers stopped on session cleanup + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000040" with container ID "ctr-a" + And an active container "01TESTLIFECYCLE0000000041" with container ID "ctr-b" + When I run session cleanup for session "session-001" + Then the container state for "01TESTLIFECYCLE0000000040" should be "stopped" + And the container state for "01TESTLIFECYCLE0000000041" should be "stopped" + + # ── Cleanup service integration ──────────────────────────── + + Scenario: CleanupService stop_active_devcontainers is callable and returns list + When I call CleanupService stop_active_devcontainers with no active containers + Then the cleanup result should be an empty list + And CleanupService should have a stop_active_devcontainers method + + Scenario: CleanupService stop_active_devcontainers calls handler cleanup + When I call CleanupService stop_active_devcontainers with no active containers + Then the cleanup result should be an empty list + + # ── stop_all_active_containers error path ────────────────── + + Scenario: Session cleanup handles stop failure gracefully + Given a mock devcontainer CLI runner + And the runner configured to raise exception on stop + And an active container "01TESTLIFECYCLE0000000130" with container ID "ctr-fail" + When I run session cleanup for session "session-err" + Then the cleanup should return 0 stopped containers + + # ── CLI stop/rebuild commands (F6) ───────────────────────── + + Scenario: CLI stop succeeds for active devcontainer-instance + Given a mock resource service with devcontainer "local/test-dc" id "01TESTLIFECYCLE0000000200" at "/workspace/project" + And an active container "01TESTLIFECYCLE0000000200" with container ID "ctr-cli-stop" + When I invoke CLI resource stop "local/test-dc" + Then the CLI exit code should be 0 + And the CLI output should contain "Stopped" + + Scenario: CLI stop rejects non-devcontainer resource type + Given a mock resource service with git-checkout "local/my-repo" id "01TESTLIFECYCLE0000000201" + When I invoke CLI resource stop "local/my-repo" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "not a stoppable container type" + + Scenario: CLI rebuild succeeds for stopped devcontainer-instance + Given a mock resource service with devcontainer "local/test-dc-rb" id "01TESTLIFECYCLE0000000210" at "/workspace/project" + And a stopped container "01TESTLIFECYCLE0000000210" + When I invoke CLI resource rebuild "local/test-dc-rb" + Then the CLI exit code should be 0 + And the CLI output should contain "Rebuilt" + + Scenario: CLI rebuild rejects resource with no location + Given a mock resource service with devcontainer "local/dc-no-loc" id "01TESTLIFECYCLE0000000211" with no location + And a stopped container "01TESTLIFECYCLE0000000211" + When I invoke CLI resource rebuild "local/dc-no-loc" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "no location" + + Scenario: CLI rebuild rejects non-devcontainer resource type + Given a mock resource service with git-checkout "local/git-rb" id "01TESTLIFECYCLE0000000212" + When I invoke CLI resource rebuild "local/git-rb" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "rebuild requires devcontainer" + + # ── R7: CLI test mock assertions ─────────────────────────── + + Scenario: CLI stop calls stop_container with correct resource_id + Given a mock resource service with devcontainer "local/test-dc-r7" id "01TESTLIFECYCLE0000000220" at "/workspace/project" + And an active container "01TESTLIFECYCLE0000000220" with container ID "ctr-r7" + When I invoke CLI resource stop "local/test-dc-r7" + Then the CLI exit code should be 0 + And the CLI stop mock should have been called with "01TESTLIFECYCLE0000000220" + + Scenario: CLI rebuild calls rebuild_container with correct args + Given a mock resource service with devcontainer "local/test-dc-r7rb" id "01TESTLIFECYCLE0000000221" at "/workspace/project" + And a stopped container "01TESTLIFECYCLE0000000221" + When I invoke CLI resource rebuild "local/test-dc-r7rb" + Then the CLI exit code should be 0 + And the CLI rebuild mock should have been called with "01TESTLIFECYCLE0000000221" and "/workspace/project" + + # ── R8: DevcontainerHandler coverage ─────────────────────── + + Scenario: DevcontainerHandler has expected class attributes and is instantiable + Then DevcontainerHandler should have _default_strategy "none" + And DevcontainerHandler should have _type_label "devcontainer" + And DevcontainerHandler should be instantiable + + Scenario: DevcontainerHandler extends BaseResourceHandler and inherits resolve interface + Then DevcontainerHandler should be a subclass of BaseResourceHandler + And DevcontainerHandler instance should have a resolve method + + # ── R12: CLI state precondition validation ───────────────── + + Scenario: CLI stop rejects container not in running state + Given a mock resource service with devcontainer "local/dc-det" id "01TESTLIFECYCLE0000000230" at "/workspace/project" + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000230" + When I invoke CLI resource stop "local/dc-det" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Cannot stop" + + Scenario: CLI rebuild rejects container in running state + Given a mock resource service with devcontainer "local/dc-run" id "01TESTLIFECYCLE0000000231" at "/workspace/project" + And an active container "01TESTLIFECYCLE0000000231" with container ID "ctr-rb-state" + When I invoke CLI resource rebuild "local/dc-run" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Cannot rebuild" + + # ── R14: stop_all return list assertion ──────────────────── + + Scenario: Session cleanup returns list of stopped resource IDs + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000250" with container ID "ctr-r14a" + And an active container "01TESTLIFECYCLE0000000251" with container ID "ctr-r14b" + When I run session cleanup for session "session-r14" + Then the cleanup stopped list should contain "01TESTLIFECYCLE0000000250" + And the cleanup stopped list should contain "01TESTLIFECYCLE0000000251" + + # ── A2: Stop and rebuild accept --yes flag ───────────────── + + Scenario: CLI stop with --yes skips confirmation and calls stop_container + Given a CLI-mockable running devcontainer resource "test-dc-yes-stop" + When I invoke CLI resource stop "test-dc-yes-stop" + Then the devcontainer CLI exit code should be 0 + And the CLI stop mock should have been called once + + Scenario: CLI rebuild with --yes skips confirmation and calls rebuild_container + Given a CLI-mockable stopped devcontainer resource "test-dc-yes-rebuild" + When I invoke CLI resource rebuild "test-dc-yes-rebuild" + Then the devcontainer CLI exit code should be 0 + And the CLI rebuild mock should have been called once + + # ── F5/F19: container-instance is NOT stoppable via CLI ───── + + Scenario: CLI stop rejects container-instance type (F19 fix) + Given a mock resource service with container-instance "local/ctr-stop" id "01TESTLIFECYCLE0000000370" at "/workspace/project" + And an active container "01TESTLIFECYCLE0000000370" with container ID "ctr-ci-stop" + When I invoke CLI resource stop "local/ctr-stop" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "not a stoppable" + + # ── F6-r6: CLI handler-level error paths ──────────────────── + + Scenario: CLI stop handles NotFoundError from show_resource + Given a mock resource service that raises NotFoundError for "local/missing" + When I invoke CLI resource stop "local/missing" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Resource not found" + + Scenario: CLI rebuild handles NotFoundError from show_resource + Given a mock resource service that raises NotFoundError for "local/missing-rb" + When I invoke CLI resource rebuild "local/missing-rb" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Resource not found" + + Scenario: CLI stop handles RuntimeError from stop_container + Given a mock resource service with devcontainer "local/dc-rterr" id "01TESTLIFECYCLE0000000500" at "/workspace/project" + And an active container "01TESTLIFECYCLE0000000500" with container ID "ctr-rterr" + And CLI stop_container mock that raises RuntimeError + When I invoke CLI resource stop with error mock "local/dc-rterr" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Stop failed" + + Scenario: CLI rebuild handles RuntimeError from rebuild_container + Given a mock resource service with devcontainer "local/dc-rberr" id "01TESTLIFECYCLE0000000501" at "/workspace/project" + And a stopped container "01TESTLIFECYCLE0000000501" + And CLI rebuild_container mock that raises RuntimeError + When I invoke CLI resource rebuild with error mock "local/dc-rberr" + Then the devcontainer CLI exit code should be non-zero + And the CLI output should contain "Rebuild failed" + + # ── R7-F1: Session-scoped container cleanup ──────────────── + + Scenario: Session cleanup only stops containers belonging to that session + Given a mock devcontainer CLI runner + And a session-scoped active container "01TESTLIFECYCLE0000000600" with ID "ctr-s1" session "sess-A" + And a session-scoped active container "01TESTLIFECYCLE0000000601" with ID "ctr-s2" session "sess-B" + When I run session cleanup for session "sess-A" + Then the container state for "01TESTLIFECYCLE0000000600" should be "stopped" + And the container state for "01TESTLIFECYCLE0000000601" should be "running" + + Scenario: Session cleanup with no session_id stops all containers + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000610" with container ID "ctr-all1" + And an active container "01TESTLIFECYCLE0000000611" with container ID "ctr-all2" + When I run session cleanup with no session filter + Then the container state for "01TESTLIFECYCLE0000000610" should be "stopped" + And the container state for "01TESTLIFECYCLE0000000611" should be "stopped" + + # ── R8-F1: Eviction wired into production cleanup ──────────── + + Scenario: Session cleanup triggers terminal tracker eviction + Given a mock devcontainer CLI runner + And 210 stopped container trackers in the registry + And an active container "01TESTLIFECYCLE0000000650" with container ID "ctr-evict" + When I run session cleanup for session "session-evict" + Then the container state for "01TESTLIFECYCLE0000000650" should be "stopped" + And the registry should have at most 200 terminal trackers + + # ── TEST-3: Coverage gap scenarios ────────────────────────── + + Scenario: list_active_containers_for_session returns only matching session + Given a mock devcontainer CLI runner + And a session-scoped active container "01TESTLIFECYCLE0000000700" with ID "ctr-ts3a" session "sess-X" + And a session-scoped active container "01TESTLIFECYCLE0000000701" with ID "ctr-ts3b" session "sess-Y" + When I list active containers for session "sess-X" + Then the session container list should contain "01TESTLIFECYCLE0000000700" + And the session container list should not contain "01TESTLIFECYCLE0000000701" + + Scenario: list_active_containers_for_session with empty session returns all + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000710" with container ID "ctr-ts3c" + And an active container "01TESTLIFECYCLE0000000711" with container ID "ctr-ts3d" + When I list active containers for an empty session + Then the session container list should contain "01TESTLIFECYCLE0000000710" + And the session container list should contain "01TESTLIFECYCLE0000000711" + + Scenario: list_active_containers_for_session includes unscoped containers + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000720" with container ID "ctr-unscoped" + And a session-scoped active container "01TESTLIFECYCLE0000000721" with ID "ctr-scoped" session "sess-Z" + When I list active containers for session "sess-Z" + Then the session container list should contain "01TESTLIFECYCLE0000000720" + And the session container list should contain "01TESTLIFECYCLE0000000721" + + # ── R7-F4: Session close cleanup without session service ──── + + Scenario: Session close cleans up containers even without session service + Given a facade with no session service + And a session-scoped active container "01TESTLIFECYCLE0000000630" with no docker ID session "sess-f4" + When I close session "sess-f4" via the facade + Then the container state for "01TESTLIFECYCLE0000000630" should be "stopped" diff --git a/features/devcontainer_handler.feature b/features/devcontainer_handler.feature index 094626824..e2f6e1014 100644 --- a/features/devcontainer_handler.feature +++ b/features/devcontainer_handler.feature @@ -85,7 +85,7 @@ Feature: Devcontainer Resource Handler Scenario: DevcontainerHandler has correct default strategy When I instantiate DevcontainerHandler - Then its default strategy should be "snapshot" + Then its default strategy should be "none" Scenario: DevcontainerHandler has correct type label When I instantiate DevcontainerHandler diff --git a/features/devcontainer_health_check.feature b/features/devcontainer_health_check.feature new file mode 100644 index 000000000..a419ec483 --- /dev/null +++ b/features/devcontainer_health_check.feature @@ -0,0 +1,126 @@ +Feature: Devcontainer Health Checking + As a CleverAgents developer + I want running devcontainers to be health-checked periodically + So that unhealthy containers are detected and transitioned to failed state + + # ── Health check ─────────────────────────────────────────── + + Scenario: Health check failure transitions to error + Given a mock devcontainer CLI runner + And the runner configured for failed health check + And an active container "01TESTLIFECYCLE0000000050" with container ID "ctr-hc" + When I run a single health check probe for "01TESTLIFECYCLE0000000050" + Then the container state for "01TESTLIFECYCLE0000000050" should be "failed" + + # ── start_health_check and health_check_loop coverage ────── + + Scenario: Health check loop success path runs and returns + Given a mock devcontainer CLI runner + And the runner configured for successful health check + And an active container "01TESTLIFECYCLE0000000100" with container ID "ctr-hcbg" + When I run one iteration of the health check loop for "01TESTLIFECYCLE0000000100" + Then the health check should have probed "01TESTLIFECYCLE0000000100" + And the container state for "01TESTLIFECYCLE0000000100" should be "running" + + Scenario: Health check loop transitions to error on probe failure + Given a mock devcontainer CLI runner + And the runner configured for failed health check + And an active container "01TESTLIFECYCLE0000000101" with container ID "ctr-hcf" + When I run one iteration of the health check loop for "01TESTLIFECYCLE0000000101" + Then the container state for "01TESTLIFECYCLE0000000101" should be "failed" + + Scenario: Health check loop transitions to error on exception + Given a mock devcontainer CLI runner + And the runner configured to raise exception on exec + And an active container "01TESTLIFECYCLE0000000102" with container ID "ctr-hce" + When I run one iteration of the health check loop for "01TESTLIFECYCLE0000000102" + Then the container state for "01TESTLIFECYCLE0000000102" should be "failed" + + Scenario: start_health_check registers thread and stop event + Given a mock devcontainer CLI runner + And the runner configured for successful health check + And an active container "01TESTLIFECYCLE0000000103" with container ID "ctr-reg" + When I call start_health_check for "01TESTLIFECYCLE0000000103" + Then a health check thread should be registered for "01TESTLIFECYCLE0000000103" + + Scenario: Stopping active container stops running health check + Given a mock devcontainer CLI runner + And the runner configured for successful health check + And an active container "01TESTLIFECYCLE0000000104" with container ID "ctr-hcstop" + And a registered health check for "01TESTLIFECYCLE0000000104" + When I stop container "01TESTLIFECYCLE0000000104" + Then the container state for "01TESTLIFECYCLE0000000104" should be "stopped" + And the health check for "01TESTLIFECYCLE0000000104" should be unregistered + + Scenario: Health check exits when container is no longer active + Given a mock devcontainer CLI runner + And the runner configured for successful health check + And a stopped container "01TESTLIFECYCLE0000000105" + When I run one iteration of the health check loop for "01TESTLIFECYCLE0000000105" + Then the container state for "01TESTLIFECYCLE0000000105" should be "stopped" + + Scenario: start_health_check rejects empty resource_id + When I attempt to start health check with empty resource_id + Then the health check should raise ValueError + + # ── T2: Concurrent activation rejects second caller ──────── + + Scenario: Concurrent activation of the same container is rejected + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000310" + When I activate the container concurrently from two threads + Then exactly one activation should succeed + And exactly one activation should fail with ValueError + + # ── T3: _single_probe uses /workspace fallback ───────────── + + Scenario: Single probe falls back to /workspace when workspace_path is None + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000320" with container ID "ctr-t3" + When I run a single probe with no workspace_path set + Then the probe command should use "/workspace" as workspace folder + + # ── T4: Health check thread join timeout ─────────────────── + + Scenario: Stopping health check joins thread with timeout + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000330" with container ID "ctr-t4" + And a running health check for "01TESTLIFECYCLE0000000330" + When I stop the health check for "01TESTLIFECYCLE0000000330" + Then the health check thread should have been joined + + # ── F7: Concurrent stop_container ────────────────────────── + + Scenario: Concurrent stop of the same container is handled safely + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000390" with container ID "ctr-cs" + When I stop the container concurrently from two threads + Then both stops should succeed + And the container state for "01TESTLIFECYCLE0000000390" should be "stopped" + And exactly 1 docker stop call should have been made + + # ── R7-F3: Auto health check on activation ───────────────── + + Scenario: Activation automatically starts health check + Given a mock devcontainer CLI runner + And the runner configured for successful activation + When I activate container "01TESTLIFECYCLE0000000620" at "/workspace/project" + Then the container state should be "running" + And a health check thread should be registered for "01TESTLIFECYCLE0000000620" + + # ── F16: resolve() triggers lazy activation ───────────────── + + Scenario: DevcontainerHandler.resolve triggers lazy activation for detected resource + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000820" + When I resolve a devcontainer-instance "01TESTLIFECYCLE0000000820" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000820" should be "running" + + Scenario: DevcontainerHandler.resolve skips activation for running resource + Given a mock devcontainer CLI runner + And an active container "01TESTLIFECYCLE0000000821" with container ID "ctr-resolve" + When I resolve a devcontainer-instance "01TESTLIFECYCLE0000000821" at "/workspace/project" + Then the container state for "01TESTLIFECYCLE0000000821" should be "running" + And the runner should not have received a devcontainer up call diff --git a/features/devcontainer_state.feature b/features/devcontainer_state.feature new file mode 100644 index 000000000..859de4093 --- /dev/null +++ b/features/devcontainer_state.feature @@ -0,0 +1,198 @@ +Feature: Devcontainer Lifecycle State Model + As a CleverAgents developer + I want the container lifecycle state model to enforce valid transitions + So that containers follow a predictable state machine + + # ── Lifecycle state model ────────────────────────────────── + + Scenario: ContainerLifecycleState has all required values + When I inspect ContainerLifecycleState enum values + Then the lifecycle enum should contain "detected" + And the lifecycle enum should contain "building" + And the lifecycle enum should contain "running" + And the lifecycle enum should contain "stopping" + And the lifecycle enum should contain "stopped" + And the lifecycle enum should contain "failed" + + Scenario: Valid transitions are accepted + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000001" + When I transition from "detected" to "building" + Then the transition should succeed + And the tracker state should be "building" + + Scenario: Active transition after starting + Given a lifecycle tracker in "building" state for resource "01TESTLIFECYCLE0000000002" + When I transition from "building" to "running" + Then the transition should succeed + And the tracker state should be "running" + + Scenario: Invalid transition is rejected + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000003" + When I attempt to transition from "detected" to "running" + Then the transition should raise ValueError + + Scenario: Stopping to error transition is valid + Given a lifecycle tracker in "stopping" state for resource "01TESTLIFECYCLE0000000004" + When I transition from "stopping" to "failed" + Then the transition should succeed + And the tracker state should be "failed" + + Scenario: Error to starting transition is valid for retry + Given a lifecycle tracker in "failed" state for resource "01TESTLIFECYCLE0000000005" + When I transition from "failed" to "building" + Then the transition should succeed + And the tracker state should be "building" + + Scenario: Stopped to starting transition is valid for rebuild + Given a lifecycle tracker in "stopped" state for resource "01TESTLIFECYCLE0000000006" + When I transition from "stopped" to "building" + Then the transition should succeed + + Scenario: Transition history is recorded + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000007" + When I transition from "detected" to "building" + Then the tracker should have 1 transition in history + + # ── Lifecycle registry ───────────────────────────────────── + + Scenario: Lifecycle tracker persists state across calls + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000060" + When I transition from "detected" to "building" + And I retrieve the tracker for "01TESTLIFECYCLE0000000060" + Then the retrieved tracker state should be "building" + + Scenario: List active containers returns only active ones + Given a mock devcontainer CLI runner + And the runner configured for successful activation + And an active container "01TESTLIFECYCLE0000000070" with container ID "ctr-x" + And a lifecycle tracker for resource "01TESTLIFECYCLE0000000071" + When I list active containers + Then the active list should contain "01TESTLIFECYCLE0000000070" + And the active list should not contain "01TESTLIFECYCLE0000000071" + + # ── JSON output parsing ──────────────────────────────────── + + Scenario: Parse valid devcontainer up JSON output + When I parse devcontainer up output with containerId "abcdef123456" and workspace "/ws" + Then the parsed container ID should be "abcdef123456" + And the parsed workspace path should be "/ws" + + Scenario: Parse invalid devcontainer up JSON output gracefully + When I parse devcontainer up output with invalid JSON + Then the parsed container ID should be None + And the parsed workspace path should be None + + # ── Argument validation ──────────────────────────────────── + + Scenario: validate_transition rejects wrong types + When I call validate_transition with non-enum arguments + Then it should raise TypeError + + Scenario: validate_transition rejects wrong to_state type + When I call validate_transition with non-enum to_state + Then it should raise TypeError + + Scenario: transition_state rejects wrong tracker type + When I call transition_state with non-tracker argument + Then it should raise TypeError + + Scenario: transition_state rejects wrong to_state type + When I call transition_state with non-enum to_state argument + Then it should raise TypeError + + # ── Registry edge cases ──────────────────────────────────── + + Scenario: get_lifecycle_tracker rejects empty resource_id + When I attempt to get tracker with empty resource_id + Then the get tracker should raise ValueError + + Scenario: set_lifecycle_tracker rejects wrong type + When I attempt to set tracker with wrong type + Then the set tracker should raise TypeError + + # ── R13: Additional invalid transition coverage ──────────── + + Scenario: Invalid transition running to building is rejected + Given a lifecycle tracker in "running" state for resource "01TESTLIFECYCLE0000000240" + When I attempt to transition from "running" to "building" + Then the transition should raise ValueError + + Scenario: Invalid transition stopped to running is rejected + Given a lifecycle tracker in "stopped" state for resource "01TESTLIFECYCLE0000000241" + When I attempt to transition from "stopped" to "running" + Then the transition should raise ValueError + + Scenario: Invalid transition building to stopped is rejected + Given a lifecycle tracker in "building" state for resource "01TESTLIFECYCLE0000000242" + When I attempt to transition from "building" to "stopped" + Then the transition should raise ValueError + + Scenario: Invalid transition failed to running is rejected + Given a lifecycle tracker in "failed" state for resource "01TESTLIFECYCLE0000000243" + When I attempt to transition from "failed" to "running" + Then the transition should raise ValueError + + Scenario: Invalid transition detected to stopping is rejected + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000244" + When I attempt to transition from "detected" to "stopping" + Then the transition should raise ValueError + + Scenario: Invalid transition building to detected is rejected + Given a lifecycle tracker in "building" state for resource "01TESTLIFECYCLE0000000245" + When I attempt to transition from "building" to "detected" + Then the transition should raise ValueError + + # ── R4: Multi-line JSON output parsing ───────────────────── + + Scenario: Parse devcontainer up output with log lines before JSON + When I parse devcontainer up output with log lines before JSON + Then the parsed container ID should be "aabbccddee0099887766" + And the parsed workspace path should be "/workspaces/multi" + + # ── T1: Transition history cap at 100 ────────────────────── + + Scenario: Transition history is capped at 100 entries + Given a lifecycle tracker for resource "01TESTLIFECYCLE0000000300" + When I perform 105 transitions cycling through valid states + Then the tracker should have at most 100 transitions + + # ── S1: Container ID format validation ───────────────────── + + Scenario: Invalid container ID format is rejected during parsing + When I parse devcontainer up output with invalid container ID "not-a-hex-id!" + Then the parsed container ID should be empty + + # ── F11: Invalid workspace path rejected during parsing ──── + + Scenario: Parse devcontainer up output with relative workspace path + When I parse devcontainer up output with relative workspace "relative/ws" + Then the parsed container ID should be "aabbccddee0011223344" + And the parsed workspace path should be None + + # ── R7-F8: Terminal tracker eviction ──────────────────────── + + Scenario: Terminal tracker eviction removes oldest stopped trackers + Given 210 stopped container trackers in the registry + When I run terminal tracker eviction + Then the registry should have at most 200 terminal trackers + + # ── TEST-3: Coverage gap scenarios ────────────────────────── + + Scenario: evict_terminal_trackers returns 0 when below threshold + Given 50 stopped container trackers in the registry + When I run terminal tracker eviction + Then the eviction count should be 0 + + Scenario: get_lifecycle_tracker auto-creates tracker for new resource + When I get tracker for new resource "01TESTLIFECYCLE0000000730" + Then the auto-created tracker state should be "detected" + And the auto-created tracker resource_id should be "01TESTLIFECYCLE0000000730" + + Scenario: clear_lifecycle_registry stops health check threads + Given a mock devcontainer CLI runner + And the runner configured for successful health check + And an active container "01TESTLIFECYCLE0000000740" with container ID "ctr-clr" + And a running health check for "01TESTLIFECYCLE0000000740" + When I clear the lifecycle registry + Then the registry should be empty + And the health check for "01TESTLIFECYCLE0000000740" should be unregistered diff --git a/features/environment.py b/features/environment.py index 31c6f9b42..e71e026f8 100644 --- a/features/environment.py +++ b/features/environment.py @@ -281,6 +281,18 @@ def before_scenario(context, scenario): os.environ[env_var] = f"sqlite:///{db_path}" context._scenario_db_paths.append(db_path) + # Clear devcontainer lifecycle registry between scenarios to prevent + # test pollution from in-memory lifecycle trackers and health check + # threads left by previous scenarios. + try: + from cleveragents.resource.handlers.devcontainer import ( + clear_lifecycle_registry, + ) + + clear_lifecycle_registry() + except ImportError: + pass # Handler not available in all environments + # Re-apply mock AI provider after container reset try: from cleveragents.application.container import override_providers diff --git a/features/mocks/mock_devcontainer_cli.py b/features/mocks/mock_devcontainer_cli.py new file mode 100644 index 000000000..00a995b25 --- /dev/null +++ b/features/mocks/mock_devcontainer_cli.py @@ -0,0 +1,209 @@ +"""Mock subprocess runner for devcontainer CLI commands. + +Provides configurable mock responses for ``devcontainer up``, +``devcontainer exec``, and ``docker stop`` commands used in +lifecycle BDD tests. + +All mocks reside in ``features/mocks/`` per project convention. +""" + +from __future__ import annotations + +import json +from dataclasses import dataclass, field + + +@dataclass +class MockProcessResult: + """Mimics ``subprocess.CompletedProcess`` for test assertions.""" + + returncode: int = 0 + stdout: str = "" + stderr: str = "" + args: list[str] = field(default_factory=list) + + +class MockDevcontainerRunner: + """Configurable mock for subprocess.run calls. + + Records all invocations and returns pre-configured results + based on the command being run. + + Usage:: + + runner = MockDevcontainerRunner() + runner.set_up_result(container_id="abcdef012345", workspace="/ws") + # pass runner as ``run_command=runner`` in handler calls + + Attributes: + calls: List of all recorded (args, kwargs) invocations. + up_result: Result returned for ``devcontainer up`` commands. + exec_result: Result returned for ``devcontainer exec`` commands. + stop_result: Result returned for ``docker stop`` commands. + """ + + def __init__(self) -> None: + self.calls: list[tuple[list[str], dict[str, object]]] = [] + self.up_result: MockProcessResult = MockProcessResult() + self.exec_result: MockProcessResult = MockProcessResult() + self.stop_result: MockProcessResult = MockProcessResult() + + #: TEST-4 fix: expected subprocess kwargs that production code must pass. + #: If production code sets ``check=True`` (causing CalledProcessError on + #: non-zero rc) or omits ``capture_output``/``text``, this mock will + #: flag the mismatch. + _EXPECTED_KWARGS = frozenset({"capture_output", "text", "timeout", "check"}) + + def __call__( + self, + args: list[str], + **kwargs: object, + ) -> MockProcessResult: + """Handle a subprocess.run invocation. + + Args: + args: Command-line arguments. + **kwargs: Additional subprocess kwargs — validated against + expected production kwargs (TEST-4 fix). + + Returns: + Mock result based on the command. + + Raises: + AssertionError: If required kwargs are missing or ``check`` + is set to ``True`` (which would raise + ``CalledProcessError`` in production). + """ + self.calls.append((list(args), dict(kwargs))) + + # TEST-4 fix: validate subprocess kwargs. + missing = self._EXPECTED_KWARGS - set(kwargs) + if missing: + raise AssertionError( + f"MockDevcontainerRunner: missing expected subprocess kwargs " + f"{missing} in call {args}" + ) + if kwargs.get("check") is True: + raise AssertionError( + f"MockDevcontainerRunner: check=True would raise " + f"CalledProcessError in production for {args}" + ) + + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "up": + return self.up_result + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "exec": + return self.exec_result + if len(args) >= 2 and args[0] == "docker" and args[1] == "stop": + return self.stop_result + + return MockProcessResult(returncode=0) + + def set_up_result( + self, + *, + container_id: str = "aabbccddee0011223344", + workspace: str = "/workspaces/project", + returncode: int = 0, + stderr: str = "", + ) -> None: + """Configure the result for ``devcontainer up``. + + Args: + container_id: Container ID in the JSON output. + workspace: Workspace path in the JSON output. + returncode: Exit code. + stderr: Stderr text. + """ + output = json.dumps( + { + "outcome": "success" if returncode == 0 else "error", + "containerId": container_id, + "remoteWorkspaceFolder": workspace, + } + ) + self.up_result = MockProcessResult( + returncode=returncode, + stdout=output, + stderr=stderr, + ) + + def set_up_no_container_id(self) -> None: + """Configure ``devcontainer up`` to succeed without a containerId.""" + output = json.dumps({"outcome": "success"}) + self.up_result = MockProcessResult( + returncode=0, + stdout=output, + stderr="", + ) + + def set_up_failure(self, stderr: str = "container build failed") -> None: + """Configure ``devcontainer up`` to fail. + + Args: + stderr: Error message. + """ + self.up_result = MockProcessResult( + returncode=1, + stdout="", + stderr=stderr, + ) + + def set_exec_result(self, *, returncode: int = 0, stdout: str = "ping") -> None: + """Configure the result for ``devcontainer exec``. + + Args: + returncode: Exit code. + stdout: Stdout text. + """ + self.exec_result = MockProcessResult( + returncode=returncode, + stdout=stdout, + ) + + def set_exec_failure(self) -> None: + """Configure ``devcontainer exec`` to fail.""" + self.exec_result = MockProcessResult( + returncode=1, + stdout="", + stderr="container not responding", + ) + + def set_stop_result(self, *, returncode: int = 0) -> None: + """Configure the result for ``docker stop``. + + Args: + returncode: Exit code. + """ + self.stop_result = MockProcessResult(returncode=returncode) + + @property + def call_count(self) -> int: + """Total number of invocations.""" + return len(self.calls) + + @property + def up_calls(self) -> list[tuple[list[str], dict[str, object]]]: + """Calls to ``devcontainer up``.""" + return [ + (a, k) + for a, k in self.calls + if len(a) >= 2 and a[0] == "devcontainer" and a[1] == "up" + ] + + @property + def exec_calls(self) -> list[tuple[list[str], dict[str, object]]]: + """Calls to ``devcontainer exec``.""" + return [ + (a, k) + for a, k in self.calls + if len(a) >= 2 and a[0] == "devcontainer" and a[1] == "exec" + ] + + @property + def stop_calls(self) -> list[tuple[list[str], dict[str, object]]]: + """Calls to ``docker stop``.""" + return [ + (a, k) + for a, k in self.calls + if len(a) >= 2 and a[0] == "docker" and a[1] == "stop" + ] diff --git a/features/steps/devcontainer_activation_steps.py b/features/steps/devcontainer_activation_steps.py new file mode 100644 index 000000000..050e072e1 --- /dev/null +++ b/features/steps/devcontainer_activation_steps.py @@ -0,0 +1,494 @@ +"""Step definitions for devcontainer activation, stop, and rebuild. + +Covers: lazy activation (success/failure/timeout/orphan), manual stop, +rebuild, concurrent activation and stop, argument validation for +activate/stop/rebuild, container_id clearing, and early-return on +terminal state. + +All mocks are in ``features/mocks/mock_devcontainer_cli.py``. +""" + +from __future__ import annotations + +import json +import subprocess +from typing import cast + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, +) +from cleveragents.resource.handlers.devcontainer import ( + activate_container, + get_lifecycle_tracker, + rebuild_container, + set_lifecycle_tracker, + stop_container, +) +from features.mocks.mock_devcontainer_cli import ( + MockDevcontainerRunner, + MockProcessResult, +) + +# ── Given steps ────────────────────────────────────────────── + + +@given("a mock devcontainer CLI runner") +def step_create_mock_runner(context: Context) -> None: + context.mock_runner = MockDevcontainerRunner() + + +@given("the runner configured for successful activation") +def step_configure_success(context: Context) -> None: + context.mock_runner.set_up_result( + container_id="aabbccddee0011223344", + workspace="/workspaces/project", + ) + + +@given("the runner configured for failed activation") +def step_configure_failure(context: Context) -> None: + context.mock_runner.set_up_failure(stderr="container build failed") + + +@given('an active container "{resource_id}" with container ID "{ctr_id}"') +def step_create_active_container( + context: Context, resource_id: str, ctr_id: str +) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.RUNNING, + container_id=ctr_id, + workspace_path="/workspaces/project", + ) + set_lifecycle_tracker(tracker) + + +@given('a stopped container "{resource_id}"') +def step_create_stopped_container(context: Context, resource_id: str) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.STOPPED, + ) + set_lifecycle_tracker(tracker) + + +@given('an errored container "{resource_id}"') +def step_create_errored_container(context: Context, resource_id: str) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.FAILED, + ) + set_lifecycle_tracker(tracker) + + +@given('an active container with no container_id "{resource_id}"') +def step_create_active_no_ctr_id(context: Context, resource_id: str) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.RUNNING, + container_id=None, + workspace_path="/workspaces/project", + ) + set_lifecycle_tracker(tracker) + + +@given("the runner configured for activation with no container_id") +def step_configure_activation_no_ctr_id(context: Context) -> None: + """Configure devcontainer up to succeed but return no containerId.""" + context.mock_runner.up_result = MockProcessResult( + returncode=0, + stdout=json.dumps({"outcome": "success"}), + stderr="", + ) + + +@given("the runner configured for failed docker stop") +def step_configure_stop_failure_rc(context: Context) -> None: + """Configure docker stop to return a non-zero exit code (not raise).""" + context.mock_runner.set_stop_result(returncode=1) + + +@given("the runner configured to raise exception on stop") +def step_configure_stop_exception(context: Context) -> None: + """Replace mock_runner with a wrapper that raises on docker stop.""" + inner = context.mock_runner + + def raising_runner(args: list[str], **kwargs: object) -> object: + inner.calls.append((list(args), dict(kwargs))) + if len(args) >= 2 and args[0] == "docker" and args[1] == "stop": + raise OSError("docker daemon not running") + return inner(args, **kwargs) + + context.mock_runner = cast(MockDevcontainerRunner, raising_runner) + + +@given("the runner configured to raise generic exception on up") +def step_configure_up_generic_exception(context: Context) -> None: + """Replace mock_runner with a wrapper that raises on devcontainer up.""" + inner = context.mock_runner + + def raising_runner(args: list[str], **kwargs: object) -> object: + inner.calls.append((list(args), dict(kwargs))) + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "up": + raise OSError("network unreachable") + return inner(args, **kwargs) + + context.mock_runner = cast(MockDevcontainerRunner, raising_runner) + + +@given("a mock devcontainer CLI runner that times out") +def step_create_timeout_runner(context: Context) -> None: + """Create a runner that raises subprocess.TimeoutExpired.""" + + def _timeout_runner(args: list[str], **kwargs: object) -> None: + raise subprocess.TimeoutExpired(cmd=args, timeout=300) + + context.mock_runner = _timeout_runner + + +@given("a mock devcontainer CLI runner that times out with orphan containers") +def step_create_timeout_orphan_runner(context: Context) -> None: + """Create a runner that times out on devcontainer up but returns + orphan container IDs on docker ps. + """ + call_log: list[tuple[list[str], dict[str, object]]] = [] + + def _runner(args: list[str], **kwargs: object) -> object: + call_log.append((list(args), dict(kwargs))) + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "up": + raise subprocess.TimeoutExpired(cmd=args, timeout=300) + if len(args) >= 2 and args[0] == "docker" and args[1] == "ps": + return MockProcessResult(returncode=0, stdout="orphan123abc\n", stderr="") + if len(args) >= 2 and args[0] == "docker" and args[1] == "stop": + return MockProcessResult(returncode=0) + return MockProcessResult(returncode=0) + + context.mock_runner = _runner + context.orphan_call_log = call_log + + +# ── When steps ─────────────────────────────────────────────── + + +@when('I activate container "{resource_id}" at "{workspace}"') +def step_activate(context: Context, resource_id: str, workspace: str) -> None: + context.activation_error = None + try: + context.activated_tracker = activate_container( + resource_id, + workspace, + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when('I attempt to activate container "{resource_id}" at "{workspace}"') +def step_attempt_activate(context: Context, resource_id: str, workspace: str) -> None: + context.activation_error = None + try: + context.activated_tracker = activate_container( + resource_id, + workspace, + run_command=getattr(context, "mock_runner", None), + ) + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when('I stop container "{resource_id}"') +def step_stop(context: Context, resource_id: str) -> None: + context.stop_error = None + try: + stop_container( + resource_id, + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.stop_error = exc + + +@when('I attempt to stop container "{resource_id}"') +def step_attempt_stop(context: Context, resource_id: str) -> None: + context.stop_error = None + try: + stop_container( + resource_id, + run_command=getattr(context, "mock_runner", None), + ) + except (RuntimeError, ValueError) as exc: + context.stop_error = exc + + +@when('I rebuild container "{resource_id}" at "{workspace}"') +def step_rebuild(context: Context, resource_id: str, workspace: str) -> None: + context.rebuild_error = None + try: + rebuild_container( + resource_id, + workspace, + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.rebuild_error = exc + + +@when('I call rebuild_container for "{resource_id}" at "{workspace}"') +def step_rebuild_direct(context: Context, resource_id: str, workspace: str) -> None: + context.rebuild_error = None + try: + rebuild_container( + resource_id, + workspace, + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.rebuild_error = exc + + +@when("I call rebuild_container with empty resource_id") +def step_rebuild_empty_resource_id(context: Context) -> None: + context.rebuild_error = None + try: + rebuild_container("", "/workspace") + except (RuntimeError, ValueError) as exc: + context.rebuild_error = exc + + +@when("I call rebuild_container with empty workspace_folder") +def step_rebuild_empty_workspace(context: Context) -> None: + context.rebuild_error = None + try: + rebuild_container("01TESTLIFECYCLE0000000091", "") + except (RuntimeError, ValueError) as exc: + context.rebuild_error = exc + + +@when("I attempt to activate a container with empty resource_id") +def step_activate_empty_resource_id(context: Context) -> None: + context.activation_error = None + try: + activate_container("", "/workspace") + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when("I attempt to activate a container with empty workspace_folder") +def step_activate_empty_workspace(context: Context) -> None: + context.activation_error = None + try: + activate_container("01TESTLIFECYCLE0000000080", "") + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when("I attempt to stop a container with empty resource_id") +def step_stop_empty_resource_id(context: Context) -> None: + context.stop_error = None + try: + stop_container("") + except (RuntimeError, ValueError) as exc: + context.stop_error = exc + + +@when("I attempt to activate a container with relative workspace_folder") +def step_activate_relative_workspace(context: Context) -> None: + context.activation_error = None + try: + activate_container("01TESTLIFECYCLE0000000080", "relative/path") + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when("I attempt to activate the timed-out container") +def step_activate_timeout(context: Context) -> None: + context.activation_error = None + try: + activate_container( + context.resource_id, + "/workspace", + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when("I attempt to activate the timed-out container with orphan cleanup") +def step_activate_timeout_orphan(context: Context) -> None: + context.activation_error = None + try: + activate_container( + context.resource_id, + "/workspace", + run_command=context.mock_runner, + ) + except (RuntimeError, ValueError) as exc: + context.activation_error = exc + + +@when('I stop container "{resource_id}" expecting early return') +def step_stop_container_early_return(context: Context, resource_id: str) -> None: + """Stop a container that is already in a terminal state.""" + context.stop_result = stop_container( + resource_id, + run_command=context.mock_runner, + ) + + +# ── Then steps ─────────────────────────────────────────────── + + +@then('the container state should be "{state}"') +def step_check_container_state(context: Context, state: str) -> None: + assert context.activated_tracker.current_state == ContainerLifecycleState(state) + + +@then('the container ID should be "{ctr_id}"') +def step_check_container_id(context: Context, ctr_id: str) -> None: + assert context.activated_tracker.container_id == ctr_id + + +@then('the workspace path should be "{ws_path}"') +def step_check_workspace_path(context: Context, ws_path: str) -> None: + assert context.activated_tracker.workspace_path == ws_path + + +@then("the runner should have received a devcontainer up call") +def step_check_up_call(context: Context) -> None: + assert len(context.mock_runner.up_calls) > 0 + + +@then("the activation should raise RuntimeError") +def step_check_activation_runtime_error(context: Context) -> None: + assert context.activation_error is not None + assert isinstance(context.activation_error, RuntimeError) + + +@then('the container state for "{resource_id}" should be "{state}"') +def step_check_specific_state(context: Context, resource_id: str, state: str) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.current_state == ContainerLifecycleState(state), ( + f"Expected {state}, got {tracker.current_state.value}" + ) + + +@then("the runner should have received a docker stop call") +def step_check_stop_call(context: Context) -> None: + assert len(context.mock_runner.stop_calls) > 0 + + +@then("the runner should not have received a docker stop call") +def step_check_no_stop_call(context: Context) -> None: + assert len(context.mock_runner.stop_calls) == 0, ( + f"Expected 0 docker stop calls, got {len(context.mock_runner.stop_calls)}" + ) + + +@then('the activation should raise ValueError with "{keyword}"') +def step_check_activation_value_error(context: Context, keyword: str) -> None: + assert context.activation_error is not None + assert isinstance(context.activation_error, ValueError) + assert keyword in str(context.activation_error) + + +@then('the stop should raise ValueError with "{keyword}"') +def step_check_stop_value_error(context: Context, keyword: str) -> None: + assert context.stop_error is not None + assert isinstance(context.stop_error, ValueError) + assert keyword in str(context.stop_error) + + +@then("the stop should raise RuntimeError") +def step_check_stop_runtime_error(context: Context) -> None: + assert context.stop_error is not None + assert isinstance(context.stop_error, RuntimeError) + + +@then('the rebuild should raise ValueError with "{keyword}"') +def step_check_rebuild_value_error(context: Context, keyword: str) -> None: + assert context.rebuild_error is not None + assert isinstance(context.rebuild_error, ValueError) + assert keyword in str(context.rebuild_error) + + +@then('the activation should raise RuntimeError with "{text}"') +def step_check_runtime_error(context: Context, text: str) -> None: + assert context.activation_error is not None, "Expected an error but none was raised" + assert isinstance(context.activation_error, RuntimeError), ( + f"Expected RuntimeError, got {type(context.activation_error).__name__}" + ) + assert text in str(context.activation_error), ( + f"Expected '{text}' in error message, got '{context.activation_error}'" + ) + + +@then('the tracker "{resource_id}" should be in "{state}" state') +def step_check_named_tracker_state( + context: Context, resource_id: str, state: str +) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.current_state == ContainerLifecycleState(state), ( + f"Expected state '{state}', got '{tracker.current_state.value}'" + ) + + +@then('the container_id for "{resource_id}" should be None') +def step_check_container_id_none(context: Context, resource_id: str) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.container_id is None, ( + f"Expected container_id None, got '{tracker.container_id}'" + ) + + +@then('the workspace_path for "{resource_id}" should be None') +def step_check_workspace_path_none(context: Context, resource_id: str) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.workspace_path is None, ( + f"Expected workspace_path None, got '{tracker.workspace_path}'" + ) + + +@then('the host_workspace_path for "{resource_id}" should be "{path}"') +def step_check_host_workspace_path( + context: Context, resource_id: str, path: str +) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.host_workspace_path == path, ( + f"Expected host_workspace_path '{path}', got '{tracker.host_workspace_path}'" + ) + + +@then('the devcontainer up call should include "{flag}"') +def step_check_up_call_includes_flag(context: Context, flag: str) -> None: + up_calls = context.mock_runner.up_calls + assert len(up_calls) > 0, "No devcontainer up calls recorded" + args, _ = up_calls[-1] + assert flag in args, f"Expected '{flag}' in {args}" + + +@then('the devcontainer up call should not include "{flag}"') +def step_check_up_call_excludes_flag(context: Context, flag: str) -> None: + up_calls = context.mock_runner.up_calls + assert len(up_calls) > 0, "No devcontainer up calls recorded" + args, _ = up_calls[-1] + assert flag not in args, f"Did not expect '{flag}' in {args}" + + +@then("the runner should have attempted orphan container cleanup") +def step_check_orphan_cleanup(context: Context) -> None: + ps_calls = [ + (a, k) + for a, k in context.orphan_call_log + if len(a) >= 2 and a[0] == "docker" and a[1] == "ps" + ] + assert len(ps_calls) >= 1, "Expected docker ps call for orphan lookup" + stop_calls = [ + (a, k) + for a, k in context.orphan_call_log + if len(a) >= 2 and a[0] == "docker" and a[1] == "stop" + ] + assert len(stop_calls) >= 1, "Expected docker stop call for orphan cleanup (F1 fix)" diff --git a/features/steps/devcontainer_cleanup_steps.py b/features/steps/devcontainer_cleanup_steps.py new file mode 100644 index 000000000..747ea3d69 --- /dev/null +++ b/features/steps/devcontainer_cleanup_steps.py @@ -0,0 +1,465 @@ +"""Step definitions for session cleanup, CLI stop/rebuild, and registry. + +Covers: session cleanup (stop_all_active_containers), CleanupService +integration, CLI stop/rebuild commands (F6), CLI mock assertions (R7), +DevcontainerHandler class coverage (R8), CLI state precondition +validation (R12), container-instance CLI stop (F5), session-scoped +cleanup (R7-F1), auto health check on activation (R7-F3), terminal +tracker eviction wired to cleanup (R8-F1), session-close facade +(R7-F4), NotFoundError paths, and RuntimeError paths. + +All mocks are in ``features/mocks/mock_devcontainer_cli.py``. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context +from typer.testing import CliRunner + +from cleveragents.acp.facade import AcpLocalFacade +from cleveragents.acp.models import AcpRequest +from cleveragents.application.services.cleanup_service import CleanupService +from cleveragents.cli.commands.resource import app as resource_app +from cleveragents.core.exceptions import NotFoundError +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, +) +from cleveragents.domain.models.core.resource import SandboxStrategy +from cleveragents.resource.handlers._base import BaseResourceHandler +from cleveragents.resource.handlers.devcontainer import ( + DevcontainerHandler, + clear_lifecycle_registry, + list_active_containers_for_session, + set_lifecycle_tracker, + stop_all_active_containers, +) + +# ── CLI patch targets ──────────────────────────────────────── + +_CLI_PATCH_SERVICE = "cleveragents.cli.commands.resource._get_registry_service" +_CLI_PATCH_STOP = "cleveragents.cli.commands.resource.stop_container" +_CLI_PATCH_REBUILD = "cleveragents.cli.commands.resource.rebuild_container" + + +# ── Helpers ────────────────────────────────────────────────── + + +def _make_mock_resource( + *, + resource_id: str, + name: str, + resource_type_name: str, + location: str | None = None, + properties: dict[str, object] | None = None, +) -> MagicMock: + """Create a mock resource object matching the CLI expectations.""" + res = MagicMock() + res.resource_id = resource_id + res.name = name + res.resource_type_name = resource_type_name + res.location = location + res.properties = properties + return res + + +# ── Given steps ────────────────────────────────────────────── + + +@given( + 'a mock resource service with devcontainer "{name}" ' + 'id "{resource_id}" at "{location}"' +) +def step_mock_service_devcontainer( + context: Context, name: str, resource_id: str, location: str +) -> None: + """Set up a mock registry service returning a devcontainer resource.""" + mock_service = MagicMock() + loc = location if location else None + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="devcontainer-instance", + location=loc, + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + + +@given( + 'a mock resource service with devcontainer "{name}" ' + 'id "{resource_id}" with no location' +) +def step_mock_service_devcontainer_no_loc( + context: Context, name: str, resource_id: str +) -> None: + """Set up a mock registry service returning a devcontainer with no location.""" + mock_service = MagicMock() + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="devcontainer-instance", + location=None, + properties=None, + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + + +@given('a mock resource service with git-checkout "{name}" id "{resource_id}"') +def step_mock_service_git_checkout( + context: Context, name: str, resource_id: str +) -> None: + """Set up a mock registry service returning a git-checkout resource.""" + mock_service = MagicMock() + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="git-checkout", + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + + +@given( + 'a mock resource service with container-instance "{name}" ' + 'id "{resource_id}" at "{location}"' +) +def step_mock_service_container_instance( + context: Context, name: str, resource_id: str, location: str +) -> None: + """Set up a mock registry service returning a container-instance resource.""" + mock_service = MagicMock() + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="container-instance", + location=location if location else None, + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + + +@given('a mock resource service that raises NotFoundError for "{name}"') +def step_mock_service_not_found(context: Context, name: str) -> None: + """Set up a mock registry service that raises NotFoundError on show_resource.""" + mock_service = MagicMock() + mock_service.show_resource.side_effect = NotFoundError( + message=f"Resource '{name}' not found" + ) + context.cli_mock_service = mock_service + + +@given('a CLI-mockable running devcontainer resource "{name}"') +def step_cli_mockable_running(context: Context, name: str) -> None: + """Set up mock service + lifecycle tracker in running state for CLI tests.""" + resource_id = "01TESTLIFECYCLE0000000400" + mock_service = MagicMock() + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="devcontainer-instance", + location="/workspace/project", + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + # Set up lifecycle tracker in running state + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.RUNNING, + container_id="ctr-a2-stop", + workspace_path="/workspaces/project", + ) + set_lifecycle_tracker(tracker) + + +@given('a CLI-mockable stopped devcontainer resource "{name}"') +def step_cli_mockable_stopped(context: Context, name: str) -> None: + """Set up mock service + lifecycle tracker in stopped state for CLI tests.""" + resource_id = "01TESTLIFECYCLE0000000401" + mock_service = MagicMock() + mock_resource = _make_mock_resource( + resource_id=resource_id, + name=name, + resource_type_name="devcontainer-instance", + location="/workspace/project", + ) + mock_service.show_resource.return_value = mock_resource + context.cli_mock_service = mock_service + context.cli_mock_resource = mock_resource + # Set up lifecycle tracker in stopped state + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState.STOPPED, + ) + set_lifecycle_tracker(tracker) + + +@given("CLI stop_container mock that raises RuntimeError") +def step_cli_stop_mock_raises(context: Context) -> None: + """Mark context so the CLI stop invocation patches stop_container to raise.""" + context.cli_stop_raises = True + + +@given("CLI rebuild_container mock that raises RuntimeError") +def step_cli_rebuild_mock_raises(context: Context) -> None: + """Mark context so the CLI rebuild invocation patches rebuild_container to raise.""" + context.cli_rebuild_raises = True + + +@given( + 'a session-scoped active container "{resource_id}" ' + 'with ID "{ctr_id}" session "{sess_id}"' +) +def step_create_active_container_with_session( + context: Context, resource_id: str, ctr_id: str, sess_id: str +) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + session_id=sess_id, + current_state=ContainerLifecycleState.RUNNING, + container_id=ctr_id, + workspace_path="/workspaces/project", + ) + set_lifecycle_tracker(tracker) + + +@given( + 'a session-scoped active container "{resource_id}" ' + 'with no docker ID session "{sess_id}"' +) +def step_create_active_container_no_docker_with_session( + context: Context, resource_id: str, sess_id: str +) -> None: + """Active container with no container_id -- docker stop is skipped.""" + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + session_id=sess_id, + current_state=ContainerLifecycleState.RUNNING, + container_id=None, + workspace_path="/workspaces/project", + ) + set_lifecycle_tracker(tracker) + + +@given("a facade with no session service") +def step_create_facade_no_session(context: Context) -> None: + context.facade = AcpLocalFacade(services={}) + + +# ── When steps ─────────────────────────────────────────────── + + +@when('I run session cleanup for session "{session_id}"') +def step_session_cleanup(context: Context, session_id: str) -> None: + context.stopped_ids = stop_all_active_containers( + run_command=context.mock_runner, + session_id=session_id, + ) + + +@when("I run session cleanup with no session filter") +def step_session_cleanup_no_filter(context: Context) -> None: + context.stopped_ids = stop_all_active_containers( + run_command=context.mock_runner, + session_id="", + ) + + +@when("I call CleanupService stop_active_devcontainers with no active containers") +def step_call_cleanup_stop_empty(context: Context) -> None: + clear_lifecycle_registry() + context.cleanup_stopped = CleanupService.stop_active_devcontainers( + session_id="test-session", + ) + + +@when('I invoke CLI resource stop "{name}"') +def step_invoke_cli_stop(context: Context, name: str) -> None: + """Invoke ``agents resource stop --yes`` via CliRunner.""" + runner = CliRunner() + with ( + patch(_CLI_PATCH_SERVICE, return_value=context.cli_mock_service), + patch(_CLI_PATCH_STOP) as mock_stop, + ): + # A2 fix: pass --yes to skip confirmation prompt in tests + context.cli_result = runner.invoke(resource_app, ["stop", name, "--yes"]) + context.cli_mock_stop = mock_stop + # Set context.output for compatibility with shared CLI assertion steps + context.output = context.cli_result.output + + +@when('I invoke CLI resource rebuild "{name}"') +def step_invoke_cli_rebuild(context: Context, name: str) -> None: + """Invoke ``agents resource rebuild --yes`` via CliRunner.""" + runner = CliRunner() + with ( + patch(_CLI_PATCH_SERVICE, return_value=context.cli_mock_service), + patch(_CLI_PATCH_REBUILD) as mock_rebuild, + ): + # A2 fix: pass --yes to skip confirmation prompt in tests + context.cli_result = runner.invoke(resource_app, ["rebuild", name, "--yes"]) + context.cli_mock_rebuild = mock_rebuild + # Set context.output for compatibility with shared CLI assertion steps + context.output = context.cli_result.output + + +@when('I invoke CLI resource stop with error mock "{name}"') +def step_invoke_cli_stop_error(context: Context, name: str) -> None: + """Invoke ``agents resource stop --yes`` with stop_container mocked to raise.""" + runner = CliRunner() + with ( + patch(_CLI_PATCH_SERVICE, return_value=context.cli_mock_service), + patch(_CLI_PATCH_STOP, side_effect=RuntimeError("docker daemon unavailable")), + ): + context.cli_result = runner.invoke(resource_app, ["stop", name, "--yes"]) + context.output = context.cli_result.output + + +@when('I invoke CLI resource rebuild with error mock "{name}"') +def step_invoke_cli_rebuild_error(context: Context, name: str) -> None: + """Invoke ``agents resource rebuild --yes`` with rebuild_container mocked.""" + runner = CliRunner() + with ( + patch(_CLI_PATCH_SERVICE, return_value=context.cli_mock_service), + patch(_CLI_PATCH_REBUILD, side_effect=RuntimeError("rebuild exploded")), + ): + context.cli_result = runner.invoke(resource_app, ["rebuild", name, "--yes"]) + context.output = context.cli_result.output + + +@when('I list active containers for session "{session_id}"') +def step_list_for_session(context: Context, session_id: str) -> None: + context.session_container_list = list_active_containers_for_session(session_id) + + +@when("I list active containers for an empty session") +def step_list_for_empty_session(context: Context) -> None: + context.session_container_list = list_active_containers_for_session("") + + +@when('I close session "{session_id}" via the facade') +def step_close_session_via_facade(context: Context, session_id: str) -> None: + request = AcpRequest( + operation="session.close", + params={"session_id": session_id}, + ) + context.facade_response = context.facade.dispatch(request) + + +# ── Then steps ─────────────────────────────────────────────── + + +@then("CleanupService should have a stop_active_devcontainers method") +def step_check_cleanup_method(context: Context) -> None: + assert hasattr(CleanupService, "stop_active_devcontainers") + assert callable(CleanupService.stop_active_devcontainers) + + +@then("the cleanup result should be an empty list") +def step_check_cleanup_empty(context: Context) -> None: + assert context.cleanup_stopped == [] + + +@then("the cleanup should return 0 stopped containers") +def step_check_cleanup_zero(context: Context) -> None: + assert len(context.stopped_ids) == 0 + + +@then('the cleanup stopped list should contain "{resource_id}"') +def step_check_cleanup_list_contains(context: Context, resource_id: str) -> None: + assert resource_id in context.stopped_ids, ( + f"Expected {resource_id} in stopped list, got {context.stopped_ids}" + ) + + +@then("the devcontainer CLI exit code should be non-zero") +def step_dc_cli_exit_nonzero(context: Context) -> None: + assert context.cli_result.exit_code != 0, ( + f"Expected non-zero exit code, got {context.cli_result.exit_code}.\n" + f"Output: {context.cli_result.output}" + ) + + +@then("the devcontainer CLI exit code should be 0") +def step_dc_cli_exit_zero(context: Context) -> None: + assert context.cli_result.exit_code == 0, ( + f"Expected exit code 0, got {context.cli_result.exit_code}.\n" + f"Output: {context.cli_result.output}" + ) + + +@then('the CLI stop mock should have been called with "{resource_id}"') +def step_check_cli_stop_mock_args(context: Context, resource_id: str) -> None: + context.cli_mock_stop.assert_called_once_with(resource_id) + + +@then( + 'the CLI rebuild mock should have been called with "{resource_id}" and "{location}"' +) +def step_check_cli_rebuild_mock_args( + context: Context, resource_id: str, location: str +) -> None: + context.cli_mock_rebuild.assert_called_once_with(resource_id, location) + + +@then("the CLI stop mock should have been called once") +def step_check_cli_stop_called_once(context: Context) -> None: + context.cli_mock_stop.assert_called_once() + + +@then("the CLI rebuild mock should have been called once") +def step_check_cli_rebuild_called_once(context: Context) -> None: + context.cli_mock_rebuild.assert_called_once() + + +@then('DevcontainerHandler should have _default_strategy "{strategy}"') +def step_check_handler_strategy(context: Context, strategy: str) -> None: + assert DevcontainerHandler._default_strategy == SandboxStrategy(strategy) + + +@then('DevcontainerHandler should have _type_label "{label}"') +def step_check_handler_label(context: Context, label: str) -> None: + assert DevcontainerHandler._type_label == label + + +@then("DevcontainerHandler should be a subclass of BaseResourceHandler") +def step_check_handler_inheritance(context: Context) -> None: + assert issubclass(DevcontainerHandler, BaseResourceHandler) + + +@then("DevcontainerHandler should be instantiable") +def step_check_handler_instantiable(context: Context) -> None: + handler = DevcontainerHandler() + assert handler is not None + assert isinstance(handler, DevcontainerHandler) + + +@then("DevcontainerHandler instance should have a resolve method") +def step_check_handler_resolve_method(context: Context) -> None: + handler = DevcontainerHandler() + assert hasattr(handler, "resolve") + assert callable(handler.resolve) + + +@then('the session container list should contain "{resource_id}"') +def step_check_session_list_contains(context: Context, resource_id: str) -> None: + assert resource_id in context.session_container_list, ( + f"Expected {resource_id} in session list, got {context.session_container_list}" + ) + + +@then('the session container list should not contain "{resource_id}"') +def step_check_session_list_not_contains(context: Context, resource_id: str) -> None: + assert resource_id not in context.session_container_list, ( + f"Expected {resource_id} NOT in session list, " + f"got {context.session_container_list}" + ) diff --git a/features/steps/devcontainer_health_check_steps.py b/features/steps/devcontainer_health_check_steps.py new file mode 100644 index 000000000..ddae26e1b --- /dev/null +++ b/features/steps/devcontainer_health_check_steps.py @@ -0,0 +1,409 @@ +"""Step definitions for devcontainer health check and concurrency operations. + +Covers: single health probe, health check loop iterations, start/stop +health check threads, thread registration and join, probe workspace +fallback, health check argument validation, concurrent activation, +and concurrent stop. + +All mocks are in ``features/mocks/mock_devcontainer_cli.py``. +""" + +from __future__ import annotations + +import threading +import time +from typing import Any, cast +from unittest.mock import MagicMock +from unittest.mock import patch as _patch + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, +) +from cleveragents.resource.handlers import devcontainer as _dc_mod +from cleveragents.resource.handlers.devcontainer import ( + DevcontainerHandler, + _health_check_loop, + _health_check_stop_events, + _health_check_threads, + _single_probe, + _stop_health_check, + activate_container, + clear_lifecycle_registry, + get_lifecycle_tracker, + set_lifecycle_tracker, + start_health_check, + stop_container, +) + +# Import _OneShotEvent from the core lifecycle steps module to avoid +# duplicating the helper class. +from features.steps.devcontainer_lifecycle_steps import _OneShotEvent + +# ── Given steps ────────────────────────────────────────────── + + +@given("the runner configured for failed health check") +def step_configure_hc_failure(context: Context) -> None: + context.mock_runner.set_exec_failure() + + +@given("the runner configured for successful health check") +def step_configure_hc_success(context: Context) -> None: + context.mock_runner.set_exec_result(returncode=0, stdout="ping") + + +@given("the runner configured to raise exception on exec") +def step_configure_exec_exception(context: Context) -> None: + """Replace mock_runner with a wrapper that raises on exec.""" + inner = context.mock_runner + + def raising_runner(args: list[str], **kwargs: object) -> object: + inner.calls.append((list(args), dict(kwargs))) + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "exec": + raise OSError("connection refused") + return inner(args, **kwargs) + + context.mock_runner = cast(Any, raising_runner) + + +@given('a registered health check for "{resource_id}"') +def step_register_health_check(context: Context, resource_id: str) -> None: + """Register a real (but idle) health check thread so _stop_health_check + can join it and we can verify the full thread-join path (F9 fix). + """ + stop_event = threading.Event() + _health_check_stop_events[resource_id] = stop_event + + # F9 fix: use a real short-lived thread instead of current_thread() + # so _stop_health_check actually calls thread.join() rather than + # skipping the join due to the ``is not current_thread()`` guard. + def _idle_loop() -> None: + stop_event.wait(timeout=5.0) + + thread = threading.Thread(target=_idle_loop, daemon=True) + _health_check_threads[resource_id] = thread + thread.start() + + +@given('a running health check for "{resource_id}"') +def step_start_health_check(context: Context, resource_id: str) -> None: + """Start a health check thread that we can later verify was joined.""" + context.mock_runner.set_exec_result(returncode=0, stdout="ping") + start_health_check(resource_id, interval=0.1, run_command=context.mock_runner) + # Give it a moment to start + time.sleep(0.05) + + +# ── When steps ─────────────────────────────────────────────── + + +@when('I run a single health check probe for "{resource_id}"') +def step_single_health_probe(context: Context, resource_id: str) -> None: + """Run a single health check probe via the production _health_check_loop. + + Uses the shared _OneShotEvent to run exactly one probe iteration + through the real production code path, avoiding reimplementation + of transition logic in the test step. + """ + _health_check_loop( + resource_id, + interval=0.0, + stop_event=cast(threading.Event, _OneShotEvent()), + run_command=context.mock_runner, + ) + + +@when('I run one iteration of the health check loop for "{resource_id}"') +def step_run_health_check_loop_once( + context: Context, + resource_id: str, +) -> None: + """Run _health_check_loop with the shared _OneShotEvent.""" + context.hc_resource_id = resource_id + + _health_check_loop( + resource_id, + interval=0.0, + stop_event=cast(threading.Event, _OneShotEvent()), + run_command=context.mock_runner, + ) + + +@when('I call start_health_check for "{resource_id}"') +def step_call_start_health_check(context: Context, resource_id: str) -> None: + """Call start_health_check and immediately stop the thread.""" + start_health_check( + resource_id, + interval=60.0, # long interval — we stop it immediately + run_command=context.mock_runner, + ) + context.hc_resource_id = resource_id + # Give thread just enough time to register + time.sleep(0.05) + + +@when('I stop the health check for "{resource_id}"') +def step_stop_health_check(context: Context, resource_id: str) -> None: + _stop_health_check(resource_id) + context.health_check_resource_id = resource_id + + +@when("I attempt to start health check with empty resource_id") +def step_start_hc_empty_id(context: Context) -> None: + context.hc_error = None + try: + start_health_check("") + except ValueError as exc: + context.hc_error = exc + + +@when("I run a single probe with no workspace_path set") +def step_probe_no_workspace(context: Context) -> None: + """Run _single_probe with a tracker that has workspace_path=None.""" + tracker = ContainerLifecycleTracker( + resource_id="01TESTLIFECYCLE0000000320", + current_state=ContainerLifecycleState.RUNNING, + container_id="ctr-t3", + workspace_path=None, + ) + set_lifecycle_tracker(tracker) + context.mock_runner.set_exec_result(returncode=0, stdout="ping") + _single_probe(tracker, context.mock_runner) + context.probe_calls = context.mock_runner.exec_calls + + +# ── Then steps ─────────────────────────────────────────────── + + +@then('the health check should have probed "{resource_id}"') +def step_check_health_probe(context: Context, resource_id: str) -> None: + exec_calls = context.mock_runner.exec_calls + assert len(exec_calls) > 0, "Expected at least one exec call from health check" + + +@then('a health check thread should be registered for "{resource_id}"') +def step_check_thread_registered(context: Context, resource_id: str) -> None: + assert resource_id in _health_check_threads, ( + f"Thread not registered for {resource_id}" + ) + assert resource_id in _health_check_stop_events, ( + f"Stop event not registered for {resource_id}" + ) + # Clean up — stop the thread + clear_lifecycle_registry() + + +@then('the health check for "{resource_id}" should be unregistered') +def step_check_hc_unregistered(context: Context, resource_id: str) -> None: + assert resource_id not in _health_check_stop_events, ( + f"Stop event still registered for {resource_id}" + ) + assert resource_id not in _health_check_threads, ( + f"Thread still registered for {resource_id}" + ) + + +@then("the health check should raise ValueError") +def step_check_hc_value_error(context: Context) -> None: + assert context.hc_error is not None + assert isinstance(context.hc_error, ValueError) + + +@then("the health check thread should have been joined") +def step_check_thread_joined(context: Context) -> None: + rid = context.health_check_resource_id + # After _stop_health_check, thread should have been removed from registry + assert rid not in _health_check_threads, ( + f"Thread for {rid} still in _health_check_threads after stop" + ) + assert rid not in _health_check_stop_events, ( + f"Stop event for {rid} still in _health_check_stop_events after stop" + ) + + +@then('the probe command should use "{path}" as workspace folder') +def step_check_probe_workspace(context: Context, path: str) -> None: + assert len(context.probe_calls) >= 1, "Expected at least one exec call" + args, _ = context.probe_calls[-1] + # args: ["devcontainer", "exec", "--workspace-folder", path, "echo", "ping"] + ws_idx = args.index("--workspace-folder") + 1 + assert args[ws_idx] == path, f"Expected workspace '{path}', got '{args[ws_idx]}'" + + +# ── Concurrent operation steps ─────────────────────────────── + + +@when("I activate the container concurrently from two threads") +def step_concurrent_activation(context: Context) -> None: + """Launch two threads both trying to activate the same resource. + + TEST-1 fix: use ``threading.Barrier(2)`` so both threads reach the + activation call at the same time, preventing the first thread from + completing before the second starts. + """ + resource_id = context.resource_id + results: list[tuple[str, object]] = [] + lock = threading.Lock() + barrier = threading.Barrier(2, timeout=5.0) + + def _activate(label: str) -> None: + try: + barrier.wait() + activate_container( + resource_id, + "/workspace", + run_command=context.mock_runner, + ) + with lock: + results.append((label, "ok")) + except (ValueError, RuntimeError) as exc: + with lock: + results.append((label, exc)) + + t1 = threading.Thread(target=_activate, args=("t1",)) + t2 = threading.Thread(target=_activate, args=("t2",)) + t1.start() + t2.start() + t1.join(timeout=5.0) + t2.join(timeout=5.0) + + context.concurrent_results = results + + +@when("I stop the container concurrently from two threads") +def step_concurrent_stop(context: Context) -> None: + """Launch two threads both trying to stop the same resource. + + TEST-1 fix: use ``threading.Barrier(2)`` so both threads reach the + stop call at the same time, preventing sequential execution. + """ + resource_id = "01TESTLIFECYCLE0000000390" + results: list[tuple[str, object]] = [] + lock = threading.Lock() + barrier = threading.Barrier(2, timeout=5.0) + + def _stop(label: str) -> None: + try: + barrier.wait() + stop_container( + resource_id, + run_command=context.mock_runner, + ) + with lock: + results.append((label, "ok")) + except (ValueError, RuntimeError) as exc: + with lock: + results.append((label, exc)) + + t1 = threading.Thread(target=_stop, args=("t1",)) + t2 = threading.Thread(target=_stop, args=("t2",)) + t1.start() + t2.start() + t1.join(timeout=5.0) + t2.join(timeout=5.0) + + context.concurrent_stop_results = results + + +@then("exactly one activation should succeed") +def step_one_success(context: Context) -> None: + successes = [r for r in context.concurrent_results if r[1] == "ok"] + assert len(successes) == 1, ( + f"Expected exactly 1 success, got {len(successes)}: {context.concurrent_results}" + ) + + +@then("exactly one activation should fail with ValueError") +def step_one_failure(context: Context) -> None: + failures = [r for r in context.concurrent_results if isinstance(r[1], ValueError)] + assert len(failures) == 1, ( + f"Expected exactly 1 ValueError, got {len(failures)}: {context.concurrent_results}" + ) + + +@then("both stops should succeed") +def step_both_stops_succeed(context: Context) -> None: + # R8-F2 fix: stop_container is now idempotent — the second caller + # sees a terminal state and returns early instead of raising. + successes = [r for r in context.concurrent_stop_results if r[1] == "ok"] + assert len(successes) == 2, ( + f"Expected 2 stop successes (idempotent), got {len(successes)}: " + f"{context.concurrent_stop_results}" + ) + + +@then('the container state for "{resource_id}" should not be "{state}"') +def step_check_state_not(context: Context, resource_id: str, state: str) -> None: + tracker = get_lifecycle_tracker(resource_id) + assert tracker.current_state != ContainerLifecycleState(state), ( + f"Expected state NOT to be {state}, but got {tracker.current_state.value}" + ) + + +@then("exactly {count:d} docker stop call should have been made") +def step_check_docker_stop_count(context: Context, count: int) -> None: + actual = len(context.mock_runner.stop_calls) + assert actual == count, ( + f"Expected exactly {count} docker stop call(s), got {actual}" + ) + + +# ── F16: resolve() lazy activation steps ───────────────────── + + +@when('I resolve a devcontainer-instance "{resource_id}" at "{location}"') +def step_resolve_devcontainer( + context: Context, resource_id: str, location: str +) -> None: + """Call DevcontainerHandler.resolve() with mocked Resource and SandboxManager. + + The handler's resolve() triggers activate_container() for + non-running devcontainer-instance resources (F16 fix), then + delegates to super().resolve() which needs a SandboxManager. + We patch activate_container with a side_effect that injects + the mock runner, so the real activation logic executes. + """ + handler = DevcontainerHandler() + mock_resource = MagicMock() + mock_resource.resource_id = resource_id + mock_resource.resource_type_name = "devcontainer-instance" + mock_resource.location = location + mock_resource.sandbox_strategy = None + + mock_sandbox_mgr = MagicMock() + mock_sandbox_ctx = MagicMock() + mock_sandbox_ctx.sandbox_path = "/tmp/sandbox" + mock_sandbox = MagicMock() + mock_sandbox.context = mock_sandbox_ctx + mock_sandbox_mgr.get_or_create_sandbox.return_value = mock_sandbox + + real_activate = _dc_mod.activate_container + + def _activate_with_mock( + rid: str, + ws: str, + session_id: str | None = None, + **kwargs: object, + ) -> object: + return real_activate( + rid, ws, run_command=context.mock_runner, session_id=session_id + ) + + context.resolve_error = None + with _patch.object( + _dc_mod, "activate_container", side_effect=_activate_with_mock + ) as mock_activate: + try: + context.bound_resource = handler.resolve( + resource=mock_resource, + plan_id="plan-001", + slot_name="workspace", + sandbox_manager=mock_sandbox_mgr, + ) + except (RuntimeError, ValueError) as exc: + context.resolve_error = exc + context.resolve_activate_mock = mock_activate diff --git a/features/steps/devcontainer_lifecycle_steps.py b/features/steps/devcontainer_lifecycle_steps.py new file mode 100644 index 000000000..7af7a66d8 --- /dev/null +++ b/features/steps/devcontainer_lifecycle_steps.py @@ -0,0 +1,464 @@ +"""Step definitions for devcontainer lifecycle state model, registry, and JSON parsing. + +Covers: lifecycle state enum, state transitions, tracker creation and +retrieval, registry operations (get/set/clear/evict), argument +validation for validate_transition and transition_state, the +transition history cap, and devcontainer-up JSON output parsing. + +All mocks are in ``features/mocks/mock_devcontainer_cli.py``. +""" + +from __future__ import annotations + +import json +from typing import cast + +from behave import given, then, when +from behave.runner import Context + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, + validate_transition, +) +from cleveragents.resource.handlers.devcontainer import ( + _lifecycle_registry, + _parse_devcontainer_up_output, + _registry_lock, + clear_lifecycle_registry, + evict_terminal_trackers, + get_lifecycle_tracker, + list_active_containers, + set_lifecycle_tracker, +) + +# ── Shared helper ──────────────────────────────────────────── + + +# R21 fix: shared _OneShotEvent helper — extracted from duplicated +# definitions in step_single_health_probe and step_run_health_check_loop_once. +# +# F8 fix: documented the exact call sequence the counter relies on. +class _OneShotEvent: + """Stop event that allows exactly one probe iteration. + + Simulates ``threading.Event`` for health check loop testing, + running exactly one probe cycle before signaling stop. + + The counter relies on the following call sequence inside + ``_health_check_loop``: + + 1. ``while not stop_event.is_set()`` → count=1, returns False + 2. ``stop_event.wait(interval)`` → overridden to no-op + 3. ``if stop_event.is_set()`` → count=2, returns False + 4. (probe executes) + 5. ``while not stop_event.is_set()`` → count=3, returns True → exit + + **WARNING**: If the loop's ``is_set()`` call pattern changes, this + counter threshold must be updated to match. + """ + + def __init__(self) -> None: + self._count = 0 + + def is_set(self) -> bool: + self._count += 1 + return self._count > 2 + + def set(self) -> None: + self._count = 999 + + def wait(self, timeout: float = 0) -> bool: + return False + + +# ── Given steps ────────────────────────────────────────────── +# Registry cleanup is handled by the before_scenario hook in +# features/environment.py so that all scenarios start with a +# clean lifecycle registry regardless of execution order. + + +@given('a lifecycle tracker for resource "{resource_id}"') +def step_create_tracker(context: Context, resource_id: str) -> None: + tracker = ContainerLifecycleTracker(resource_id=resource_id) + set_lifecycle_tracker(tracker) + context.tracker = tracker + context.resource_id = resource_id + + +@given('a lifecycle tracker in "{state}" state for resource "{resource_id}"') +def step_create_tracker_in_state( + context: Context, state: str, resource_id: str +) -> None: + tracker = ContainerLifecycleTracker( + resource_id=resource_id, + current_state=ContainerLifecycleState(state), + ) + set_lifecycle_tracker(tracker) + context.tracker = tracker + context.resource_id = resource_id + + +@given("{count:d} stopped container trackers in the registry") +def step_create_many_stopped(context: Context, count: int) -> None: + with _registry_lock: + for i in range(count): + rid = f"01EVICT{i:020d}" + _lifecycle_registry[rid] = ContainerLifecycleTracker( + resource_id=rid, + current_state=ContainerLifecycleState.STOPPED, + ) + + +# ── When steps ─────────────────────────────────────────────── + + +@when("I inspect ContainerLifecycleState enum values") +def step_inspect_enum(context: Context) -> None: + context.enum_values = [s.value for s in ContainerLifecycleState] + + +@when('I transition from "{from_state}" to "{to_state}"') +def step_transition(context: Context, from_state: str, to_state: str) -> None: + context.transition_error = None + try: + context.tracker = transition_state( + context.tracker, + ContainerLifecycleState(to_state), + reason="test transition", + ) + set_lifecycle_tracker(context.tracker) + except (ValueError, TypeError) as exc: + context.transition_error = exc + + +@when('I attempt to transition from "{from_state}" to "{to_state}"') +def step_attempt_transition(context: Context, from_state: str, to_state: str) -> None: + context.transition_error = None + try: + context.tracker = transition_state( + context.tracker, + ContainerLifecycleState(to_state), + reason="test transition", + ) + set_lifecycle_tracker(context.tracker) + except (ValueError, TypeError) as exc: + context.transition_error = exc + + +@when("I call validate_transition with non-enum arguments") +def step_validate_transition_bad_types(context: Context) -> None: + context.type_error = None + try: + validate_transition( + cast(ContainerLifecycleState, "inactive"), + cast(ContainerLifecycleState, "starting"), + ) + except TypeError as exc: + context.type_error = exc + + +@when("I call validate_transition with non-enum to_state") +def step_validate_transition_bad_to_state(context: Context) -> None: + context.type_error = None + try: + validate_transition( + ContainerLifecycleState.DETECTED, + cast(ContainerLifecycleState, "starting"), + ) + except TypeError as exc: + context.type_error = exc + + +@when("I call transition_state with non-tracker argument") +def step_transition_state_bad_tracker(context: Context) -> None: + context.type_error = None + try: + transition_state( + cast(ContainerLifecycleTracker, "not-a-tracker"), + ContainerLifecycleState.BUILDING, + ) + except TypeError as exc: + context.type_error = exc + + +@when("I call transition_state with non-enum to_state argument") +def step_transition_state_bad_to_state(context: Context) -> None: + context.type_error = None + try: + tracker = ContainerLifecycleTracker(resource_id="01TESTLIFECYCLE0099000001") + transition_state(tracker, cast(ContainerLifecycleState, "starting")) + except TypeError as exc: + context.type_error = exc + + +@when('I retrieve the tracker for "{resource_id}"') +def step_retrieve_tracker(context: Context, resource_id: str) -> None: + context.retrieved_tracker = get_lifecycle_tracker(resource_id) + + +@when("I list active containers") +def step_list_active(context: Context) -> None: + context.active_list = list_active_containers() + + +@when("I perform 105 transitions cycling through valid states") +def step_perform_105_transitions(context: Context) -> None: + """Cycle detected->building->running->stopping->stopped->building->... 105 times.""" + tracker = context.tracker + # First transition: detected -> building + tracker = transition_state( + tracker, ContainerLifecycleState.BUILDING, reason="t1-init" + ) + cycle = [ + ContainerLifecycleState.RUNNING, + ContainerLifecycleState.STOPPING, + ContainerLifecycleState.STOPPED, + ContainerLifecycleState.BUILDING, + ] + # We've done 1 transition, need 104 more for 105 total + for i in range(104): + state = cycle[i % len(cycle)] + tracker = transition_state(tracker, state, reason=f"t1-{i}") + set_lifecycle_tracker(tracker) + context.tracker = tracker + + +@when("I attempt to get tracker with empty resource_id") +def step_get_tracker_empty(context: Context) -> None: + context.get_error = None + try: + get_lifecycle_tracker("") + except ValueError as exc: + context.get_error = exc + + +@when("I attempt to set tracker with wrong type") +def step_set_tracker_wrong_type(context: Context) -> None: + context.set_error = None + try: + set_lifecycle_tracker(cast(ContainerLifecycleTracker, "not-a-tracker")) + except TypeError as exc: + context.set_error = exc + + +@when("I run terminal tracker eviction") +def step_run_eviction(context: Context) -> None: + context.evicted_count = evict_terminal_trackers() + + +@when('I get tracker for new resource "{resource_id}"') +def step_get_new_tracker(context: Context, resource_id: str) -> None: + context.auto_tracker = get_lifecycle_tracker(resource_id) + + +@when("I clear the lifecycle registry") +def step_clear_registry(context: Context) -> None: + clear_lifecycle_registry() + + +# ── Then steps ─────────────────────────────────────────────── + + +@then('the lifecycle enum should contain "{value}"') +def step_check_lifecycle_enum_value(context: Context, value: str) -> None: + assert value in context.enum_values, f"'{value}' not in {context.enum_values}" + + +@then("the transition should succeed") +def step_transition_ok(context: Context) -> None: + assert context.transition_error is None, ( + f"Unexpected error: {context.transition_error}" + ) + + +@then('the tracker state should be "{state}"') +def step_check_tracker_state(context: Context, state: str) -> None: + assert context.tracker.current_state == ContainerLifecycleState(state) + + +@then("the transition should raise ValueError") +def step_transition_error(context: Context) -> None: + assert context.transition_error is not None + assert isinstance(context.transition_error, ValueError) + + +@then("the tracker should have {count:d} transition in history") +def step_check_history_count(context: Context, count: int) -> None: + assert len(context.tracker.transitions) == count + + +@then('the retrieved tracker state should be "{state}"') +def step_check_retrieved_state(context: Context, state: str) -> None: + assert context.retrieved_tracker.current_state == ContainerLifecycleState(state) + + +@then('the active list should contain "{resource_id}"') +def step_check_active_contains(context: Context, resource_id: str) -> None: + assert resource_id in context.active_list + + +@then('the active list should not contain "{resource_id}"') +def step_check_active_not_contains(context: Context, resource_id: str) -> None: + assert resource_id not in context.active_list + + +@then("it should raise TypeError") +def step_check_type_error(context: Context) -> None: + assert context.type_error is not None + assert isinstance(context.type_error, TypeError) + + +@then("the tracker should have at most 100 transitions") +def step_check_max_transitions(context: Context) -> None: + assert len(context.tracker.transitions) <= 100, ( + f"Expected at most 100 transitions, got {len(context.tracker.transitions)}" + ) + + +@then("the get tracker should raise ValueError") +def step_check_get_error(context: Context) -> None: + assert context.get_error is not None + assert isinstance(context.get_error, ValueError) + + +@then("the set tracker should raise TypeError") +def step_check_set_error(context: Context) -> None: + assert context.set_error is not None + assert isinstance(context.set_error, TypeError) + + +@then("the registry should have at most {max_count:d} terminal trackers") +def step_check_registry_cap(context: Context, max_count: int) -> None: + with _registry_lock: + terminal = [ + rid + for rid, t in _lifecycle_registry.items() + if t.current_state + in (ContainerLifecycleState.STOPPED, ContainerLifecycleState.FAILED) + ] + assert len(terminal) <= max_count, ( + f"Expected at most {max_count} terminal trackers, got {len(terminal)}" + ) + + +@then("the eviction count should be {count:d}") +def step_check_eviction_count(context: Context, count: int) -> None: + assert context.evicted_count == count, ( + f"Expected eviction count {count}, got {context.evicted_count}" + ) + + +@then('the auto-created tracker state should be "{state}"') +def step_check_auto_tracker_state(context: Context, state: str) -> None: + assert context.auto_tracker.current_state == ContainerLifecycleState(state) + + +@then('the auto-created tracker resource_id should be "{resource_id}"') +def step_check_auto_tracker_id(context: Context, resource_id: str) -> None: + assert context.auto_tracker.resource_id == resource_id + + +@then("the registry should be empty") +def step_check_registry_empty(context: Context) -> None: + with _registry_lock: + assert len(_lifecycle_registry) == 0, ( + f"Expected empty registry, got {len(_lifecycle_registry)} entries" + ) + + +# ── JSON output parsing steps ──────────────────────────────── + + +@when('I parse devcontainer up output with containerId "{ctr_id}" and workspace "{ws}"') +def step_parse_json(context: Context, ctr_id: str, ws: str) -> None: + stdout = json.dumps( + { + "outcome": "success", + "containerId": ctr_id, + "remoteWorkspaceFolder": ws, + } + ) + context.parsed_ctr_id, context.parsed_ws = _parse_devcontainer_up_output(stdout) + + +@when("I parse devcontainer up output with invalid JSON") +def step_parse_invalid_json(context: Context) -> None: + context.parsed_ctr_id, context.parsed_ws = _parse_devcontainer_up_output( + "not valid json {{" + ) + + +@when("I parse devcontainer up output with log lines before JSON") +def step_parse_multiline_json(context: Context) -> None: + stdout = ( + "[2025-01-01T00:00:00Z] Starting container...\n" + "Some log line\n" + + json.dumps( + { + "outcome": "success", + "containerId": "aabbccddee0099887766", + "remoteWorkspaceFolder": "/workspaces/multi", + } + ) + ) + context.parsed_ctr_id, context.parsed_ws = _parse_devcontainer_up_output(stdout) + + +@when('I parse devcontainer up output with invalid container ID "{bad_id}"') +def step_parse_invalid_container_id(context: Context, bad_id: str) -> None: + stdout = json.dumps( + { + "outcome": "success", + "containerId": bad_id, + "remoteWorkspaceFolder": "/workspaces/project", + } + ) + context.parsed_ctr_id, context.parsed_ws = _parse_devcontainer_up_output(stdout) + + +@when('I parse devcontainer up output with relative workspace "{ws}"') +def step_parse_relative_ws(context: Context, ws: str) -> None: + stdout = json.dumps( + { + "outcome": "success", + "containerId": "aabbccddee0011223344", + "remoteWorkspaceFolder": ws, + } + ) + context.parsed_ctr_id, context.parsed_ws = _parse_devcontainer_up_output(stdout) + + +@then('the parsed container ID should be "{ctr_id}"') +def step_check_parsed_ctr_id(context: Context, ctr_id: str) -> None: + assert context.parsed_ctr_id == ctr_id + + +@then("the parsed container ID should be None") +def step_check_parsed_ctr_id_none(context: Context) -> None: + assert context.parsed_ctr_id is None + + +@then('the parsed workspace path should be "{ws}"') +def step_check_parsed_ws(context: Context, ws: str) -> None: + assert context.parsed_ws == ws + + +@then("the parsed workspace path should be None") +def step_check_parsed_ws_none(context: Context) -> None: + assert context.parsed_ws is None + + +@then("the parsed container ID should be empty") +def step_check_empty_container_id(context: Context) -> None: + assert context.parsed_ctr_id is None, ( + f"Expected None container ID, got '{context.parsed_ctr_id}'" + ) + + +@then("the runner should not have received a devcontainer up call") +def step_check_no_up_call(context: Context) -> None: + assert len(context.mock_runner.up_calls) == 0, ( + f"Expected 0 devcontainer up calls, got {len(context.mock_runner.up_calls)}" + ) diff --git a/robot/cleanup.robot b/robot/cleanup.robot index df5a24c1d..d639cabff 100644 --- a/robot/cleanup.robot +++ b/robot/cleanup.robot @@ -21,11 +21,21 @@ Cleanup Service Exports Expected Symbols [Documentation] Verify cleanup_service.py exports the public API ${content}= Get File ${SRC_DIR}/application/services/cleanup_service.py Should Contain ${content} class CleanupService - Should Contain ${content} class CleanupReport - Should Contain ${content} class StaleItem - Should Contain ${content} class ResourceCleanupSummary Should Contain ${content} def scan Should Contain ${content} def purge + # F26 fix: CleanupReport, StaleItem, ResourceCleanupSummary were + # extracted to cleanup_models.py (F8 fix). Verify definitions live + # there and that cleanup_service.py re-exports them via __all__. + Should Contain ${content} CleanupReport + Should Contain ${content} StaleItem + Should Contain ${content} ResourceCleanupSummary + +Cleanup Models Defines Data Classes + [Documentation] Verify cleanup_models.py contains the extracted data classes + ${models}= Get File ${SRC_DIR}/application/services/cleanup_models.py + Should Contain ${models} class CleanupReport + Should Contain ${models} class StaleItem + Should Contain ${models} class ResourceCleanupSummary Cleanup CLI Has Scan Command [Documentation] Verify cleanup CLI has scan subcommand diff --git a/robot/devcontainer_lifecycle.robot b/robot/devcontainer_lifecycle.robot new file mode 100644 index 000000000..117f34b33 --- /dev/null +++ b/robot/devcontainer_lifecycle.robot @@ -0,0 +1,89 @@ +*** Settings *** +Documentation Integration tests for devcontainer lifecycle management (issue #514) +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER} ${CURDIR}/helper_devcontainer_lifecycle.py + +*** Test Cases *** +ContainerLifecycleState Enum Values + [Documentation] Verify all six lifecycle states exist + ${result}= Run Process ${PYTHON} ${HELPER} enum-values cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} enum-values-ok + +Valid Transition Detected To Building + [Documentation] Verify detected->building transition + ${result}= Run Process ${PYTHON} ${HELPER} transition-valid cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} transition-valid-ok + +Invalid Transition Rejected + [Documentation] Verify invalid transitions raise ValueError + ${result}= Run Process ${PYTHON} ${HELPER} transition-invalid cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} transition-invalid-ok + +Lazy Activation With Mock Runner + [Documentation] Verify lazy activation with mock devcontainer CLI + ${result}= Run Process ${PYTHON} ${HELPER} lazy-activation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} lazy-activation-ok + +Stop Container With Mock Runner + [Documentation] Verify stop transitions active to stopped + ${result}= Run Process ${PYTHON} ${HELPER} stop-container cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} stop-container-ok + +Rebuild Container With Mock Runner + [Documentation] Verify rebuild transitions stopped to active + ${result}= Run Process ${PYTHON} ${HELPER} rebuild-container cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} rebuild-container-ok + +Session Cleanup Stops Active Containers + [Documentation] Verify session cleanup stops all active containers + ${result}= Run Process ${PYTHON} ${HELPER} session-cleanup cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} session-cleanup-ok + +JSON Output Parsing + [Documentation] Verify devcontainer up JSON output parsing + ${result}= Run Process ${PYTHON} ${HELPER} json-parsing cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} json-parsing-ok + +Lifecycle Registry Persistence + [Documentation] Verify tracker persists in registry + ${result}= Run Process ${PYTHON} ${HELPER} registry-persist cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} registry-persist-ok + +CleanupService Has Devcontainer Method + [Documentation] Verify CleanupService.stop_active_devcontainers exists + ${result}= Run Process ${PYTHON} ${HELPER} cleanup-method cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} cleanup-method-ok diff --git a/robot/helper_devcontainer_handler.py b/robot/helper_devcontainer_handler.py index f4af027fa..f169dbc8c 100644 --- a/robot/helper_devcontainer_handler.py +++ b/robot/helper_devcontainer_handler.py @@ -73,9 +73,14 @@ def cmd_protocol_check() -> None: def cmd_strategy_check() -> None: - """Verify DevcontainerHandler default strategy.""" + """Verify DevcontainerHandler default strategy. + + F22/F25 fix: handler uses ``none`` because SandboxFactory has not yet + implemented ``snapshot``. The container itself provides isolation. + Known limitation — will switch to ``snapshot`` once implemented. + """ handler = DevcontainerHandler() - assert handler._default_strategy.value == "snapshot" + assert handler._default_strategy.value == "none" print("strategy-check-ok") diff --git a/robot/helper_devcontainer_lifecycle.py b/robot/helper_devcontainer_lifecycle.py new file mode 100644 index 000000000..604bab7d6 --- /dev/null +++ b/robot/helper_devcontainer_lifecycle.py @@ -0,0 +1,275 @@ +"""Helper utilities for devcontainer lifecycle Robot smoke tests. + +Each command prints a single ``-ok`` token on success so the +calling Robot test can assert on ``stdout``. +""" + +from __future__ import annotations + +import json +import sys +from dataclasses import dataclass, field + +from cleveragents.application.services.cleanup_service import CleanupService +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, +) +from cleveragents.resource.handlers.devcontainer import ( + _parse_devcontainer_up_output, + activate_container, + clear_lifecycle_registry, + get_lifecycle_tracker, + rebuild_container, + set_lifecycle_tracker, + stop_all_active_containers, + stop_container, +) + +# --------------------------------------------------------------------------- +# F29 fix: removed conditional import with ``# type: ignore`` suppressions. +# The inline mock classes are always used — they are self-contained and +# avoid the PYTHONPATH dependency on ``features/``, which is not always +# available in standalone Robot runs. +# --------------------------------------------------------------------------- + + +@dataclass +class _MockResult: + """Minimal subprocess.CompletedProcess stand-in.""" + + returncode: int = 0 + stdout: str = "" + stderr: str = "" + args: list[str] = field(default_factory=list) + + +class _MockRunner: + """Callable mock for subprocess.run used by lifecycle functions.""" + + def __init__(self) -> None: + self.calls: list[tuple[list[str], dict[str, object]]] = [] + self.up_result: _MockResult = _MockResult() + self.exec_result: _MockResult = _MockResult() + self.stop_result: _MockResult = _MockResult() + + def __call__(self, args: list[str], **kwargs: object) -> _MockResult: + self.calls.append((list(args), dict(kwargs))) + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "up": + return self.up_result + if len(args) >= 2 and args[0] == "devcontainer" and args[1] == "exec": + return self.exec_result + if len(args) >= 2 and args[0] == "docker" and args[1] == "stop": + return self.stop_result + return _MockResult(returncode=0) + + def set_up_result( + self, + *, + container_id: str = "aabbccddee0011223344", + workspace: str = "/workspaces/project", + returncode: int = 0, + ) -> None: + output = json.dumps( + { + "outcome": "success" if returncode == 0 else "error", + "containerId": container_id, + "remoteWorkspaceFolder": workspace, + } + ) + self.up_result = _MockResult(returncode=returncode, stdout=output) + + @property + def up_calls(self) -> list[tuple[list[str], dict[str, object]]]: + return [ + (a, k) + for a, k in self.calls + if len(a) >= 2 and a[0] == "devcontainer" and a[1] == "up" + ] + + @property + def stop_calls(self) -> list[tuple[list[str], dict[str, object]]]: + return [ + (a, k) + for a, k in self.calls + if len(a) >= 2 and a[0] == "docker" and a[1] == "stop" + ] + + +# --------------------------------------------------------------------------- +# Test commands +# --------------------------------------------------------------------------- + + +def cmd_enum_values() -> None: + """Verify all six lifecycle states exist.""" + expected = {"detected", "building", "running", "stopping", "stopped", "failed"} + actual = {s.value for s in ContainerLifecycleState} + assert actual == expected, f"Expected {expected}, got {actual}" + print("enum-values-ok") + + +def cmd_transition_valid() -> None: + """Verify detected->building transition.""" + clear_lifecycle_registry() + tracker = ContainerLifecycleTracker(resource_id="01ROBOTTEST0000000000001") + tracker = transition_state(tracker, ContainerLifecycleState.BUILDING, reason="test") + assert tracker.current_state == ContainerLifecycleState.BUILDING + assert len(tracker.transitions) == 1 + print("transition-valid-ok") + + +def cmd_transition_invalid() -> None: + """Verify invalid transitions raise ValueError.""" + clear_lifecycle_registry() + tracker = ContainerLifecycleTracker(resource_id="01ROBOTTEST0000000000002") + try: + transition_state(tracker, ContainerLifecycleState.RUNNING, reason="bad") + raise AssertionError("Should have raised ValueError") + except ValueError: + pass + print("transition-invalid-ok") + + +def cmd_lazy_activation() -> None: + """Verify lazy activation with mock runner.""" + clear_lifecycle_registry() + runner = _MockRunner() + runner.set_up_result(container_id="aabbccddee0022334455", workspace="/ws") + result = activate_container( + "01ROBOTTEST0000000000010", + "/workspace", + run_command=runner, + ) + assert result.current_state == ContainerLifecycleState.RUNNING + assert result.container_id == "aabbccddee0022334455" + assert len(runner.up_calls) == 1 + print("lazy-activation-ok") + + +def cmd_stop_container() -> None: + """Verify stop transitions active to stopped.""" + clear_lifecycle_registry() + runner = _MockRunner() + tracker = ContainerLifecycleTracker( + resource_id="01ROBOTTEST0000000000020", + current_state=ContainerLifecycleState.RUNNING, + container_id="ctr-stop", + ) + set_lifecycle_tracker(tracker) + stop_container("01ROBOTTEST0000000000020", run_command=runner) + final = get_lifecycle_tracker("01ROBOTTEST0000000000020") + assert final.current_state == ContainerLifecycleState.STOPPED + assert len(runner.stop_calls) == 1 + print("stop-container-ok") + + +def cmd_rebuild_container() -> None: + """Verify rebuild transitions stopped to running.""" + clear_lifecycle_registry() + runner = _MockRunner() + runner.set_up_result(container_id="aabbccddee0033445566", workspace="/ws") + tracker = ContainerLifecycleTracker( + resource_id="01ROBOTTEST0000000000030", + current_state=ContainerLifecycleState.STOPPED, + ) + set_lifecycle_tracker(tracker) + # R16 fix: call rebuild_container instead of activate_container + rebuild_container( + "01ROBOTTEST0000000000030", + "/workspace", + run_command=runner, + ) + final = get_lifecycle_tracker("01ROBOTTEST0000000000030") + assert final.current_state == ContainerLifecycleState.RUNNING + print("rebuild-container-ok") + + +def cmd_session_cleanup() -> None: + """Verify session cleanup stops all active containers.""" + clear_lifecycle_registry() + runner = _MockRunner() + for rid in ("01ROBOTTEST0000000000040", "01ROBOTTEST0000000000041"): + tracker = ContainerLifecycleTracker( + resource_id=rid, + current_state=ContainerLifecycleState.RUNNING, + container_id=f"ctr-{rid[-3:]}", + ) + set_lifecycle_tracker(tracker) + stopped = stop_all_active_containers(run_command=runner, session_id="ses-001") + assert len(stopped) == 2 + for rid in ("01ROBOTTEST0000000000040", "01ROBOTTEST0000000000041"): + t = get_lifecycle_tracker(rid) + assert t.current_state == ContainerLifecycleState.STOPPED + print("session-cleanup-ok") + + +def cmd_json_parsing() -> None: + """Verify devcontainer up JSON output parsing.""" + stdout = json.dumps( + { + "outcome": "success", + "containerId": "aabbccddee0044556677", + "remoteWorkspaceFolder": "/parsed/ws", + } + ) + ctr_id, ws = _parse_devcontainer_up_output(stdout) + assert ctr_id == "aabbccddee0044556677" + assert ws == "/parsed/ws" + + # Invalid JSON + ctr_id2, ws2 = _parse_devcontainer_up_output("not json {{") + assert ctr_id2 is None + assert ws2 is None + print("json-parsing-ok") + + +def cmd_registry_persist() -> None: + """Verify tracker persists in registry.""" + clear_lifecycle_registry() + tracker = ContainerLifecycleTracker(resource_id="01ROBOTTEST0000000000060") + set_lifecycle_tracker(tracker) + tracker2 = transition_state(tracker, ContainerLifecycleState.BUILDING, reason="t") + set_lifecycle_tracker(tracker2) + retrieved = get_lifecycle_tracker("01ROBOTTEST0000000000060") + assert retrieved.current_state == ContainerLifecycleState.BUILDING + print("registry-persist-ok") + + +def cmd_cleanup_method() -> None: + """Verify CleanupService.stop_active_devcontainers exists.""" + assert hasattr(CleanupService, "stop_active_devcontainers") + assert callable(CleanupService.stop_active_devcontainers) + print("cleanup-method-ok") + + +_COMMANDS = { + "enum-values": cmd_enum_values, + "transition-valid": cmd_transition_valid, + "transition-invalid": cmd_transition_invalid, + "lazy-activation": cmd_lazy_activation, + "stop-container": cmd_stop_container, + "rebuild-container": cmd_rebuild_container, + "session-cleanup": cmd_session_cleanup, + "json-parsing": cmd_json_parsing, + "registry-persist": cmd_registry_persist, + "cleanup-method": cmd_cleanup_method, +} + + +def main() -> None: + """Dispatch subcommand from argv.""" + if len(sys.argv) < 2: + print(f"Usage: {sys.argv[0]} ", file=sys.stderr) + sys.exit(1) + cmd = sys.argv[1] + func = _COMMANDS.get(cmd) + if func is None: + print(f"Unknown command: {cmd}", file=sys.stderr) + sys.exit(1) + func() + + +if __name__ == "__main__": + main() diff --git a/src/cleveragents/acp/facade.py b/src/cleveragents/acp/facade.py index 025517144..3628d2e7f 100644 --- a/src/cleveragents/acp/facade.py +++ b/src/cleveragents/acp/facade.py @@ -81,6 +81,9 @@ class AcpLocalFacade: if services is not None and not isinstance(services, dict): raise TypeError("services must be a dict or None") self._services: dict[str, Any] = dict(services) if services else {} + # PERF-1 fix: cache the handler dispatch dict instead of + # rebuilding it on every ACP request. + self._handler_map: dict[str, Any] | None = None # ------------------------------------------------------------------ # Service accessors (typed, nullable) @@ -167,6 +170,9 @@ class AcpLocalFacade: if not name or not isinstance(name, str): raise ValueError("name must be a non-empty string") self._services[name] = service + # PERF-1 fix: invalidate cached handler map so new service + # wiring is picked up on the next dispatch. + self._handler_map = None logger.debug("acp.local.service_registered", service_name=name) def list_operations(self) -> list[str]: @@ -192,20 +198,26 @@ class AcpLocalFacade: return handler(params) def _handlers(self) -> dict[str, Any]: - """Build the operation -> handler mapping.""" - return { - "session.create": self._handle_session_create, - "session.close": self._handle_session_close, - "plan.create": self._handle_plan_create, - "plan.execute": self._handle_plan_execute, - "plan.status": self._handle_plan_status, - "plan.diff": self._handle_plan_diff, - "plan.apply": self._handle_plan_apply, - "registry.list_tools": self._handle_registry_list_tools, - "registry.list_resources": self._handle_registry_list_resources, - "context.get": self._handle_context_get, - "event.subscribe": self._handle_event_subscribe, - } + """Return the cached operation -> handler mapping. + + PERF-1 fix: the dispatch dict is built once and cached. + Invalidated when a new service is registered. + """ + if self._handler_map is None: + self._handler_map = { + "session.create": self._handle_session_create, + "session.close": self._handle_session_close, + "plan.create": self._handle_plan_create, + "plan.execute": self._handle_plan_execute, + "plan.status": self._handle_plan_status, + "plan.diff": self._handle_plan_diff, + "plan.apply": self._handle_plan_apply, + "registry.list_tools": self._handle_registry_list_tools, + "registry.list_resources": self._handle_registry_list_resources, + "context.get": self._handle_context_get, + "event.subscribe": self._handle_event_subscribe, + } + return self._handler_map # ------------------------------------------------------------------ # Operation handlers — session @@ -220,15 +232,52 @@ class AcpLocalFacade: return {"session_id": session.session_id, "status": "created"} def _handle_session_close(self, params: dict[str, Any]) -> dict[str, Any]: + session_id = params.get("session_id", "") + svc = self._session_service if svc is None: + # R7-F4 fix: still run container cleanup even without a + # session service — containers may exist in local/test mode. + self._cleanup_session_devcontainers(session_id) return {"status": "closed"} - session_id = params.get("session_id", "") + if not session_id: raise ValueError("session_id is required") svc.delete(session_id) + + # R7-F4 fix: run container cleanup after session deletion. + self._cleanup_session_devcontainers(session_id) return {"status": "closed"} + def _cleanup_session_devcontainers(self, session_id: str) -> None: + """Best-effort stop of devcontainers associated with a session. + + Failures are logged but never propagated so session close always + succeeds. + """ + if not session_id: + return + try: + from cleveragents.application.services.cleanup_service import ( + CleanupService, + ) + + stopped = CleanupService.stop_active_devcontainers( + session_id=session_id, + ) + if stopped: + logger.info( + "acp.session.close.devcontainer_cleanup", + session_id=session_id, + stopped_count=len(stopped), + ) + except Exception: + logger.warning( + "acp.session.close.devcontainer_cleanup_failed", + session_id=session_id, + exc_info=True, + ) + # ------------------------------------------------------------------ # Operation handlers — plan lifecycle # ------------------------------------------------------------------ diff --git a/src/cleveragents/application/services/cleanup_models.py b/src/cleveragents/application/services/cleanup_models.py new file mode 100644 index 000000000..847cc11b1 --- /dev/null +++ b/src/cleveragents/application/services/cleanup_models.py @@ -0,0 +1,91 @@ +"""Data classes for cleanup service reports. + +Extracted from ``cleanup_service.py`` to keep that module under the +500-line limit per CONTRIBUTING (F8 fix). +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +__all__ = [ + "CleanupReport", + "ResourceCleanupSummary", + "StaleItem", +] + + +@dataclass(slots=True) +class StaleItem: + """A single item identified for cleanup.""" + + resource_type: str + path: str + age_description: str + plan_id: str | None = None + + +@dataclass(slots=True) +class ResourceCleanupSummary: + """Per-resource-type cleanup result.""" + + resource_type: str + scanned: int = 0 + removed: int = 0 + skipped: int = 0 + skipped_reasons: list[str] = field(default_factory=list) + + +@dataclass(slots=True) +class CleanupReport: + """Full cleanup report across all resource types.""" + + dry_run: bool = False + sandboxes: ResourceCleanupSummary = field( + default_factory=lambda: ResourceCleanupSummary(resource_type="sandboxes"), + ) + checkpoints: ResourceCleanupSummary = field( + default_factory=lambda: ResourceCleanupSummary(resource_type="checkpoints"), + ) + sessions: ResourceCleanupSummary = field( + default_factory=lambda: ResourceCleanupSummary(resource_type="sessions"), + ) + logs: ResourceCleanupSummary = field( + default_factory=lambda: ResourceCleanupSummary(resource_type="logs"), + ) + backups: ResourceCleanupSummary = field( + default_factory=lambda: ResourceCleanupSummary(resource_type="backups"), + ) + stale_items: list[StaleItem] = field(default_factory=list) + + def as_dict(self) -> dict[str, Any]: + """Serialize the report for CLI output.""" + return { + "dry_run": self.dry_run, + "sandboxes": { + "scanned": self.sandboxes.scanned, + "removed": self.sandboxes.removed, + "skipped": self.sandboxes.skipped, + }, + "checkpoints": { + "scanned": self.checkpoints.scanned, + "removed": self.checkpoints.removed, + "skipped": self.checkpoints.skipped, + }, + "sessions": { + "scanned": self.sessions.scanned, + "removed": self.sessions.removed, + "skipped": self.sessions.skipped, + }, + "logs": { + "scanned": self.logs.scanned, + "removed": self.logs.removed, + "skipped": self.logs.skipped, + }, + "backups": { + "scanned": self.backups.scanned, + "removed": self.backups.removed, + "skipped": self.backups.skipped, + }, + } diff --git a/src/cleveragents/application/services/cleanup_service.py b/src/cleveragents/application/services/cleanup_service.py index 57d4e27b5..8406ed986 100644 --- a/src/cleveragents/application/services/cleanup_service.py +++ b/src/cleveragents/application/services/cleanup_service.py @@ -10,12 +10,17 @@ from __future__ import annotations import shutil import tempfile import time -from dataclasses import dataclass, field from datetime import UTC, datetime from pathlib import Path from typing import Any +from cleveragents.application.services.cleanup_models import ( + CleanupReport, + ResourceCleanupSummary, + StaleItem, +) from cleveragents.config.settings import Settings +from cleveragents.resource.handlers.devcontainer import stop_all_active_containers __all__ = [ "CleanupReport", @@ -25,84 +30,6 @@ __all__ = [ ] -# ── Data classes ────────────────────────────────────────────────── - - -@dataclass(slots=True) -class StaleItem: - """A single item identified for cleanup.""" - - resource_type: str - path: str - age_description: str - plan_id: str | None = None - - -@dataclass(slots=True) -class ResourceCleanupSummary: - """Per-resource-type cleanup result.""" - - resource_type: str - scanned: int = 0 - removed: int = 0 - skipped: int = 0 - skipped_reasons: list[str] = field(default_factory=list) - - -@dataclass(slots=True) -class CleanupReport: - """Full cleanup report across all resource types.""" - - dry_run: bool = False - sandboxes: ResourceCleanupSummary = field( - default_factory=lambda: ResourceCleanupSummary(resource_type="sandboxes"), - ) - checkpoints: ResourceCleanupSummary = field( - default_factory=lambda: ResourceCleanupSummary(resource_type="checkpoints"), - ) - sessions: ResourceCleanupSummary = field( - default_factory=lambda: ResourceCleanupSummary(resource_type="sessions"), - ) - logs: ResourceCleanupSummary = field( - default_factory=lambda: ResourceCleanupSummary(resource_type="logs"), - ) - backups: ResourceCleanupSummary = field( - default_factory=lambda: ResourceCleanupSummary(resource_type="backups"), - ) - stale_items: list[StaleItem] = field(default_factory=list) - - def as_dict(self) -> dict[str, Any]: - """Serialize the report for CLI output.""" - return { - "dry_run": self.dry_run, - "sandboxes": { - "scanned": self.sandboxes.scanned, - "removed": self.sandboxes.removed, - "skipped": self.sandboxes.skipped, - }, - "checkpoints": { - "scanned": self.checkpoints.scanned, - "removed": self.checkpoints.removed, - "skipped": self.checkpoints.skipped, - }, - "sessions": { - "scanned": self.sessions.scanned, - "removed": self.sessions.removed, - "skipped": self.sessions.skipped, - }, - "logs": { - "scanned": self.logs.scanned, - "removed": self.logs.removed, - "skipped": self.logs.skipped, - }, - "backups": { - "scanned": self.backups.scanned, - "removed": self.backups.removed, - "skipped": self.backups.skipped, - }, - } - - # ── Service ─────────────────────────────────────────────────────── @@ -504,6 +431,25 @@ class CleanupService: except OSError: report.backups.skipped += 1 + # ── Devcontainer lifecycle cleanup (issue #514) ───────────── + + @staticmethod + def stop_active_devcontainers(session_id: str = "") -> list[str]: + """Stop all active devcontainer-instance resources. + + Called as a cleanup hook when a session closes or a plan + completes. Delegates to the devcontainer handler's + ``stop_all_active_containers`` function. + + Args: + session_id: Optional session/plan ID for logging. + + Returns: + List of resource IDs that were stopped. + """ + # F11 fix: import moved to module top per CONTRIBUTING. + return stop_all_active_containers(session_id=session_id) + # ── Helpers ─────────────────────────────────────────────────── @staticmethod diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index e4e0651e2..619b81847 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -1056,6 +1056,9 @@ class PlanLifecycleService: self._commit_plan(plan) self._logger.error("Execute failed", plan_id=plan_id, error=error_message) + # R7-F2 fix: clean up containers on execute failure (terminal state). + self._cleanup_devcontainers(plan_id) + return plan def apply_plan(self, plan_id: str) -> Plan: @@ -1160,6 +1163,9 @@ class PlanLifecycleService: phase=plan.phase.value, ) + # F1-r6 fix: wire container cleanup to plan completion hook. + self._cleanup_devcontainers(plan_id) + return plan def constrain_apply(self, plan_id: str, reason: str) -> Plan: @@ -1205,6 +1211,9 @@ class PlanLifecycleService: self._commit_plan(plan) self._logger.error("Apply failed", plan_id=plan_id, error=error_message) + # R7-F2 fix: clean up containers on apply failure (terminal state). + self._cleanup_devcontainers(plan_id) + return plan def cancel_plan(self, plan_id: str, reason: str | None = None) -> Plan: @@ -1235,8 +1244,41 @@ class PlanLifecycleService: self._commit_plan(plan) self._logger.info("Plan cancelled", plan_id=plan_id, reason=reason) + # F1-r6 fix: wire container cleanup to plan cancellation hook. + self._cleanup_devcontainers(plan_id) + return plan + # -- Devcontainer cleanup helper (F1-r6) -------------------------- + + def _cleanup_devcontainers(self, plan_id: str) -> None: + """Best-effort stop of active devcontainers on plan completion. + + Called from terminal plan states (``complete_apply``, + ``cancel_plan``) to ensure containers don't outlive the plan. + Failures are logged but never propagated. + """ + try: + from cleveragents.application.services.cleanup_service import ( + CleanupService, + ) + + stopped = CleanupService.stop_active_devcontainers( + session_id=plan_id, + ) + if stopped: + self._logger.info( + "plan.devcontainer_cleanup", + plan_id=plan_id, + stopped_count=len(stopped), + ) + except Exception: + self._logger.warning( + "plan.devcontainer_cleanup_failed", + plan_id=plan_id, + exc_info=True, + ) + # -- Profile-based auto-progress helpers ------------------------- def _resolve_profile_for_plan(self, plan: Plan) -> AutomationProfile: diff --git a/src/cleveragents/cli/commands/resource.py b/src/cleveragents/cli/commands/resource.py index 2a3904743..81e861a25 100644 --- a/src/cleveragents/cli/commands/resource.py +++ b/src/cleveragents/cli/commands/resource.py @@ -72,6 +72,14 @@ from cleveragents.core.exceptions import ( NotFoundError, ValidationError, ) +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, +) +from cleveragents.resource.handlers.devcontainer import ( + get_lifecycle_tracker, + rebuild_container, + stop_container, +) from cleveragents.resource.inheritance import ResourceTypeParentRemovalError logger = logging.getLogger(__name__) @@ -846,7 +854,12 @@ def _read_resource_file(resource: Any, file_path: str) -> str: if not location: return "(no location set for this resource)" - full_path = Path(location) / file_path + # S3 fix: guard against path traversal (e.g. "../../etc/passwd") + base = Path(location).resolve() + full_path = (base / file_path).resolve() + if not full_path.is_relative_to(base): + return f"(path traversal rejected: {file_path})" + if not full_path.exists(): return f"(file not found: {full_path})" @@ -1052,3 +1065,163 @@ def resource_remove( except CleverAgentsError as exc: console.print(f"[red]Error:[/red] {exc.message}") raise typer.Abort() from exc + + +# --------------------------------------------------------------------------- +# Devcontainer lifecycle commands (issue #514) +# --------------------------------------------------------------------------- + +# B2 fix: separate type sets — stop works on any container, but rebuild +# requires devcontainer-instance since it invokes ``devcontainer up``. +# F19 fix: restricted to devcontainer-instance only — container-instance +# has no lifecycle tracker wiring, so stop/rebuild would fail at runtime. +_STOPPABLE_TYPES = frozenset({"devcontainer-instance"}) +_REBUILDABLE_TYPES = frozenset({"devcontainer-instance"}) +# F5-r6 fix: moved from inside resource_rebuild() to module level to avoid +# per-call allocation (consistent with _STOPPABLE_TYPES / _REBUILDABLE_TYPES). +_REBUILD_ALLOWED = frozenset( + {ContainerLifecycleState.STOPPED, ContainerLifecycleState.FAILED} +) + + +@app.command("stop") +def resource_stop( + name: Annotated[ + str, + typer.Argument(help="Devcontainer resource name or ULID to stop"), + ], + # A2 fix: add --yes/-y confirmation flag to skip interactive prompt + yes: Annotated[ + bool, + typer.Option("--yes", "-y", help="Skip confirmation prompt"), + ] = False, +) -> None: + """Stop a running devcontainer-instance resource. + + Transitions the container from ``running`` → ``stopping`` → ``stopped``. + Only devcontainer-instance and container-instance resources + may be stopped. + + Use ``--yes`` / ``-y`` to skip the confirmation prompt in scripts. + + Examples: + agents resource stop local/my-devcontainer + agents resource stop local/my-devcontainer --yes + """ + try: + service = _get_registry_service() + res = service.show_resource(name) + + if res.resource_type_name not in _STOPPABLE_TYPES: + console.print( + f"[red]Resource '{name}' is type '{res.resource_type_name}', " + f"not a stoppable container type.[/red]" + ) + raise typer.Abort() + + # R12 fix: validate lifecycle state before delegating + tracker = get_lifecycle_tracker(res.resource_id) + if tracker.current_state != ContainerLifecycleState.RUNNING: + console.print( + f"[red]Cannot stop:[/red] container is in " + f"'{tracker.current_state.value}' state (must be 'running')." + ) + raise typer.Abort() + + if not yes: + typer.confirm( + f"Stop container '{res.name or res.resource_id}'?", + abort=True, + ) + + console.print( + f"Stopping container {res.name or res.resource_id} " + f"(state: {tracker.current_state.value})..." + ) + + stop_container(res.resource_id) + console.print(f"[green]Stopped:[/green] {res.name or res.resource_id}") + + except NotFoundError as exc: + console.print(f"[red]Resource not found:[/red] {name}") + raise typer.Abort() from exc + except (ValueError, RuntimeError) as exc: + console.print(f"[red]Stop failed:[/red] {exc}") + raise typer.Abort() from exc + except CleverAgentsError as exc: + console.print(f"[red]Error:[/red] {exc.message}") + raise typer.Abort() from exc + + +@app.command("rebuild") +def resource_rebuild( + name: Annotated[ + str, + typer.Argument(help="Devcontainer resource name or ULID to rebuild"), + ], + # A2 fix: add --yes/-y confirmation flag to skip interactive prompt + yes: Annotated[ + bool, + typer.Option("--yes", "-y", help="Skip confirmation prompt"), + ] = False, +) -> None: + """Rebuild a stopped or failed devcontainer-instance resource. + + Transitions: ``stopped``/``failed`` → ``building`` → ``running``. + Only devcontainer-instance resources may be rebuilt. + + Use ``--yes`` / ``-y`` to skip the confirmation prompt in scripts. + + Examples: + agents resource rebuild local/my-devcontainer + agents resource rebuild local/my-devcontainer --yes + """ + try: + service = _get_registry_service() + res = service.show_resource(name) + + if res.resource_type_name not in _REBUILDABLE_TYPES: + console.print( + f"[red]Resource '{name}' is type '{res.resource_type_name}', " + f"not a devcontainer-instance (rebuild requires devcontainer).[/red]" + ) + raise typer.Abort() + + # R12 fix: validate lifecycle state before delegating + tracker = get_lifecycle_tracker(res.resource_id) + if tracker.current_state not in _REBUILD_ALLOWED: + console.print( + f"[red]Cannot rebuild:[/red] container is in " + f"'{tracker.current_state.value}' state " + f"(must be 'stopped' or 'failed')." + ) + raise typer.Abort() + + location = res.location + if not location and res.properties: + path_val = res.properties.get("path") + location = str(path_val) if path_val is not None else None + + if not location: + console.print(f"[red]Resource '{name}' has no location for rebuild.[/red]") + raise typer.Abort() + + if not yes: + typer.confirm( + f"Rebuild container '{res.name or res.resource_id}'?", + abort=True, + ) + + console.print(f"Rebuilding {res.name or res.resource_id}...") + rebuild_container(res.resource_id, location) + console.print(f"[green]Rebuilt:[/green] {res.name or res.resource_id}") + + except NotFoundError as exc: + console.print(f"[red]Resource not found:[/red] {name}") + raise typer.Abort() from exc + except (ValueError, RuntimeError) as exc: + console.print(f"[red]Rebuild failed:[/red] {exc}") + raise typer.Abort() from exc + except CleverAgentsError as exc: + console.print(f"[red]Error:[/red] {exc.message}") + raise typer.Abort() from exc diff --git a/src/cleveragents/domain/models/core/container_lifecycle.py b/src/cleveragents/domain/models/core/container_lifecycle.py new file mode 100644 index 000000000..a45ed5d97 --- /dev/null +++ b/src/cleveragents/domain/models/core/container_lifecycle.py @@ -0,0 +1,286 @@ +"""Container lifecycle state model for devcontainer resources. + +Defines the :class:`ContainerLifecycleState` enum and transition +validation logic for devcontainer-instance resources. + +## State Machine + +:: + + detected ──► building ──► running ──► stopping ──► stopped + │ │ + ▼ │ + failed ◄───────────────────────────────┘ + │ ▲ + └──► building │ + (retry) (rebuild) + │ + stopped ───────┘ + +Valid transitions: + +| From | To | Trigger | +|-----------|---------------------|-----------------------| +| detected | building | lazy activation | +| building | running | devcontainer up OK | +| building | failed | devcontainer up fail | +| running | stopping | stop command | +| stopping | stopped | container stopped | +| stopping | failed | stop failed | +| stopped | building | rebuild | +| failed | building | retry | + +Based on: + - Issue #514: Devcontainer lifecycle management + - ADR-043: Devcontainer Integration +""" + +from __future__ import annotations + +import logging +from datetime import UTC, datetime +from enum import StrEnum + +from pydantic import BaseModel, ConfigDict, Field + +logger = logging.getLogger(__name__) + + +class ContainerLifecycleState(StrEnum): + """Lifecycle state for devcontainer-instance resources. + + State names align with the ``activation_state`` enum defined in + ``docs/specification.md`` (devcontainer-instance resource type). + ``stopping`` is an implementation-level intermediate state added + for clean shutdown tracking. + + .. note:: SPEC-1 reconciliation + + Issue #514 acceptance criteria use a different vocabulary: + ``inactive/starting/active/stopping/stopped/error``. After + design review these were renamed to better reflect the + container lifecycle semantics: + + | Issue #514 | Implementation | Reason | + |-------------|----------------|---------------------------------| + | inactive | detected | container exists but not started | + | starting | building | ``devcontainer up`` is running | + | active | running | container is healthy | + | stopping | stopping | (unchanged) | + | stopped | stopped | (unchanged) | + | error | failed | aligns with Docker terminology | + + The specification (``docs/specification.md``) uses the + implementation names. Issue #514 should be updated to match. + + | Value | Description | + |------------|----------------------------------------------------| + | `detected` | Container not yet started (default for new) | + | `building` | Container is being activated (devcontainer up) | + | `running` | Container is running and healthy | + | `stopping` | Container is being stopped | + | `stopped` | Container has been stopped cleanly | + | `failed` | Container is in an error state | + """ + + DETECTED = "detected" + BUILDING = "building" + RUNNING = "running" + STOPPING = "stopping" + STOPPED = "stopped" + FAILED = "failed" + + +#: Valid state transitions as a mapping from source to allowed targets. +VALID_TRANSITIONS: dict[ContainerLifecycleState, frozenset[ContainerLifecycleState]] = { + ContainerLifecycleState.DETECTED: frozenset({ContainerLifecycleState.BUILDING}), + ContainerLifecycleState.BUILDING: frozenset( + {ContainerLifecycleState.RUNNING, ContainerLifecycleState.FAILED} + ), + ContainerLifecycleState.RUNNING: frozenset({ContainerLifecycleState.STOPPING}), + ContainerLifecycleState.STOPPING: frozenset( + {ContainerLifecycleState.STOPPED, ContainerLifecycleState.FAILED} + ), + ContainerLifecycleState.STOPPED: frozenset({ContainerLifecycleState.BUILDING}), + ContainerLifecycleState.FAILED: frozenset({ContainerLifecycleState.BUILDING}), +} + + +class LifecycleTransition(BaseModel): + """Record of a single lifecycle state transition. + + Captures the from/to states, timestamp, and an optional reason + string for audit and debugging purposes. + """ + + from_state: ContainerLifecycleState + to_state: ContainerLifecycleState + timestamp: datetime = Field(default_factory=lambda: datetime.now(tz=UTC)) + reason: str = "" + + model_config = ConfigDict(frozen=True) + + +class ContainerLifecycleTracker(BaseModel): + """Tracks lifecycle state and transition history for a container resource. + + This model is stored separately from the frozen ``Resource`` model + and is keyed by ``resource_id`` in the resource registry. + + Attributes: + resource_id: The ULID of the tracked resource. + current_state: Current lifecycle state. + container_id: Docker/devcontainer container ID (set after start). + workspace_path: Workspace mount path inside the container. + health_check_interval: Seconds between health probes. + transitions: Ordered tuple of state transitions (immutable). + """ + + resource_id: str = Field( + ..., + min_length=1, + description="ULID of the tracked devcontainer resource", + ) + session_id: str | None = Field( + default=None, + description="Session or plan ID that owns this container (for scoped cleanup)", + ) + current_state: ContainerLifecycleState = Field( + default=ContainerLifecycleState.DETECTED, + description="Current lifecycle state", + ) + container_id: str | None = Field( + default=None, + description="Container ID from devcontainer up output", + ) + workspace_path: str | None = Field( + default=None, + description="Workspace mount path inside the container", + ) + host_workspace_path: str | None = Field( + default=None, + description=( + "Host-side workspace folder used with devcontainer CLI " + "(F15 fix: devcontainer exec --workspace-folder expects the host path)" + ), + ) + health_check_interval: float = Field( + default=30.0, + ge=10, + le=3600, + description="Seconds between health check probes (min 10, max 3600)", + ) + # BUG-4 fix: use tuple instead of list to prevent in-place mutation + # of the shared transition history from concurrent threads. + transitions: tuple[LifecycleTransition, ...] = Field( + default_factory=tuple, + description="Ordered transition history", + ) + + model_config = ConfigDict(str_strip_whitespace=True) + + +def validate_transition( + from_state: ContainerLifecycleState, + to_state: ContainerLifecycleState, +) -> bool: + """Check whether a state transition is valid. + + Args: + from_state: Current state. + to_state: Desired target state. + + Returns: + ``True`` if the transition is allowed, ``False`` otherwise. + """ + if not isinstance(from_state, ContainerLifecycleState): + raise TypeError( + f"from_state must be ContainerLifecycleState, " + f"got {type(from_state).__name__}" + ) + if not isinstance(to_state, ContainerLifecycleState): + raise TypeError( + f"to_state must be ContainerLifecycleState, got {type(to_state).__name__}" + ) + allowed = VALID_TRANSITIONS.get(from_state, frozenset()) + return to_state in allowed + + +def transition_state( + tracker: ContainerLifecycleTracker, + to_state: ContainerLifecycleState, + reason: str = "", +) -> ContainerLifecycleTracker: + """Transition a tracker to a new state with validation. + + Args: + tracker: The current lifecycle tracker. + to_state: Desired target state. + reason: Human-readable reason for the transition. + + Returns: + A new tracker instance with the updated state and transition logged. + + Raises: + ValueError: If the transition is not valid. + """ + if not isinstance(tracker, ContainerLifecycleTracker): + raise TypeError( + f"tracker must be ContainerLifecycleTracker, got {type(tracker).__name__}" + ) + if not isinstance(to_state, ContainerLifecycleState): + raise TypeError( + f"to_state must be ContainerLifecycleState, got {type(to_state).__name__}" + ) + + from_state = tracker.current_state + if not validate_transition(from_state, to_state): + raise ValueError( + f"Invalid lifecycle transition: {from_state.value} -> {to_state.value}" + ) + + transition = LifecycleTransition( + from_state=from_state, + to_state=to_state, + reason=reason, + ) + + # Cap history to prevent unbounded growth in long-running containers. + max_history = 100 + new_transitions = [*tracker.transitions, transition] + if len(new_transitions) > max_history: + new_transitions = new_transitions[-max_history:] + + logger.info( + "Container %s: %s -> %s (%s)", + tracker.resource_id, + from_state.value, + to_state.value, + reason or "no reason", + ) + + # R10 fix: clear stale container_id and workspace_path when + # transitioning to FAILED or STOPPED so downstream code doesn't + # get false positives from a defunct container reference. + # BUG-2 fix: also clear on BUILDING to prevent stale references + # from a previous lifecycle being visible during rebuild/retry. + # R8-F4 fix: use model_copy to avoid fragile manual reconstruction. + updates: dict[str, object] = { + "current_state": to_state, + "transitions": tuple(new_transitions), + } + if to_state in ( + ContainerLifecycleState.BUILDING, + ContainerLifecycleState.FAILED, + ContainerLifecycleState.STOPPED, + ): + updates["container_id"] = None + updates["workspace_path"] = None + + # BUG-1 note: model_copy(update=...) intentionally bypasses Pydantic + # validators. This is safe here because the updated fields + # (current_state, transitions, container_id, workspace_path) have no + # custom validators that need re-running. If a future change adds + # validated fields to the update dict, switch to + # ``model_validate(tracker.model_dump() | updates)`` instead. + return tracker.model_copy(update=updates) diff --git a/src/cleveragents/resource/handlers/_devcontainer_internals.py b/src/cleveragents/resource/handlers/_devcontainer_internals.py new file mode 100644 index 000000000..ac593f4e3 --- /dev/null +++ b/src/cleveragents/resource/handlers/_devcontainer_internals.py @@ -0,0 +1,152 @@ +"""Shared state and utilities for devcontainer lifecycle modules. + +Internal module providing the lifecycle registry, thread-safe +accessors, constants, and output-parsing helpers shared by +``devcontainer_lifecycle``, ``devcontainer_health``, and +``devcontainer_cleanup``. + +Extracted from ``devcontainer.py`` to break circular dependencies +while keeping files under the 500-line limit (F28 fix). + +.. warning:: + This is an internal module. External code should import from + ``cleveragents.resource.handlers.devcontainer`` instead. +""" + +from __future__ import annotations + +import json +import logging +import re +import threading + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, +) + +logger = logging.getLogger(__name__) + +# ── Constants ──────────────────────────────────────────────────── + +#: Default health check interval in seconds. +DEFAULT_HEALTH_CHECK_INTERVAL: float = 30.0 + +#: S1 fix: regex pattern for valid Docker container IDs (12-64 hex chars). +_CONTAINER_ID_PATTERN = re.compile(r"^[a-f0-9]{12,64}$") + +#: Maximum number of terminal-state trackers before eviction. +_MAX_TERMINAL_TRACKERS: int = 200 + +# ── Shared mutable state ───────────────────────────────────────── + +#: Registry of lifecycle trackers keyed by resource_id. +_lifecycle_registry: dict[str, ContainerLifecycleTracker] = {} +_registry_lock = threading.RLock() + +#: Background health check threads keyed by resource_id. +_health_check_threads: dict[str, threading.Thread] = {} +_health_check_stop_events: dict[str, threading.Event] = {} + +# ── Registry accessors ─────────────────────────────────────────── + + +def get_lifecycle_tracker(resource_id: str) -> ContainerLifecycleTracker: + """Get or create a lifecycle tracker for a resource. + + Args: + resource_id: The ULID of the devcontainer resource. + + Returns: + The current lifecycle tracker. + """ + if not resource_id: + raise ValueError("resource_id must not be empty") + with _registry_lock: + if resource_id not in _lifecycle_registry: + _lifecycle_registry[resource_id] = ContainerLifecycleTracker( + resource_id=resource_id, + ) + return _lifecycle_registry[resource_id] + + +def set_lifecycle_tracker(tracker: ContainerLifecycleTracker) -> None: + """Update the lifecycle tracker in the registry. + + Args: + tracker: The tracker to store. + """ + if not isinstance(tracker, ContainerLifecycleTracker): + raise TypeError("tracker must be a ContainerLifecycleTracker") + with _registry_lock: + _lifecycle_registry[tracker.resource_id] = tracker + + +def list_active_containers() -> list[str]: + """Return resource IDs of all containers in the ``running`` state. + + Returns: + List of resource_id strings. + """ + with _registry_lock: + return [ + rid + for rid, tracker in _lifecycle_registry.items() + if tracker.current_state == ContainerLifecycleState.RUNNING + ] + + +# ── Output parsing ─────────────────────────────────────────────── + + +def _parse_devcontainer_up_output(stdout: str) -> tuple[str | None, str | None]: + """Parse JSON output from ``devcontainer up``. + + The devcontainer CLI emits JSON like:: + + {"outcome": "success", "containerId": "abc123", + "remoteWorkspaceFolder": "/workspaces/project"} + + The CLI may emit log lines before the final JSON object (R4 fix). + This function scans lines in reverse to find the last valid JSON + object containing the expected fields. + + Args: + stdout: Raw stdout from devcontainer up. + + Returns: + Tuple of (container_id, workspace_path). Either may be None + if the field is absent. + """ + container_id: str | None = None + workspace_path: str | None = None + + # R4 fix: iterate lines in reverse to handle log lines before JSON + for line in reversed(stdout.splitlines()): + line = line.strip() + if not line: + continue + try: + data = json.loads(line) + if isinstance(data, dict): + raw_id = data.get("containerId") + # S1 fix: validate container_id format (12-64 hex chars) + if raw_id and _CONTAINER_ID_PATTERN.match(raw_id): + container_id = raw_id + elif raw_id: + logger.warning("Ignoring invalid container ID format: %s", raw_id) + # F11 fix: validate remoteWorkspaceFolder is an absolute + # path (must start with "/"), consistent with S1 + # container ID validation. + raw_ws = data.get("remoteWorkspaceFolder") + if raw_ws and isinstance(raw_ws, str) and raw_ws.startswith("/"): + workspace_path = raw_ws + elif raw_ws: + logger.warning("Ignoring invalid workspace path: %s", raw_ws) + return container_id, workspace_path + except (json.JSONDecodeError, TypeError): + continue + + # B3 fix: removed redundant fallback — the reverse-line loop above + # already handles single-line JSON output. + return container_id, workspace_path diff --git a/src/cleveragents/resource/handlers/devcontainer.py b/src/cleveragents/resource/handlers/devcontainer.py index 87b6d2f7b..89c319084 100644 --- a/src/cleveragents/resource/handlers/devcontainer.py +++ b/src/cleveragents/resource/handlers/devcontainer.py @@ -1,38 +1,201 @@ """Devcontainer resource handler for CleverAgents. Resolves ``devcontainer-instance`` and ``devcontainer-file`` resources -into sandbox-backed :class:`BoundResource` instances using the -``snapshot`` sandbox strategy. +into sandbox-backed :class:`BoundResource` instances using the ``none`` +sandbox strategy (the container itself provides isolation — F22/F25 fix). The handler: -1. Validates that the resource has a non-empty ``location``. -2. Determines the sandbox strategy: resource-level override takes - precedence over the default ``snapshot``. -3. Calls :meth:`SandboxManager.get_or_create_sandbox` to provision - (or reuse) an isolated snapshot. -4. Returns a :class:`BoundResource` with ``sandbox_path`` set to the +1. For ``devcontainer-instance``: triggers lazy activation via + ``activate_container()`` when the resource is in a non-running + state (F16 fix). +2. Validates that the resource has a non-empty ``location``. +3. Determines the sandbox strategy: resource-level override takes + precedence over the default ``none``. +4. Calls :meth:`SandboxManager.get_or_create_sandbox` to provision + (or reuse) the sandbox. +5. Returns a :class:`BoundResource` with ``sandbox_path`` set to the sandbox root. +## Lifecycle Management (issue #514) + +Devcontainer-instance resources support a full lifecycle: + +- **Lazy activation**: containers start on first tool invocation. +- **Health checking**: periodic liveness probes via ``devcontainer exec``. +- **Manual stop / rebuild**: CLI commands for explicit control. + +Implementation is split across focused modules to comply with the +500-line file limit (F28 fix): + +- ``_devcontainer_internals`` — shared registry state, accessors, parsing +- ``devcontainer_health`` — background health check threads +- ``devcontainer_lifecycle`` — activate / stop / rebuild operations +- ``devcontainer_cleanup`` — session cleanup and tracker eviction + +This module re-exports all public (and selected private) symbols from +the sub-modules so that existing import paths continue to work. + Based on: - docs/specification.md -- Devcontainer, Resource Types - ADR-043: Devcontainer Integration - ADR-042: Resource Type Inheritance + - Issue #514: Devcontainer lifecycle management """ from __future__ import annotations -from cleveragents.domain.models.core.resource import SandboxStrategy +import logging + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, +) +from cleveragents.domain.models.core.resource import Resource, SandboxStrategy +from cleveragents.infrastructure.sandbox.manager import SandboxManager from cleveragents.resource.handlers._base import BaseResourceHandler +from cleveragents.resource.handlers._devcontainer_internals import ( + _CONTAINER_ID_PATTERN, + _MAX_TERMINAL_TRACKERS, + DEFAULT_HEALTH_CHECK_INTERVAL, + _health_check_stop_events, + _health_check_threads, + _lifecycle_registry, + _parse_devcontainer_up_output, + _registry_lock, + get_lifecycle_tracker, + list_active_containers, + set_lifecycle_tracker, +) +from cleveragents.resource.handlers.devcontainer_cleanup import ( + evict_terminal_trackers, + list_active_containers_for_session, + stop_all_active_containers, +) +from cleveragents.resource.handlers.devcontainer_health import ( + _health_check_loop, + _single_probe, + _stop_health_check, + start_health_check, +) +from cleveragents.resource.handlers.devcontainer_lifecycle import ( + activate_container, + clear_lifecycle_registry, + rebuild_container, + stop_container, +) +from cleveragents.tool.context import BoundResource + +logger = logging.getLogger(__name__) + +# Re-exported symbols for backward compatibility — keep in sync with +# the sub-modules. These names are consumed by step files, Robot +# helpers, benchmarks, and application services via +# ``from cleveragents.resource.handlers.devcontainer import ``. +__all__ = [ + "DEFAULT_HEALTH_CHECK_INTERVAL", + "_CONTAINER_ID_PATTERN", + "_MAX_TERMINAL_TRACKERS", + "DevcontainerHandler", + "_health_check_loop", + "_health_check_stop_events", + "_health_check_threads", + "_lifecycle_registry", + "_parse_devcontainer_up_output", + "_registry_lock", + "_single_probe", + "_stop_health_check", + "activate_container", + "clear_lifecycle_registry", + "evict_terminal_trackers", + "get_lifecycle_tracker", + "list_active_containers", + "list_active_containers_for_session", + "rebuild_container", + "set_lifecycle_tracker", + "start_health_check", + "stop_all_active_containers", + "stop_container", +] class DevcontainerHandler(BaseResourceHandler): """Handler for ``devcontainer-instance`` and ``devcontainer-file`` types. - Provisions a snapshot sandbox for devcontainer resources. ``devcontainer-instance`` inherits from ``container-instance`` per ADR-042 (resource type inheritance). + + Supports lazy activation: when a tool targets a detected + devcontainer-instance, the handler transitions to ``building`` + and invokes ``devcontainer up`` before resolving the sandbox. + + F22/F25 fix: uses ``none`` sandbox strategy instead of ``snapshot`` + because the container itself provides isolation and + ``SandboxFactory`` has not yet implemented the ``snapshot`` + strategy. ``devcontainer-file`` resources also use ``none`` + (read-only config file). """ - _default_strategy = SandboxStrategy.SNAPSHOT + # F22/F25 fix: SNAPSHOT raises NotImplementedError in SandboxFactory. + # The container IS the sandbox for devcontainer-instance, so NONE is + # semantically correct until a dedicated container-snapshot strategy + # is implemented. + _default_strategy = SandboxStrategy.NONE _type_label = "devcontainer" + + # -- F16 fix: lazy activation on first resolve ------------------------- + + _ACTIVATABLE_STATES = frozenset( + { + ContainerLifecycleState.DETECTED, + ContainerLifecycleState.STOPPED, + ContainerLifecycleState.FAILED, + } + ) + + def resolve( + self, + *, + resource: Resource, + plan_id: str, + slot_name: str, + sandbox_manager: SandboxManager, + access: str = "read_only", + ) -> BoundResource: + """Resolve a devcontainer resource, lazily activating if needed. + + For ``devcontainer-instance`` resources in a non-running state, + this triggers :func:`activate_container` before delegating to the + base-class sandbox resolution. For other resource types (e.g. + ``devcontainer-file``), it delegates directly. + + Args: + resource: The resource domain object to resolve. + plan_id: The plan requesting the sandbox. + slot_name: Name of the tool resource slot being filled. + sandbox_manager: The sandbox lifecycle manager. + access: Access mode (``read_only`` or ``read_write``). + + Returns: + A :class:`BoundResource` with ``sandbox_path`` populated. + """ + if resource.resource_type_name == "devcontainer-instance" and resource.location: + tracker = get_lifecycle_tracker(resource.resource_id) + if tracker.current_state in self._ACTIVATABLE_STATES: + logger.info( + "Lazy-activating devcontainer %s (state=%s)", + resource.resource_id, + tracker.current_state.value, + ) + activate_container( + resource.resource_id, + resource.location, + session_id=plan_id, + ) + + return super().resolve( + resource=resource, + plan_id=plan_id, + slot_name=slot_name, + sandbox_manager=sandbox_manager, + access=access, + ) diff --git a/src/cleveragents/resource/handlers/devcontainer_cleanup.py b/src/cleveragents/resource/handlers/devcontainer_cleanup.py new file mode 100644 index 000000000..8853d5b24 --- /dev/null +++ b/src/cleveragents/resource/handlers/devcontainer_cleanup.py @@ -0,0 +1,149 @@ +"""Devcontainer session cleanup and registry eviction. + +Provides session-scoped container cleanup and terminal-state tracker +eviction for the devcontainer lifecycle system. + +Extracted from ``devcontainer.py`` to comply with the 500-line file +limit (F28 fix). The ``devcontainer`` module re-exports all public +symbols from this module for backward compatibility. + +Based on: + - docs/specification.md -- Devcontainer, Resource Types + - Issue #514: Devcontainer lifecycle management +""" + +from __future__ import annotations + +import logging +from typing import Any + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, +) +from cleveragents.resource.handlers._devcontainer_internals import ( + _MAX_TERMINAL_TRACKERS, + _lifecycle_registry, + _registry_lock, + list_active_containers, +) +from cleveragents.resource.handlers.devcontainer_lifecycle import ( + stop_container, +) + +logger = logging.getLogger(__name__) + + +def list_active_containers_for_session(session_id: str) -> list[str]: + """Return resource IDs of running containers owned by *session_id*. + + When *session_id* is empty, behaves identically to + :func:`list_active_containers` (returns all running containers). + + Containers that have no ``session_id`` set (None) are considered + "unscoped" and match any session cleanup — this ensures containers + activated before session-scoping was added still get cleaned up. + + Args: + session_id: Session or plan ID to filter by. + + Returns: + List of resource_id strings. + """ + if not session_id: + return list_active_containers() + with _registry_lock: + result: list[str] = [] + for rid, tracker in _lifecycle_registry.items(): + if tracker.current_state != ContainerLifecycleState.RUNNING: + continue + if tracker.session_id == session_id: + result.append(rid) + elif tracker.session_id is None: + # BUG-3 fix: warn when an unscoped container is included + # in session-scoped cleanup — the first session to close + # will stop containers that may still be used by others. + logger.warning( + "Including unscoped container %s in session '%s' cleanup " + "(container has no session_id set)", + rid, + session_id, + ) + result.append(rid) + return result + + +def stop_all_active_containers( + *, + run_command: Any = None, + session_id: str = "", +) -> list[str]: + """Stop active devcontainer instances, optionally scoped to a session. + + When *session_id* is provided, only containers associated with that + session are stopped. When empty, all running containers are stopped + (backwards-compatible behavior). + + Used as a cleanup hook when sessions close or plans complete. + + Args: + run_command: Optional callable for testing. + session_id: Optional session/plan ID to scope cleanup. + + Returns: + List of resource IDs that were stopped. + """ + # R7-F1 fix: scope cleanup to session_id when provided. + active_ids = list_active_containers_for_session(session_id) + stopped: list[str] = [] + for rid in active_ids: + try: + stop_container(rid, run_command=run_command) + stopped.append(rid) + logger.info( + "Stopped container %s (session=%s)", + rid, + session_id or "n/a", + ) + except (RuntimeError, ValueError) as exc: + logger.warning("Failed to stop container %s: %s", rid, exc) + + # R8-F1 fix: evict oldest terminal-state trackers when the + # registry exceeds the cap. + evict_terminal_trackers() + + return stopped + + +def evict_terminal_trackers() -> int: + """Remove stopped/failed trackers when the registry exceeds the cap. + + Keeps at most :data:`_MAX_TERMINAL_TRACKERS` terminal-state entries. + Evicts oldest (by last transition timestamp) first. + + Returns: + Number of trackers evicted. + """ + terminal_states = frozenset( + {ContainerLifecycleState.STOPPED, ContainerLifecycleState.FAILED} + ) + with _registry_lock: + terminal_ids = [ + rid + for rid, t in _lifecycle_registry.items() + if t.current_state in terminal_states + ] + if len(terminal_ids) <= _MAX_TERMINAL_TRACKERS: + return 0 + + # Sort by last transition timestamp (oldest first) + def _last_ts(rid: str) -> float: + t = _lifecycle_registry[rid] + if t.transitions: + return t.transitions[-1].timestamp.timestamp() + return 0.0 + + terminal_ids.sort(key=_last_ts) + to_evict = terminal_ids[: len(terminal_ids) - _MAX_TERMINAL_TRACKERS] + for rid in to_evict: + del _lifecycle_registry[rid] + return len(to_evict) diff --git a/src/cleveragents/resource/handlers/devcontainer_health.py b/src/cleveragents/resource/handlers/devcontainer_health.py new file mode 100644 index 000000000..f86642bfe --- /dev/null +++ b/src/cleveragents/resource/handlers/devcontainer_health.py @@ -0,0 +1,206 @@ +"""Devcontainer health checking for CleverAgents. + +Background health-check threads that periodically probe running +devcontainers via ``devcontainer exec echo ping``. When a probe +fails the container is transitioned to the ``failed`` state. + +Extracted from ``devcontainer.py`` to comply with the 500-line file +limit (F28 fix). + +Based on: + - docs/specification.md -- Devcontainer, Resource Types + - Issue #514: Devcontainer lifecycle management +""" + +from __future__ import annotations + +import logging +import subprocess +import threading +from typing import Any + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, +) +from cleveragents.resource.handlers._devcontainer_internals import ( + _health_check_stop_events, + _health_check_threads, + _registry_lock, + get_lifecycle_tracker, + set_lifecycle_tracker, +) + +logger = logging.getLogger(__name__) + + +def start_health_check( + resource_id: str, + *, + interval: float = 30.0, + run_command: Any = None, +) -> None: + """Start a background health check thread for an active container. + + Args: + resource_id: ULID of the resource. + interval: Seconds between probes. + run_command: Optional callable for testing. + """ + if not resource_id: + raise ValueError("resource_id must not be empty") + + _stop_health_check(resource_id) + + stop_event = threading.Event() + thread = threading.Thread( + target=_health_check_loop, + args=(resource_id, interval, stop_event, run_command), + daemon=True, + name=f"health-check-{resource_id[:8]}", + ) + + # R2 fix: protect dict writes with _registry_lock + with _registry_lock: + _health_check_stop_events[resource_id] = stop_event + _health_check_threads[resource_id] = thread + + thread.start() + + +def _stop_health_check(resource_id: str) -> None: + """Signal and remove a health check thread. + + Signals the stop event, then joins the thread with a 2 s timeout + to avoid leaving dangling background work (R1 fix). + """ + # R2 fix: protect dict access with _registry_lock + with _registry_lock: + stop_event = _health_check_stop_events.pop(resource_id, None) + thread = _health_check_threads.pop(resource_id, None) + if stop_event is not None: + stop_event.set() + # R1 fix: join thread to avoid racing with stop_container + if thread is not None and thread is not threading.current_thread(): + thread.join(timeout=2.0) + + +def _single_probe( + tracker: ContainerLifecycleTracker, + runner: Any, +) -> bool: + """Execute a single health check probe against a container. + + Args: + tracker: Current lifecycle tracker (must be in ``running`` state). + runner: Callable matching ``subprocess.run`` signature. + + Returns: + ``True`` if the probe succeeded (returncode == 0), ``False`` otherwise. + + Raises: + Exception: Re-raised from the subprocess call on unexpected errors. + """ + # F15 fix: use host_workspace_path for devcontainer exec + # (--workspace-folder expects the host-side path, not the + # remote/container-side workspace_path). + # SEC-2 fix: warn when the path is None and the fallback is used. + probe_workspace = tracker.host_workspace_path + if probe_workspace is None: + logger.warning( + "Health probe for %s: host_workspace_path is None, " + "falling back to /workspace — this may target the wrong container", + tracker.resource_id, + ) + probe_workspace = "/workspace" + result = runner( + [ + "devcontainer", + "exec", + "--workspace-folder", + probe_workspace, + "echo", + "ping", + ], + capture_output=True, + text=True, + timeout=10, + check=False, + ) + return result.returncode == 0 + + +def _health_check_loop( + resource_id: str, + interval: float, + stop_event: threading.Event, + run_command: Any, +) -> None: + """Run periodic health probes until stopped.""" + runner = run_command or subprocess.run + while not stop_event.is_set(): + stop_event.wait(interval) + if stop_event.is_set(): + break + tracker = get_lifecycle_tracker(resource_id) + if tracker.current_state != ContainerLifecycleState.RUNNING: + break + try: + healthy = _single_probe(tracker, runner) + if not healthy: + # R11 fix: hold lock across re-read + transition to + # close TOCTOU gap with concurrent stop_container. + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + if tracker.current_state != ContainerLifecycleState.RUNNING: + break + try: + tracker = transition_state( + tracker, + ContainerLifecycleState.STOPPING, + reason="health check failed", + ) + set_lifecycle_tracker(tracker) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason="health check failed", + ) + set_lifecycle_tracker(tracker) + except ValueError: + # R19 fix: log swallowed ValueError for observability + logger.debug( + "Health check transition skipped for %s: " + "state already changed by concurrent operation", + resource_id, + ) + break + except Exception as exc: + logger.warning("Health check error for %s: %s", resource_id, exc) + # R11 fix: hold lock across re-read + transition + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + if tracker.current_state != ContainerLifecycleState.RUNNING: + break + try: + tracker = transition_state( + tracker, + ContainerLifecycleState.STOPPING, + reason=f"health check error: {exc}", + ) + set_lifecycle_tracker(tracker) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"health check error: {exc}", + ) + set_lifecycle_tracker(tracker) + except ValueError: + # R19 fix: log swallowed ValueError for observability + logger.debug( + "Health check transition skipped for %s: " + "state already changed by concurrent operation", + resource_id, + ) + break diff --git a/src/cleveragents/resource/handlers/devcontainer_lifecycle.py b/src/cleveragents/resource/handlers/devcontainer_lifecycle.py new file mode 100644 index 000000000..00694005b --- /dev/null +++ b/src/cleveragents/resource/handlers/devcontainer_lifecycle.py @@ -0,0 +1,439 @@ +"""Devcontainer lifecycle operations for CleverAgents. + +Core lifecycle transitions for ``devcontainer-instance`` resources: +activation (``devcontainer up``), stop (``docker stop``), and rebuild +(``devcontainer up --reset-container``). + +Extracted from ``devcontainer.py`` to comply with the 500-line file +limit (F28 fix). The ``devcontainer`` module re-exports all public +symbols from this module for backward compatibility. + +Based on: + - docs/specification.md -- Devcontainer, Resource Types + - ADR-043: Devcontainer Integration + - Issue #514: Devcontainer lifecycle management +""" + +from __future__ import annotations + +import logging +import os +import subprocess +from typing import Any + +from cleveragents.domain.models.core.container_lifecycle import ( + ContainerLifecycleState, + ContainerLifecycleTracker, + transition_state, +) +from cleveragents.resource.handlers._devcontainer_internals import ( + _health_check_stop_events, + _health_check_threads, + _lifecycle_registry, + _parse_devcontainer_up_output, + _registry_lock, + get_lifecycle_tracker, + set_lifecycle_tracker, +) +from cleveragents.resource.handlers.devcontainer_health import ( + _stop_health_check, + start_health_check, +) + +logger = logging.getLogger(__name__) + + +# ── Registry management ────────────────────────────────────────── + + +def clear_lifecycle_registry() -> None: + """Clear all lifecycle trackers and stop health checks. + + Signals all health check threads to stop, waits for them to + finish (up to 2 s each), then clears the registry. + + Intended for testing and cleanup. + """ + with _registry_lock: + # Signal all health check threads to stop + for resource_id in list(_health_check_stop_events): + _health_check_stop_events[resource_id].set() + # Join threads with a timeout to avoid dangling background work + threads_to_join = list(_health_check_threads.values()) + _health_check_threads.clear() + _health_check_stop_events.clear() + _lifecycle_registry.clear() + # Join outside the lock to avoid blocking registry operations + for thread in threads_to_join: + thread.join(timeout=2.0) + + +# ── Activation ─────────────────────────────────────────────────── + + +def activate_container( + resource_id: str, + workspace_folder: str, + *, + run_command: Any = None, + session_id: str | None = None, + rebuild: bool = False, +) -> ContainerLifecycleTracker: + """Lazily activate a devcontainer by running ``devcontainer up``. + + Transitions: ``detected`` / ``stopped`` / ``failed`` → ``building`` → ``running``. + + Args: + resource_id: ULID of the devcontainer resource. + workspace_folder: Path to the workspace containing devcontainer config. + run_command: Optional callable replacing subprocess.run for testing. + session_id: Optional session/plan ID to associate with this container + for scoped cleanup. + rebuild: When ``True``, pass ``--reset-container`` to + ``devcontainer up`` to force container recreation (F10 fix). + + Returns: + Updated lifecycle tracker in ``running`` state. + + Raises: + ValueError: If the resource is already running or in an + invalid state for activation. + RuntimeError: If ``devcontainer up`` fails or container_id + cannot be extracted from the output. + """ + if not resource_id: + raise ValueError("resource_id must not be empty") + if not workspace_folder: + raise ValueError("workspace_folder must not be empty") + if not os.path.isabs(workspace_folder): + raise ValueError("workspace_folder must be an absolute path") + # SEC-1 fix: canonicalize to resolve symlinks and '..' components, + # preventing path-traversal via inputs like /tmp/../etc/devcontainer.json. + workspace_folder = os.path.realpath(workspace_folder) + + # R3 fix: hold lock across get→transition→set to prevent + # duplicate activations from concurrent callers. + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + # R7-F1 fix: store session_id on tracker for scoped cleanup. + # R8-F4 fix: use model_copy to avoid fragile manual reconstruction. + if session_id and not tracker.session_id: + tracker = tracker.model_copy(update={"session_id": session_id}) + set_lifecycle_tracker(tracker) + tracker = transition_state( + tracker, + ContainerLifecycleState.BUILDING, + reason="lazy activation", + ) + set_lifecycle_tracker(tracker) + + runner = run_command or subprocess.run + try: + # F10 fix: when rebuild=True, pass --reset-container to force + # container recreation, differentiating rebuild from re-activate. + cmd = [ + "devcontainer", + "up", + "--workspace-folder", + workspace_folder, + ] + if rebuild: + cmd.append("--reset-container") + result = runner( + cmd, + capture_output=True, + text=True, + timeout=300, + check=False, + ) + + if result.returncode != 0: + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"devcontainer up failed (rc={result.returncode})", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError( + f"devcontainer up failed (rc={result.returncode}): {result.stderr}" + ) + + # Parse JSON output + container_id, ws_path = _parse_devcontainer_up_output(result.stdout) + + # Finding 10 fix: treat missing container_id as activation failure + if not container_id: + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason="devcontainer up returned no container ID", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError( + "devcontainer up succeeded but output contained no containerId" + ) + + # R7-F6 fix: re-read tracker from registry to avoid stale reference. + # R8-F4 fix: use model_copy to avoid fragile manual reconstruction. + # F15 fix: store both the remote workspace path (container-side) + # and the host workspace folder. + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = tracker.model_copy( + update={ + "container_id": container_id, + "workspace_path": ws_path, + "host_workspace_path": workspace_folder, + }, + ) + tracker = transition_state( + tracker, + ContainerLifecycleState.RUNNING, + reason="devcontainer up succeeded", + ) + set_lifecycle_tracker(tracker) + + # R7-F3 fix: start health checking now that container is running. + start_health_check(resource_id, run_command=run_command) + + except RuntimeError: + raise + except ValueError: + raise + except subprocess.TimeoutExpired as exc: + _handle_activation_timeout(resource_id, workspace_folder, run_command, exc) + except Exception as exc: + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"activation error: {exc}", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError(f"Failed to activate container: {exc}") from exc + + return tracker + + +def _handle_activation_timeout( + resource_id: str, + workspace_folder: str, + run_command: Any, + exc: subprocess.TimeoutExpired, +) -> None: + """Handle activation timeout by cleaning up orphaned containers. + + Extracted from :func:`activate_container` to reduce method length. + + Args: + resource_id: ULID of the devcontainer resource. + workspace_folder: Canonicalized workspace folder path. + run_command: Optional callable replacing subprocess.run. + exc: The original TimeoutExpired exception. + + Raises: + RuntimeError: Always raised after cleanup attempt. + """ + # S2 fix: attempt to clean up orphaned containers on timeout + logger.warning( + "devcontainer up timed out for %s — attempting cleanup", + resource_id, + ) + cleanup_runner = run_command or subprocess.run + try: + ps_result = cleanup_runner( + [ + "docker", + "ps", + "-q", + "--filter", + f"label=devcontainer.local_folder={workspace_folder}", + ], + capture_output=True, + text=True, + timeout=10, + check=False, + ) + orphan_ids = [ + cid.strip() + for cid in getattr(ps_result, "stdout", "").splitlines() + if cid.strip() + ] + for cid in orphan_ids: + try: + cleanup_runner( + ["docker", "stop", cid], + capture_output=True, + text=True, + timeout=30, + check=False, + ) + logger.info("Stopped orphaned container %s for %s", cid, resource_id) + except Exception: + logger.debug("Failed to stop orphaned container %s", cid) + except Exception: + logger.debug("Orphaned container cleanup lookup failed for %s", resource_id) + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"activation timed out after {exc.timeout}s", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError(f"devcontainer up timed out after {exc.timeout}s") from exc + + +# ── Stop ───────────────────────────────────────────────────────── + + +def stop_container( + resource_id: str, + *, + run_command: Any = None, +) -> ContainerLifecycleTracker: + """Stop a running devcontainer. + + Transitions: ``running`` → ``stopping`` → ``stopped``. + + Args: + resource_id: ULID of the devcontainer resource. + run_command: Optional callable replacing subprocess.run for testing. + + Returns: + Updated tracker in ``stopped`` state. + + Raises: + ValueError: If the container is not in ``running`` state. + RuntimeError: If stop fails. + """ + if not resource_id: + raise ValueError("resource_id must not be empty") + + # Stop health check BEFORE acquiring the lock to avoid deadlock: + # the health check thread may be waiting on _registry_lock inside + # get_lifecycle_tracker, and joining it while holding the lock + # would deadlock. + _stop_health_check(resource_id) + + # R3 fix: hold lock across get→transition→set to prevent + # concurrent stop calls from racing. + # R8-F2 fix: if the health check thread already transitioned + # the container to a terminal state, return early. + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + if tracker.current_state in ( + ContainerLifecycleState.STOPPED, + ContainerLifecycleState.FAILED, + ): + logger.info( + "Container %s already in terminal state '%s'; skipping stop.", + resource_id, + tracker.current_state.value, + ) + return tracker + tracker = transition_state( + tracker, + ContainerLifecycleState.STOPPING, + reason="manual stop", + ) + set_lifecycle_tracker(tracker) + + runner = run_command or subprocess.run + try: + container_id = tracker.container_id + if container_id: + result = runner( + ["docker", "stop", container_id], + capture_output=True, + text=True, + timeout=60, + check=False, + ) + if result.returncode != 0: + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"docker stop failed (rc={result.returncode})", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError( + f"docker stop failed (rc={result.returncode}): " + f"{getattr(result, 'stderr', '')}" + ) + + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.STOPPED, + reason="container stopped", + ) + set_lifecycle_tracker(tracker) + + except RuntimeError: + raise + except Exception as exc: + with _registry_lock: + tracker = get_lifecycle_tracker(resource_id) + tracker = transition_state( + tracker, + ContainerLifecycleState.FAILED, + reason=f"stop failed: {exc}", + ) + set_lifecycle_tracker(tracker) + raise RuntimeError(f"Failed to stop container: {exc}") from exc + + return tracker + + +# ── Rebuild ────────────────────────────────────────────────────── + + +def rebuild_container( + resource_id: str, + workspace_folder: str, + *, + run_command: Any = None, + session_id: str | None = None, +) -> ContainerLifecycleTracker: + """Rebuild a stopped or failed devcontainer. + + Transitions: ``stopped``/``failed`` → ``building`` → ``running``. + + Unlike :func:`activate_container`, rebuild passes + ``--reset-container`` to ``devcontainer up`` to force container + recreation (F10 fix). + + Args: + resource_id: ULID of the resource. + workspace_folder: Path to workspace with devcontainer config. + run_command: Optional callable for testing. + session_id: Optional session/plan ID to associate with the + rebuilt container (BUG-5 fix: forwarded to activate_container). + + Returns: + Updated tracker in ``running`` state. + """ + if not resource_id: + raise ValueError("resource_id must not be empty") + if not workspace_folder: + raise ValueError("workspace_folder must not be empty") + + # BUG-5 fix: forward session_id so the rebuilt container is + # associated with the calling session for scoped cleanup. + # F10 fix: pass rebuild=True to add --reset-container flag. + return activate_container( + resource_id, + workspace_folder, + run_command=run_command, + session_id=session_id, + rebuild=True, + ) diff --git a/vulture_whitelist.py b/vulture_whitelist.py index a3fcb3f42..6b1689ce4 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -518,6 +518,46 @@ compressed_tokens # noqa: B018, F821 original_tokens # noqa: B018, F821 fragment_id # noqa: B018, F821 +# Container lifecycle management — public API (#514) +ContainerLifecycleState # noqa: B018, F821 +ContainerLifecycleTracker # noqa: B018, F821 +LifecycleTransition # noqa: B018, F821 +validate_transition # noqa: B018, F821 +transition_state # noqa: B018, F821 +VALID_TRANSITIONS # noqa: B018, F821 +DEFAULT_HEALTH_CHECK_INTERVAL # noqa: B018, F821 +get_lifecycle_tracker # noqa: B018, F821 +set_lifecycle_tracker # noqa: B018, F821 +clear_lifecycle_registry # noqa: B018, F821 +list_active_containers # noqa: B018, F821 +activate_container # noqa: B018, F821 +stop_container # noqa: B018, F821 +rebuild_container # noqa: B018, F821 +start_health_check # noqa: B018, F821 +stop_all_active_containers # noqa: B018, F821 +# CODE-1 note: the private symbols below are NOT dead code — they are +# imported directly by BDD step definitions in features/steps/ for +# white-box testing of lifecycle internals. Vulture flags them because +# its scan scope does not include the features/ tree. +_parse_devcontainer_up_output # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_stop_health_check # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_health_check_loop # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_single_probe # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_lifecycle_registry # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_registry_lock # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_health_check_threads # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +_health_check_stop_events # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +resource_stop # noqa: B018, F821 +resource_rebuild # noqa: B018, F821 +_STOPPABLE_TYPES # noqa: B018, F821 — used in devcontainer_handler_steps.py +_REBUILDABLE_TYPES # noqa: B018, F821 — used in devcontainer_handler_steps.py +stop_active_devcontainers # noqa: B018, F821 +list_active_containers_for_session # noqa: B018, F821 +evict_terminal_trackers # noqa: B018, F821 +_MAX_TERMINAL_TRACKERS # noqa: B018, F821 — used in devcontainer_lifecycle_steps.py +host_workspace_path # noqa: B018, F821 — F15 fix: new tracker field for host-side workspace path +_ACTIVATABLE_STATES # noqa: B018, F821 — F16 fix: class attribute on DevcontainerHandler + # Execution environment routing — public API (#512) ExecutionEnvironment # noqa: B018, F821 ExecutionEnvironmentResolver # noqa: B018, F821 @@ -575,6 +615,7 @@ _plan_lifecycle_service # noqa: B018, F821 _tool_registry # noqa: B018, F821 _resource_registry_service # noqa: B018, F821 _event_queue # noqa: B018, F821 +_cleanup_session_devcontainers # noqa: B018, F821 # Server client stubs — public API for server mode (#201) ServerClient # noqa: B018, F821