From cf67ba0a86739e774055c4e50099b52b2d630562 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Wed, 4 Mar 2026 01:30:54 +0000 Subject: [PATCH] feat(devcontainer): add lazy activation and lifecycle management Implemented lazy container activation for devcontainer-instance resources with ContainerLifecycleState enum tracking six states (inactive, starting, active, stopping, stopped, error) with validated transitions. Extended DevcontainerHandler with devcontainer up CLI integration and JSON output parsing for container start. Added periodic health checking via devcontainer exec ping with configurable interval. Added agents resource stop and agents resource rebuild CLI commands for manual lifecycle control. Wired session close and plan completion hooks to automatic container cleanup. Includes lifecycle state persistence in resource registry with timestamped transitions. Added Behave BDD tests, Robot integration tests, and ASV activation latency benchmarks. - Added remoteWorkspaceFolder absolute-path validation - Aligned spec: handler name, rebuild types, --yes flag on stop/rebuild - Added registry re-read in stop_container success path for consistency - Added session_id field to ContainerLifecycleTracker for scoped cleanup - Scoped stop_all_active_containers to session_id when provided - Wired _cleanup_devcontainers into fail_apply and fail_execute - Wired start_health_check into activate_container success path - Restructured facade session close to always run container cleanup even without session service (F4) - Re-read tracker from registry in activate_container success path - Added evict_terminal_trackers to cap registry growth - Updated devcontainer_resources.md: health check auto-start, scoped cleanup hooks, known limitations for eviction and sandbox_strategy - Wired evict_terminal_trackers into stop_all_active_containers so terminal-state trackers are actually evicted in production - Made stop_container idempotent: returns early when container is already in a terminal state instead of raising ValueError - Fixed benchmark health check thread leak in TimeActivationLatency by clearing registry after each timing loop - Added rebuild pass-through (--reset-container flag to devcontainer up) - Added host_workspace_path field on ContainerLifecycleTracker so health probes use the host-side path for devcontainer exec - Wired lazy activation into DevcontainerHandler.resolve() for devcontainer-instance resources in non-running states - Changed _default_strategy from SNAPSHOT to NONE (container itself provides isolation; SandboxFactory raises NotImplementedError for snapshot) - Restricted _STOPPABLE_TYPES to devcontainer-instance only (container-instance is not directly stoppable via CLI) ISSUES CLOSED: #514 --- CHANGELOG.md | 8 + benchmarks/devcontainer_lifecycle_bench.py | 219 ++++++++ docs/reference/devcontainer_resources.md | 168 +++++- docs/specification.md | 94 +++- features/devcontainer_activation.feature | 217 ++++++++ features/devcontainer_cleanup.feature | 237 +++++++++ features/devcontainer_handler.feature | 2 +- features/devcontainer_health_check.feature | 126 +++++ features/devcontainer_state.feature | 198 +++++++ features/environment.py | 12 + features/mocks/mock_devcontainer_cli.py | 209 ++++++++ .../steps/devcontainer_activation_steps.py | 494 ++++++++++++++++++ features/steps/devcontainer_cleanup_steps.py | 465 +++++++++++++++++ .../steps/devcontainer_health_check_steps.py | 409 +++++++++++++++ .../steps/devcontainer_lifecycle_steps.py | 464 ++++++++++++++++ robot/cleanup.robot | 16 +- robot/devcontainer_lifecycle.robot | 89 ++++ robot/helper_devcontainer_handler.py | 9 +- robot/helper_devcontainer_lifecycle.py | 275 ++++++++++ src/cleveragents/acp/facade.py | 79 ++- .../application/services/cleanup_models.py | 91 ++++ .../application/services/cleanup_service.py | 104 +--- .../services/plan_lifecycle_service.py | 42 ++ src/cleveragents/cli/commands/resource.py | 175 ++++++- .../domain/models/core/container_lifecycle.py | 286 ++++++++++ .../handlers/_devcontainer_internals.py | 152 ++++++ .../resource/handlers/devcontainer.py | 185 ++++++- .../resource/handlers/devcontainer_cleanup.py | 149 ++++++ .../resource/handlers/devcontainer_health.py | 206 ++++++++ .../handlers/devcontainer_lifecycle.py | 439 ++++++++++++++++ vulture_whitelist.py | 41 ++ 31 files changed, 5527 insertions(+), 133 deletions(-) create mode 100644 benchmarks/devcontainer_lifecycle_bench.py create mode 100644 features/devcontainer_activation.feature create mode 100644 features/devcontainer_cleanup.feature create mode 100644 features/devcontainer_health_check.feature create mode 100644 features/devcontainer_state.feature create mode 100644 features/mocks/mock_devcontainer_cli.py create mode 100644 features/steps/devcontainer_activation_steps.py create mode 100644 features/steps/devcontainer_cleanup_steps.py create mode 100644 features/steps/devcontainer_health_check_steps.py create mode 100644 features/steps/devcontainer_lifecycle_steps.py create mode 100644 robot/devcontainer_lifecycle.robot create mode 100644 robot/helper_devcontainer_lifecycle.py create mode 100644 src/cleveragents/application/services/cleanup_models.py create mode 100644 src/cleveragents/domain/models/core/container_lifecycle.py create mode 100644 src/cleveragents/resource/handlers/_devcontainer_internals.py create mode 100644 src/cleveragents/resource/handlers/devcontainer_cleanup.py create mode 100644 src/cleveragents/resource/handlers/devcontainer_health.py create mode 100644 src/cleveragents/resource/handlers/devcontainer_lifecycle.py 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