cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

feat(server): container/devcontainer support lifecycle #1121

Open
freemo wants to merge 3 commits from feature/m9-container-lifecycle into master
pull from: feature/m9-container-lifecycle
merge into: cleveragents:master
Conversation 14 Commits 3 Files changed 23 +4583 -7
freemo commented 2026-03-23 04:20:49 +00:00
Owner
Copy link
No description provided.
freemo added this to the v3.8.0 milestone 2026-03-23 04:20:49 +00:00
feat(server): ASGI endpoint via uvicorn
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 20s
Details
CI / security (pull_request) Failing after 51s
Details
CI / lint (pull_request) Successful in 3m39s
Details
CI / quality (pull_request) Successful in 3m46s
Details
CI / typecheck (pull_request) Successful in 3m56s
Details
CI / e2e_tests (pull_request) Successful in 8m39s
Details
CI / coverage (pull_request) Successful in 11m6s
Details
CI / integration_tests (pull_request) Failing after 16m23s
Details
CI / unit_tests (pull_request) Failing after 16m23s
Details
CI / benchmark-regression (pull_request) Successful in 52m29s
Details
CI / docker (pull_request) Has been skipped
Details
CI / status-check (pull_request) Failing after 1s
Details
5f0a545113 
Implement FastAPI-based ASGI application served by uvicorn for
the CleverAgents server mode. Add health check endpoint, A2A
JSON-RPC routing, configurable host:port binding, and graceful
shutdown handling. Server launches via `agents server start`.

- Add FastAPI ASGI app factory at infrastructure/server/asgi_app.py
  with /.well-known/agent.json (Agent Card), /health (liveness),
  and /a2a (A2A JSON-RPC 2.0 dispatch via A2aLocalFacade)
- Add ServerLifecycle class at infrastructure/server/server_lifecycle.py
  wrapping uvicorn.Server with SIGTERM/SIGINT graceful shutdown
- Add `agents server start` CLI command with --host, --port, --log-level
  options, resolving defaults from Settings
- Add fastapi>=0.115.0 dependency to pyproject.toml (uvicorn already present)
- Add Behave BDD tests (features/server_lifecycle.feature, 20 scenarios)
- Add Robot Framework integration tests (robot/server_lifecycle.robot)
- Update CHANGELOG.md and vulture_whitelist.py

ISSUES CLOSED: #862
feat(server): container/devcontainer support lifecycle
Some checks failed
CI / build (pull_request) Successful in 17s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 3m20s
Details
CI / quality (pull_request) Successful in 3m46s
Details
CI / typecheck (pull_request) Successful in 3m55s
Details
CI / unit_tests (pull_request) Failing after 3m59s
Details
CI / security (pull_request) Successful in 4m20s
Details
CI / docker (pull_request) Has been skipped
Details
CI / integration_tests (pull_request) Successful in 6m57s
Details
CI / e2e_tests (pull_request) Successful in 8m36s
Details
CI / coverage (pull_request) Successful in 11m9s
Details
CI / status-check (pull_request) Failing after 1s
Details
CI / benchmark-regression (pull_request) Successful in 51m10s
Details
8791fd999f 
Implement server-side container and devcontainer lifecycle management
for the CleverAgents server infrastructure.

New modules in src/cleveragents/infrastructure/server/containers/:
- DevcontainerSpec: Pydantic model for devcontainer.json parsing and
  validation (image, build, features, mounts, env vars, ports,
  hostRequirements, postCreateCommand, remoteUser, workspaceFolder).
- ResourceLimits: CPU, memory, storage, and PID limits model with
  Docker CLI argument generation and hostRequirements conversion.
- ContainerManager: Server-side lifecycle orchestration with
  create/start/stop/destroy operations using pluggable Docker CLI
  runner (mocked for tests).
- HealthMonitor: Periodic container health probing via docker
  inspect with background thread management and probe history.
- ContainerStatus enum: pending/created/running/stopped/destroyed/error
  with validated state transitions.

Tests:
- Behave BDD: 67 scenarios covering spec parsing, resource limits,
  lifecycle operations, health monitoring, and error handling
  (features/container_lifecycle.feature).
- Robot Framework: 13 integration test cases with mock Docker
  runner (robot/container_lifecycle_server.robot).

ISSUES CLOSED: #865
fix(test): use re step matcher for empty-string host validation scenarios
Some checks failed
CI / build (pull_request) Successful in 18s
Details
CI / lint (pull_request) Successful in 3m35s
Details
CI / quality (pull_request) Successful in 3m43s
Details
CI / security (pull_request) Successful in 4m4s
Details
CI / typecheck (pull_request) Successful in 4m5s
Details
CI / unit_tests (pull_request) Successful in 6m45s
Details
CI / integration_tests (pull_request) Successful in 7m0s
Details
CI / docker (pull_request) Successful in 1m17s
Details
CI / e2e_tests (pull_request) Successful in 9m42s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / coverage (pull_request) Failing after 21m27s
Details
CI / benchmark-regression (pull_request) Successful in 1h5m37s
Details
CI / status-check (pull_request) Failing after 2s
Details
892c3c5969 
The parse step matcher cannot match empty strings inside {host}.
Switch to re matcher for the two steps that test empty host rejection
(create_asgi_app and ServerLifecycle), then reset to parse.
freemo force-pushed feature/m9-container-lifecycle from 892c3c5969
Some checks failed
CI / build (pull_request) Successful in 18s
Details
CI / lint (pull_request) Successful in 3m35s
Details
CI / quality (pull_request) Successful in 3m43s
Details
CI / security (pull_request) Successful in 4m4s
Details
CI / typecheck (pull_request) Successful in 4m5s
Details
CI / unit_tests (pull_request) Successful in 6m45s
Details
CI / integration_tests (pull_request) Successful in 7m0s
Details
CI / docker (pull_request) Successful in 1m17s
Details
CI / e2e_tests (pull_request) Successful in 9m42s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / coverage (pull_request) Failing after 21m27s
Details
CI / benchmark-regression (pull_request) Successful in 1h5m37s
Details
CI / status-check (pull_request) Failing after 2s
Details
to c414f4e1a7
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 30s
Details
CI / lint (pull_request) Successful in 5m24s
Details
CI / quality (pull_request) Successful in 5m43s
Details
CI / typecheck (pull_request) Successful in 5m49s
Details
CI / security (pull_request) Successful in 6m23s
Details
CI / integration_tests (pull_request) Successful in 8m56s
Details
CI / unit_tests (pull_request) Successful in 9m9s
Details
CI / docker (pull_request) Successful in 1m38s
Details
CI / e2e_tests (pull_request) Successful in 10m52s
Details
CI / coverage (pull_request) Successful in 11m24s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Failing after 46m48s
Details
2026-03-23 16:48:21 +00:00 Compare
freemo force-pushed feature/m9-container-lifecycle from c414f4e1a7
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 30s
Details
CI / lint (pull_request) Successful in 5m24s
Details
CI / quality (pull_request) Successful in 5m43s
Details
CI / typecheck (pull_request) Successful in 5m49s
Details
CI / security (pull_request) Successful in 6m23s
Details
CI / integration_tests (pull_request) Successful in 8m56s
Details
CI / unit_tests (pull_request) Successful in 9m9s
Details
CI / docker (pull_request) Successful in 1m38s
Details
CI / e2e_tests (pull_request) Successful in 10m52s
Details
CI / coverage (pull_request) Successful in 11m24s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Failing after 46m48s
Details
to 8438857ae7
Some checks are pending
CI / lint (pull_request) Waiting to run
Details
CI / typecheck (pull_request) Waiting to run
Details
CI / security (pull_request) Waiting to run
Details
CI / quality (pull_request) Waiting to run
Details
CI / unit_tests (pull_request) Waiting to run
Details
CI / integration_tests (pull_request) Waiting to run
Details
CI / e2e_tests (pull_request) Waiting to run
Details
CI / coverage (pull_request) Blocked by required conditions
Details
CI / benchmark-regression (pull_request) Blocked by required conditions
Details
CI / build (pull_request) Waiting to run
Details
CI / docker (pull_request) Blocked by required conditions
Details
CI / status-check (pull_request) Blocked by required conditions
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
2026-03-23 22:49:36 +00:00 Compare
freemo force-pushed feature/m9-container-lifecycle from 8438857ae7
Some checks are pending
CI / lint (pull_request) Waiting to run
Details
CI / typecheck (pull_request) Waiting to run
Details
CI / security (pull_request) Waiting to run
Details
CI / quality (pull_request) Waiting to run
Details
CI / unit_tests (pull_request) Waiting to run
Details
CI / integration_tests (pull_request) Waiting to run
Details
CI / e2e_tests (pull_request) Waiting to run
Details
CI / coverage (pull_request) Blocked by required conditions
Details
CI / benchmark-regression (pull_request) Blocked by required conditions
Details
CI / build (pull_request) Waiting to run
Details
CI / docker (pull_request) Blocked by required conditions
Details
CI / status-check (pull_request) Blocked by required conditions
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
to 9c4655fd5c
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 32s
Details
CI / lint (pull_request) Successful in 4m55s
Details
CI / quality (pull_request) Successful in 5m33s
Details
CI / typecheck (pull_request) Successful in 5m45s
Details
CI / security (pull_request) Successful in 5m55s
Details
CI / unit_tests (pull_request) Successful in 7m48s
Details
CI / integration_tests (pull_request) Successful in 7m51s
Details
CI / docker (pull_request) Successful in 1m2s
Details
CI / e2e_tests (pull_request) Successful in 10m18s
Details
CI / coverage (pull_request) Successful in 11m33s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Successful in 1h1m0s
Details
2026-03-23 22:50:52 +00:00 Compare
freemo reviewed 2026-03-24 15:29:32 +00:00
freemo left a comment
Copy link

Review: Looks Good

Container/devcontainer support lifecycle for server mode. Feature PR for v3.8.0 milestone.

Note: Cannot formally approve as PR author matches the authenticated API user.

## Review: Looks Good Container/devcontainer support lifecycle for server mode. Feature PR for v3.8.0 milestone. *Note: Cannot formally approve as PR author matches the authenticated API user.*
freemo commented 2026-03-27 17:12:26 +00:00
Author
Owner
Copy link

Code Review: feat(server): container/devcontainer support lifecycle

Good implementation. A couple of notes:

Notes

1. Merge dependency
This PR carries ~1,100 lines of shared ASGI infrastructure from PR #1107. PR #1107 should merge first, then this branch should be rebased to reduce the diff to only the container-specific changes.

2. Large feature file
container_lifecycle.feature has 57 scenarios. Consider splitting into separate feature files per concern (spec parsing, resource limits, container manager, health monitor) for readability.

What's Good

  • Clean state machine: PENDING → CREATED → RUNNING → STOPPED → DESTROYED with ERROR from any active state.
  • All Docker interactions use pluggable run_command callable (DI-friendly, fully mocked in tests).
  • DevcontainerSpec supports multiple mount formats (dict, Docker string, shorthand).
  • ResourceLimits._parse_memory_string() handles gb/mb/plain integer; to_docker_args() generates proper Docker flags.
  • Security context: runAsNonRoot, capabilities.drop: [ALL].
  • 68 BDD scenarios + Robot integration tests — thorough coverage.
## Code Review: feat(server): container/devcontainer support lifecycle **Good implementation.** A couple of notes: ### Notes **1. Merge dependency** This PR carries ~1,100 lines of shared ASGI infrastructure from PR #1107. PR #1107 should merge first, then this branch should be rebased to reduce the diff to only the container-specific changes. **2. Large feature file** `container_lifecycle.feature` has 57 scenarios. Consider splitting into separate feature files per concern (spec parsing, resource limits, container manager, health monitor) for readability. ### What's Good - Clean state machine: `PENDING → CREATED → RUNNING → STOPPED → DESTROYED` with `ERROR` from any active state. - All Docker interactions use pluggable `run_command` callable (DI-friendly, fully mocked in tests). - `DevcontainerSpec` supports multiple mount formats (dict, Docker string, shorthand). - `ResourceLimits._parse_memory_string()` handles gb/mb/plain integer; `to_docker_args()` generates proper Docker flags. - Security context: `runAsNonRoot`, `capabilities.drop: [ALL]`. - 68 BDD scenarios + Robot integration tests — thorough coverage.
freemo self-assigned this 2026-04-02 08:06:41 +00:00
freemo commented 2026-04-02 18:14:31 +00:00
Author
Owner
Copy link

Review claimed by reviewer pool instance pr-reviewer-pool-2813550-1775153400. Dispatching independent code review.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

Review claimed by reviewer pool instance pr-reviewer-pool-2813550-1775153400. Dispatching independent code review. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-02 18:18:49 +00:00
Author
Owner
Copy link

Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle

Overall Assessment

The implementation is well-structured with clean state machine design, good DI patterns (pluggable command runner), and thorough BDD test coverage (67+ Behave scenarios + Robot integration tests). However, there are several hard blockers that must be resolved before this PR can be merged.

🔴 Hard Blockers

1. Merge Conflicts (mergeable: false)

This PR cannot be merged in its current state. The mergeable flag is false, indicating conflicts with master. This PR also carries the ASGI infrastructure commit from PR #1107 (5f0a5451 feat(server): ASGI endpoint via uvicorn), which is itself still open with merge conflicts.

Action required: PR #1107 must merge first, then this branch must be rebased onto master to isolate only the container-specific changes.

2. Prohibited # type: ignore Suppressions

CONTRIBUTING.md strictly prohibits # type: ignore in any form. Found in:

  • features/steps/server_lifecycle_steps.py:13: # type: ignore[import-untyped] on behave import — Note that container_lifecycle_steps.py imports behave without this suppression, proving it's unnecessary.
  • robot/helper_container_lifecycle_server.py: Multiple # type: ignore[union-attr] (lines ~233, 240, 247, 254) — Fix by adding assert or is not None checks before accessing .status.
  • robot/helper_server_lifecycle.py:108: # type: ignore[operator] — Fix by typing _COMMANDS as dict[str, Callable[[], None]] instead of dict[str, object].

3. Files Exceeding 500-Line Limit

  • features/steps/container_lifecycle_steps.py: 929 lines (nearly 2× the limit). Split into separate step files per concern: devcontainer_spec_steps.py, resource_limits_steps.py, container_manager_steps.py, health_monitor_steps.py.
  • src/cleveragents/infrastructure/server/containers/container_manager.py: 524 lines. Extract ManagedContainer model, ContainerStatus enum, and _validate_status_transition/_extract_container_id helpers into separate modules.

4. Empty PR Body

The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., Closes #865) linking to the resolved issue. The commit message has ISSUES CLOSED: #865 but the PR body itself must also contain the closing keyword.

🟡 Significant Issues

5. Weak Typing for run_command Parameters

Both ContainerManager.__init__ and HealthMonitor.__init__ accept run_command: Any. This defeats static type checking. Define a proper type alias or Protocol:

from typing import Protocol
class CommandRunner(Protocol):
    def __call__(self, args: list[str], *, capture_output: bool, text: bool, timeout: int, check: bool) -> subprocess.CompletedProcess[str]: ...

6. Encapsulation Violation in HealthMonitor

health_monitor.py:298 calls self._manager._transition(...), accessing a private method of ContainerManager. This breaks encapsulation. Either:

  • Make _transition a public method (rename to transition), or
  • Add a public mark_error(name, message) method to ContainerManager that HealthMonitor can call.

7. Bug in Coverage Boost Test

server_lifecycle_coverage_boost_steps.py:204:

ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest")

ManagedContainer has no image field. This either silently ignores the kwarg (if Pydantic extra="ignore" is set) or raises a validation error. The image parameter should be removed.

✅ What's Good

  • Clean state machine: PENDING → CREATED → RUNNING → STOPPED → DESTROYED with ERROR from any active state, and validated transitions.
  • Pluggable Docker CLI: All Docker interactions use DI-friendly run_command callable, fully mocked in tests.
  • DevcontainerSpec: Comprehensive parsing supporting multiple mount formats (dict, Docker string, shorthand), camelCase→snake_case mapping, and forward-compatible extra="ignore".
  • ResourceLimits: Handles gb/mb/plain integer parsing, generates proper Docker flags via to_docker_args().
  • Test coverage: 67+ BDD scenarios + 13 Robot integration tests — thorough coverage of happy paths, error paths, and edge cases.
  • Proper argument validation: All public methods validate inputs with descriptive error messages.

Summary

The code quality is high and the architecture is sound, but the PR has process blockers (merge conflicts, empty body, file size violations) and code standard violations (# type: ignore suppressions) that must be resolved. Please:

  1. Wait for PR #1107 to merge, then rebase this branch onto master
  2. Remove all # type: ignore suppressions
  3. Split oversized files to stay under 500 lines
  4. Add a PR body with Closes #865 and a description
  5. Fix the typing and encapsulation issues noted above

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle ### Overall Assessment The implementation is well-structured with clean state machine design, good DI patterns (pluggable command runner), and thorough BDD test coverage (67+ Behave scenarios + Robot integration tests). However, there are several **hard blockers** that must be resolved before this PR can be merged. --- ### 🔴 Hard Blockers #### 1. Merge Conflicts (mergeable: false) This PR cannot be merged in its current state. The `mergeable` flag is `false`, indicating conflicts with `master`. This PR also carries the ASGI infrastructure commit from PR #1107 (`5f0a5451 feat(server): ASGI endpoint via uvicorn`), which is itself still open with merge conflicts. **Action required:** PR #1107 must merge first, then this branch must be rebased onto `master` to isolate only the container-specific changes. #### 2. Prohibited `# type: ignore` Suppressions CONTRIBUTING.md strictly prohibits `# type: ignore` in any form. Found in: - **`features/steps/server_lifecycle_steps.py:13`**: `# type: ignore[import-untyped]` on behave import — Note that `container_lifecycle_steps.py` imports behave without this suppression, proving it's unnecessary. - **`robot/helper_container_lifecycle_server.py`**: Multiple `# type: ignore[union-attr]` (lines ~233, 240, 247, 254) — Fix by adding `assert` or `is not None` checks before accessing `.status`. - **`robot/helper_server_lifecycle.py:108`**: `# type: ignore[operator]` — Fix by typing `_COMMANDS` as `dict[str, Callable[[], None]]` instead of `dict[str, object]`. #### 3. Files Exceeding 500-Line Limit - **`features/steps/container_lifecycle_steps.py`**: **929 lines** (nearly 2× the limit). Split into separate step files per concern: `devcontainer_spec_steps.py`, `resource_limits_steps.py`, `container_manager_steps.py`, `health_monitor_steps.py`. - **`src/cleveragents/infrastructure/server/containers/container_manager.py`**: **524 lines**. Extract `ManagedContainer` model, `ContainerStatus` enum, and `_validate_status_transition`/`_extract_container_id` helpers into separate modules. #### 4. Empty PR Body The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., `Closes #865`) linking to the resolved issue. The commit message has `ISSUES CLOSED: #865` but the PR body itself must also contain the closing keyword. --- ### 🟡 Significant Issues #### 5. Weak Typing for `run_command` Parameters Both `ContainerManager.__init__` and `HealthMonitor.__init__` accept `run_command: Any`. This defeats static type checking. Define a proper type alias or Protocol: ```python from typing import Protocol class CommandRunner(Protocol): def __call__(self, args: list[str], *, capture_output: bool, text: bool, timeout: int, check: bool) -> subprocess.CompletedProcess[str]: ... ``` #### 6. Encapsulation Violation in HealthMonitor `health_monitor.py:298` calls `self._manager._transition(...)`, accessing a private method of `ContainerManager`. This breaks encapsulation. Either: - Make `_transition` a public method (rename to `transition`), or - Add a public `mark_error(name, message)` method to `ContainerManager` that `HealthMonitor` can call. #### 7. Bug in Coverage Boost Test `server_lifecycle_coverage_boost_steps.py:204`: ```python ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest") ``` `ManagedContainer` has no `image` field. This either silently ignores the kwarg (if Pydantic `extra="ignore"` is set) or raises a validation error. The `image` parameter should be removed. --- ### ✅ What's Good - **Clean state machine**: `PENDING → CREATED → RUNNING → STOPPED → DESTROYED` with `ERROR` from any active state, and validated transitions. - **Pluggable Docker CLI**: All Docker interactions use DI-friendly `run_command` callable, fully mocked in tests. - **DevcontainerSpec**: Comprehensive parsing supporting multiple mount formats (dict, Docker string, shorthand), camelCase→snake_case mapping, and forward-compatible `extra="ignore"`. - **ResourceLimits**: Handles gb/mb/plain integer parsing, generates proper Docker flags via `to_docker_args()`. - **Test coverage**: 67+ BDD scenarios + 13 Robot integration tests — thorough coverage of happy paths, error paths, and edge cases. - **Proper argument validation**: All public methods validate inputs with descriptive error messages. --- ### Summary The code quality is high and the architecture is sound, but the PR has process blockers (merge conflicts, empty body, file size violations) and code standard violations (`# type: ignore` suppressions) that must be resolved. Please: 1. Wait for PR #1107 to merge, then rebase this branch onto `master` 2. Remove all `# type: ignore` suppressions 3. Split oversized files to stay under 500 lines 4. Add a PR body with `Closes #865` and a description 5. Fix the typing and encapsulation issues noted above --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
freemo commented 2026-04-02 19:02:56 +00:00
Author
Owner
Copy link

Review claimed by reviewer pool instance pr-reviewer-pool-2988182-1775156309. Dispatching independent code review.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

Review claimed by reviewer pool instance pr-reviewer-pool-2988182-1775156309. Dispatching independent code review. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-02 19:07:34 +00:00
Author
Owner
Copy link

Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle

Independent Review Summary

This is a second independent review of this PR. The implementation demonstrates strong architectural design — the state machine, DI-friendly command runner, and comprehensive BDD coverage are all well-executed. However, several hard blockers and a correctness bug remain unresolved since the prior review.

🔴 Hard Blockers

1. Merge Conflicts (mergeable: false)

The PR cannot be merged. It carries commit 5f0a5451 from PR #1107 (feat(server): ASGI endpoint via uvicorn), which is itself still open with merge conflicts. PR #1107 must merge first, then this branch must be rebased onto master to isolate only the container-specific changes.

2. Prohibited # type: ignore Suppressions (11 instances)

CONTRIBUTING.md strictly prohibits # type: ignore in any form. This PR introduces 11 instances across 4 files:

File Line(s) Suppression Fix
container_lifecycle_steps.py 421, 841, 913, 925 [arg-type] Use typing.cast() or object() instead of string literals for wrong-type tests
server_lifecycle_steps.py 13 [import-untyped] Other step files import behave without this — remove it
server_lifecycle_steps.py 53 [arg-type] Use cast() or pass through Any-typed variable
helper_container_lifecycle_server.py 189, 195, 201, 207 [union-attr] Add assert container is not None before .status access
helper_server_lifecycle.py 113 [operator] Type _COMMANDS as dict[str, Callable[[], None]]

3. Files Exceeding 500-Line Limit

  • features/steps/container_lifecycle_steps.py: 929 lines (nearly 2× the limit). Split into separate step files per concern: devcontainer_spec_steps.py, resource_limits_steps.py, container_manager_steps.py, health_monitor_steps.py.
  • src/cleveragents/infrastructure/server/containers/container_manager.py: 524 lines. Extract ManagedContainer model and ContainerStatus enum into a separate models.py module.

4. Empty PR Body

The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., Closes #865). The commit message has ISSUES CLOSED: #865 but the PR body must also contain the closing keyword and a summary of changes.

🟡 Correctness Bug (NEW — not in prior review)

5. KB Memory Parsing Bug in _parse_memory_string

resource_limits.py line 170:

for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)):

The "kb" multiplier is 1 (same as MB), which is incorrect. This means "512kb" would be parsed as 512 MB instead of approximately 0.5 MB. The correct multiplier for KB→MB conversion should be 1/1024. Since the function returns int, use:

for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1/1024)):

Or handle KB separately with integer division.

🟡 Significant Design Issues

6. Encapsulation Violation in HealthMonitor

health_monitor.py:298 calls self._manager._transition(...), directly accessing a private method of ContainerManager. This breaks encapsulation and creates a fragile coupling. Either:

  • Make _transition public (rename to transition or update_status), or
  • Add a public mark_error(name: str, message: str) -> None method to ContainerManager that HealthMonitor can call.

7. Weak Typing for run_command Parameters

Both ContainerManager.__init__ and HealthMonitor.__init__ accept run_command: Any. This defeats static type checking entirely. Define a proper Protocol:

from typing import Protocol
class CommandRunner(Protocol):
    def __call__(
        self, args: list[str], *, capture_output: bool,
        text: bool, timeout: int, check: bool,
    ) -> subprocess.CompletedProcess[str]: ...

8. Bug in Coverage Boost Test

server_lifecycle_coverage_boost_steps.py:204:

ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest")

ManagedContainer has no image field. Pydantic v2 silently ignores extra fields by default, so this doesn't crash, but it's misleading — the image kwarg does nothing. Remove it.

✅ What's Good

  • Clean state machine: PENDING → CREATED → RUNNING → STOPPED → DESTROYED with ERROR from any active state, validated transitions, and proper error propagation.
  • Pluggable Docker CLI: All Docker interactions use DI-friendly run_command callable, fully mocked in tests — no real Docker needed.
  • DevcontainerSpec: Comprehensive parsing supporting multiple mount formats (dict, Docker string, shorthand), camelCase→snake_case mapping, and forward-compatible extra="ignore".
  • ResourceLimits: Clean Pydantic model with proper validation, coercion, and Docker CLI argument generation.
  • Test coverage: 67+ BDD scenarios + 13 Robot integration tests covering happy paths, error paths, edge cases, and state transitions.
  • Thread safety: ContainerManager and HealthMonitor use threading.RLock for concurrent access protection.
  • Proper argument validation: All public methods validate inputs with descriptive error messages following fail-fast principles.

Action Items

  1. Wait for PR #1107 to merge, then rebase this branch onto master
  2. Remove all 11 # type: ignore suppressions using the fixes described above
  3. Split oversized files to stay under 500 lines
  4. Add a PR body with Closes #865 and a description of changes
  5. Fix the KB parsing bug in _parse_memory_string (new finding)
  6. Fix the encapsulation violation — add a public method to ContainerManager for error transitions
  7. Fix the run_command typing — use a Protocol instead of Any
  8. Remove the image kwarg from the coverage boost test

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle ### Independent Review Summary This is a second independent review of this PR. The implementation demonstrates strong architectural design — the state machine, DI-friendly command runner, and comprehensive BDD coverage are all well-executed. However, several **hard blockers** and a **correctness bug** remain unresolved since the prior review. --- ### 🔴 Hard Blockers #### 1. Merge Conflicts (mergeable: false) The PR cannot be merged. It carries commit `5f0a5451` from PR #1107 (`feat(server): ASGI endpoint via uvicorn`), which is itself still open with merge conflicts. **PR #1107 must merge first**, then this branch must be rebased onto `master` to isolate only the container-specific changes. #### 2. Prohibited `# type: ignore` Suppressions (11 instances) CONTRIBUTING.md strictly prohibits `# type: ignore` in any form. This PR introduces **11 instances** across 4 files: | File | Line(s) | Suppression | Fix | |------|---------|-------------|-----| | `container_lifecycle_steps.py` | 421, 841, 913, 925 | `[arg-type]` | Use `typing.cast()` or `object()` instead of string literals for wrong-type tests | | `server_lifecycle_steps.py` | 13 | `[import-untyped]` | Other step files import behave without this — remove it | | `server_lifecycle_steps.py` | 53 | `[arg-type]` | Use `cast()` or pass through `Any`-typed variable | | `helper_container_lifecycle_server.py` | 189, 195, 201, 207 | `[union-attr]` | Add `assert container is not None` before `.status` access | | `helper_server_lifecycle.py` | 113 | `[operator]` | Type `_COMMANDS` as `dict[str, Callable[[], None]]` | #### 3. Files Exceeding 500-Line Limit - **`features/steps/container_lifecycle_steps.py`**: **929 lines** (nearly 2× the limit). Split into separate step files per concern: `devcontainer_spec_steps.py`, `resource_limits_steps.py`, `container_manager_steps.py`, `health_monitor_steps.py`. - **`src/cleveragents/infrastructure/server/containers/container_manager.py`**: **524 lines**. Extract `ManagedContainer` model and `ContainerStatus` enum into a separate `models.py` module. #### 4. Empty PR Body The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., `Closes #865`). The commit message has `ISSUES CLOSED: #865` but the PR body must also contain the closing keyword and a summary of changes. --- ### 🟡 Correctness Bug (NEW — not in prior review) #### 5. KB Memory Parsing Bug in `_parse_memory_string` `resource_limits.py` line 170: ```python for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)): ``` The `"kb"` multiplier is `1` (same as MB), which is **incorrect**. This means `"512kb"` would be parsed as **512 MB** instead of approximately **0.5 MB**. The correct multiplier for KB→MB conversion should be `1/1024`. Since the function returns `int`, use: ```python for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1/1024)): ``` Or handle KB separately with integer division. --- ### 🟡 Significant Design Issues #### 6. Encapsulation Violation in HealthMonitor `health_monitor.py:298` calls `self._manager._transition(...)`, directly accessing a **private method** of `ContainerManager`. This breaks encapsulation and creates a fragile coupling. Either: - Make `_transition` public (rename to `transition` or `update_status`), or - Add a public `mark_error(name: str, message: str) -> None` method to `ContainerManager` that `HealthMonitor` can call. #### 7. Weak Typing for `run_command` Parameters Both `ContainerManager.__init__` and `HealthMonitor.__init__` accept `run_command: Any`. This defeats static type checking entirely. Define a proper Protocol: ```python from typing import Protocol class CommandRunner(Protocol): def __call__( self, args: list[str], *, capture_output: bool, text: bool, timeout: int, check: bool, ) -> subprocess.CompletedProcess[str]: ... ``` #### 8. Bug in Coverage Boost Test `server_lifecycle_coverage_boost_steps.py:204`: ```python ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest") ``` `ManagedContainer` has no `image` field. Pydantic v2 silently ignores extra fields by default, so this doesn't crash, but it's misleading — the `image` kwarg does nothing. Remove it. --- ### ✅ What's Good - **Clean state machine**: `PENDING → CREATED → RUNNING → STOPPED → DESTROYED` with `ERROR` from any active state, validated transitions, and proper error propagation. - **Pluggable Docker CLI**: All Docker interactions use DI-friendly `run_command` callable, fully mocked in tests — no real Docker needed. - **DevcontainerSpec**: Comprehensive parsing supporting multiple mount formats (dict, Docker string, shorthand), camelCase→snake_case mapping, and forward-compatible `extra="ignore"`. - **ResourceLimits**: Clean Pydantic model with proper validation, coercion, and Docker CLI argument generation. - **Test coverage**: 67+ BDD scenarios + 13 Robot integration tests covering happy paths, error paths, edge cases, and state transitions. - **Thread safety**: `ContainerManager` and `HealthMonitor` use `threading.RLock` for concurrent access protection. - **Proper argument validation**: All public methods validate inputs with descriptive error messages following fail-fast principles. --- ### Action Items 1. **Wait for PR #1107 to merge**, then rebase this branch onto `master` 2. **Remove all 11 `# type: ignore` suppressions** using the fixes described above 3. **Split oversized files** to stay under 500 lines 4. **Add a PR body** with `Closes #865` and a description of changes 5. **Fix the KB parsing bug** in `_parse_memory_string` (new finding) 6. **Fix the encapsulation violation** — add a public method to `ContainerManager` for error transitions 7. **Fix the `run_command` typing** — use a Protocol instead of `Any` 8. **Remove the `image` kwarg** from the coverage boost test --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
freemo commented 2026-04-02 23:04:54 +00:00
Author
Owner
Copy link

Review claimed by reviewer pool instance pr-reviewer-pool-3983434-1775170710. Dispatching independent code review.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

Review claimed by reviewer pool instance pr-reviewer-pool-3983434-1775170710. Dispatching independent code review. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-03 03:54:48 +00:00
Author
Owner
Copy link

Merge conflict detected. This PR has mergeable: false — the branch has conflicts with master. The implementing agent needs to rebase this branch onto latest master before this PR can be reviewed and merged.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

Merge conflict detected. This PR has `mergeable: false` — the branch has conflicts with master. The implementing agent needs to rebase this branch onto latest master before this PR can be reviewed and merged. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-03 18:57:03 +00:00
Author
Owner
Copy link

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775243000]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775243000] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-03 19:06:07 +00:00
Author
Owner
Copy link

Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle

Third Independent Review

This is a third independent review of PR #1121. The head SHA (9c4655fd) has not changed since the two prior reviews (comments #80403 and #81167), confirming that none of the previously identified issues have been addressed. All prior blockers remain.

🔴 Hard Blockers

1. Merge Conflicts — PR is not mergeable

mergeable: false. This PR carries commit 5f0a5451 from PR #1107 (feat(server): ASGI endpoint via uvicorn), which is itself still open with merge conflicts. PR #1107 must merge first, then this branch must be rebased onto master to isolate only the container-specific changes. Until this dependency is resolved, this PR cannot be merged regardless of code quality.

2. Prohibited # type: ignore Suppressions (11 instances)

CONTRIBUTING.md strictly prohibits # type: ignore in any form. All 11 instances remain:

File Line(s) Suppression Fix
features/steps/container_lifecycle_steps.py 421 [arg-type] Use typing.cast() or object() sentinel
features/steps/container_lifecycle_steps.py 841 [arg-type] Use typing.cast() or object() sentinel
features/steps/container_lifecycle_steps.py 913, 925 [arg-type] Use typing.cast() or object() sentinel
features/steps/server_lifecycle_steps.py 13 [import-untyped] Remove — container_lifecycle_steps.py imports behave without this
features/steps/server_lifecycle_steps.py 53 [arg-type] Use cast() or pass through Any-typed variable
robot/helper_container_lifecycle_server.py 189, 195, 201, 207 [union-attr] Add assert container is not None before .status access
robot/helper_server_lifecycle.py 113 [operator] Type _COMMANDS as dict[str, Callable[[], None]]

3. Files Exceeding 500-Line Limit

  • features/steps/container_lifecycle_steps.py: 929 lines (nearly 2× the limit). Split into separate step files per concern: devcontainer_spec_steps.py, resource_limits_steps.py, container_manager_steps.py, health_monitor_steps.py.
  • src/cleveragents/infrastructure/server/containers/container_manager.py: 524 lines (over limit). Extract ManagedContainer model and ContainerStatus enum into a separate models.py module.

4. Empty PR Body

The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., Closes #865). The commit message has ISSUES CLOSED: #865 but the PR body must also contain the closing keyword and a summary of changes.

🔴 Correctness Bug

5. KB Memory Parsing Bug in _parse_memory_string

resource_limits.py line ~170:

for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)):

The "kb" multiplier is 1 (same as MB), which is incorrect. "512kb" would be parsed as 512 MB instead of approximately 0.5 MB. Fix:

for suffix, multiplier in (("gb", 1024), ("mb", 1)):
    ...
# Handle KB separately:
if value.endswith("kb"):
    num_str = value[:-2].strip()
    return max(1, int(float(num_str) / 1024))

🟡 Design Issues

6. Encapsulation Violation in HealthMonitor

health_monitor.py line 298 calls self._manager._transition(...), directly accessing a private method of ContainerManager. This creates fragile coupling. Either:

  • Make _transition public (rename to transition or update_status), or
  • Add a public mark_error(name: str, message: str) -> None method to ContainerManager.

7. Weak Typing for run_command Parameters

Both ContainerManager.__init__ (line 171) and HealthMonitor.__init__ accept run_command: Any. This defeats static type checking. Define a Protocol:

from typing import Protocol
class CommandRunner(Protocol):
    def __call__(
        self, args: list[str], *, capture_output: bool,
        text: bool, timeout: int, check: bool,
    ) -> subprocess.CompletedProcess[str]: ...

8. Misleading image kwarg in Coverage Boost Test

server_lifecycle_coverage_boost_steps.py:206:

ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest")

ManagedContainer has no image field. Pydantic v2 silently ignores extra fields by default, so this doesn't crash, but it's misleading — the image kwarg does nothing. Remove it.

✅ What's Good

The underlying implementation is solid:

  • Clean state machine: PENDING → CREATED → RUNNING → STOPPED → DESTROYED with ERROR from any active state, validated transitions.
  • Pluggable Docker CLI: All Docker interactions use DI-friendly run_command callable, fully mocked in tests.
  • DevcontainerSpec (380 lines, well within limit): Comprehensive parsing supporting multiple mount formats, camelCase→snake_case mapping.
  • ResourceLimits: Clean Pydantic model with proper validation, coercion, and Docker CLI argument generation (aside from the KB bug).
  • Thread safety: ContainerManager and HealthMonitor use threading.RLock.
  • Test coverage: 67+ BDD scenarios + Robot integration tests.
  • Proper argument validation: All public methods validate inputs with descriptive error messages.

Action Items (Priority Order)

  1. Wait for PR #1107 to merge, then rebase this branch onto master
  2. Remove all 11 # type: ignore suppressions using the fixes described above
  3. Split oversized files to stay under 500 lines
  4. Add a PR body with Closes #865 and a description of changes
  5. Fix the KB parsing bug in _parse_memory_string
  6. Fix the encapsulation violation — add a public method to ContainerManager for error transitions
  7. Fix the run_command typing — use a Protocol instead of Any
  8. Remove the image kwarg from the coverage boost test

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle ### Third Independent Review This is a third independent review of PR #1121. The head SHA (`9c4655fd`) has not changed since the two prior reviews (comments #80403 and #81167), confirming that **none of the previously identified issues have been addressed**. All prior blockers remain. --- ### 🔴 Hard Blockers #### 1. Merge Conflicts — PR is not mergeable `mergeable: false`. This PR carries commit `5f0a5451` from PR #1107 (`feat(server): ASGI endpoint via uvicorn`), which is itself still open with merge conflicts. **PR #1107 must merge first**, then this branch must be rebased onto `master` to isolate only the container-specific changes. Until this dependency is resolved, this PR cannot be merged regardless of code quality. #### 2. Prohibited `# type: ignore` Suppressions (11 instances) CONTRIBUTING.md strictly prohibits `# type: ignore` in any form. All 11 instances remain: | File | Line(s) | Suppression | Fix | |------|---------|-------------|-----| | `features/steps/container_lifecycle_steps.py` | 421 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/container_lifecycle_steps.py` | 841 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/container_lifecycle_steps.py` | 913, 925 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/server_lifecycle_steps.py` | 13 | `[import-untyped]` | Remove — `container_lifecycle_steps.py` imports behave without this | | `features/steps/server_lifecycle_steps.py` | 53 | `[arg-type]` | Use `cast()` or pass through `Any`-typed variable | | `robot/helper_container_lifecycle_server.py` | 189, 195, 201, 207 | `[union-attr]` | Add `assert container is not None` before `.status` access | | `robot/helper_server_lifecycle.py` | 113 | `[operator]` | Type `_COMMANDS` as `dict[str, Callable[[], None]]` | #### 3. Files Exceeding 500-Line Limit - **`features/steps/container_lifecycle_steps.py`**: **929 lines** (nearly 2× the limit). Split into separate step files per concern: `devcontainer_spec_steps.py`, `resource_limits_steps.py`, `container_manager_steps.py`, `health_monitor_steps.py`. - **`src/cleveragents/infrastructure/server/containers/container_manager.py`**: **524 lines** (over limit). Extract `ManagedContainer` model and `ContainerStatus` enum into a separate `models.py` module. #### 4. Empty PR Body The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., `Closes #865`). The commit message has `ISSUES CLOSED: #865` but the PR body must also contain the closing keyword and a summary of changes. --- ### 🔴 Correctness Bug #### 5. KB Memory Parsing Bug in `_parse_memory_string` `resource_limits.py` line ~170: ```python for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)): ``` The `"kb"` multiplier is `1` (same as MB), which is **incorrect**. `"512kb"` would be parsed as **512 MB** instead of approximately **0.5 MB**. Fix: ```python for suffix, multiplier in (("gb", 1024), ("mb", 1)): ... # Handle KB separately: if value.endswith("kb"): num_str = value[:-2].strip() return max(1, int(float(num_str) / 1024)) ``` --- ### 🟡 Design Issues #### 6. Encapsulation Violation in HealthMonitor `health_monitor.py` line 298 calls `self._manager._transition(...)`, directly accessing a **private method** of `ContainerManager`. This creates fragile coupling. Either: - Make `_transition` public (rename to `transition` or `update_status`), or - Add a public `mark_error(name: str, message: str) -> None` method to `ContainerManager`. #### 7. Weak Typing for `run_command` Parameters Both `ContainerManager.__init__` (line 171) and `HealthMonitor.__init__` accept `run_command: Any`. This defeats static type checking. Define a Protocol: ```python from typing import Protocol class CommandRunner(Protocol): def __call__( self, args: list[str], *, capture_output: bool, text: bool, timeout: int, check: bool, ) -> subprocess.CompletedProcess[str]: ... ``` #### 8. Misleading `image` kwarg in Coverage Boost Test `server_lifecycle_coverage_boost_steps.py:206`: ```python ManagedContainer(name=name, status=ContainerStatus.CREATED, image="test:latest") ``` `ManagedContainer` has no `image` field. Pydantic v2 silently ignores extra fields by default, so this doesn't crash, but it's misleading — the `image` kwarg does nothing. Remove it. --- ### ✅ What's Good The underlying implementation is solid: - **Clean state machine**: `PENDING → CREATED → RUNNING → STOPPED → DESTROYED` with `ERROR` from any active state, validated transitions. - **Pluggable Docker CLI**: All Docker interactions use DI-friendly `run_command` callable, fully mocked in tests. - **DevcontainerSpec** (380 lines, well within limit): Comprehensive parsing supporting multiple mount formats, camelCase→snake_case mapping. - **ResourceLimits**: Clean Pydantic model with proper validation, coercion, and Docker CLI argument generation (aside from the KB bug). - **Thread safety**: `ContainerManager` and `HealthMonitor` use `threading.RLock`. - **Test coverage**: 67+ BDD scenarios + Robot integration tests. - **Proper argument validation**: All public methods validate inputs with descriptive error messages. --- ### Action Items (Priority Order) 1. **Wait for PR #1107 to merge**, then rebase this branch onto `master` 2. **Remove all 11 `# type: ignore` suppressions** using the fixes described above 3. **Split oversized files** to stay under 500 lines 4. **Add a PR body** with `Closes #865` and a description of changes 5. **Fix the KB parsing bug** in `_parse_memory_string` 6. **Fix the encapsulation violation** — add a public method to `ContainerManager` for error transitions 7. **Fix the `run_command` typing** — use a Protocol instead of `Any` 8. **Remove the `image` kwarg** from the coverage boost test --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
freemo commented 2026-04-05 03:58:16 +00:00
Author
Owner
Copy link

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775360000]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775360000] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
freemo commented 2026-04-05 04:10:32 +00:00
Author
Owner
Copy link

Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle

Fourth Independent Review (HEAD: 9c4655fd)

The head SHA has not changed since the three prior reviews. All previously identified blockers remain unresolved. This review confirms the outstanding issues and reiterates the required fixes.

🔴 Hard Blockers (Unchanged)

1. Merge Conflicts — PR is not mergeable

mergeable: false. This PR carries commit 5f0a5451 from PR #1107 (feat(server): ASGI endpoint via uvicorn), which is itself still open with mergeable: false. PR #1107 must merge first, then this branch must be rebased onto master to isolate only the container-specific changes. Until this dependency is resolved, this PR cannot be merged regardless of code quality.

2. Prohibited # type: ignore Suppressions (11 instances)

CONTRIBUTING.md strictly prohibits # type: ignore in any form. All 11 instances remain across 4 files:

File Line(s) Suppression Fix
features/steps/container_lifecycle_steps.py 421 [arg-type] Use typing.cast() or object() sentinel
features/steps/container_lifecycle_steps.py 841 [arg-type] Use typing.cast() or object() sentinel
features/steps/container_lifecycle_steps.py 913, 925 [arg-type] Use typing.cast() or object() sentinel
features/steps/server_lifecycle_steps.py 13 [import-untyped] Remove — container_lifecycle_steps.py imports behave without this
features/steps/server_lifecycle_steps.py 53 [arg-type] Use cast() or pass through Any-typed variable
robot/helper_container_lifecycle_server.py 189, 195, 201, 207 [union-attr] Add assert container is not None before .status access
robot/helper_server_lifecycle.py 113 [operator] Type _COMMANDS as dict[str, Callable[[], None]]

3. Files Exceeding 500-Line Limit

  • features/steps/container_lifecycle_steps.py: 929 lines (nearly 2× the limit). Split into separate step files per concern.
  • src/cleveragents/infrastructure/server/containers/container_manager.py: 524 lines. Extract ManagedContainer model and ContainerStatus enum into a separate models.py module.

4. Empty PR Body

The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., Closes #865).

🔴 Correctness Bug (Unchanged)

5. KB Memory Parsing Bug in _parse_memory_string

resource_limits.py line 168:

for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)):

The "kb" multiplier is 1 (same as MB), which is incorrect. "512kb" would be parsed as 512 MB instead of approximately 0.5 MB.

🟡 Design Issues (Unchanged)

6. Encapsulation Violation in HealthMonitor

health_monitor.py:293 calls self._manager._transition(...), directly accessing a private method of ContainerManager.

7. Weak Typing for run_command Parameters

Both ContainerManager.__init__ and HealthMonitor.__init__ accept run_command: Any. Define a proper Protocol instead.

Summary

This is the fourth review with the same findings. The head SHA (9c4655fd) has not changed since the first review. All 4 hard blockers, 1 correctness bug, and 2 design issues remain. Please address these before requesting another review.

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-pr-self-reviewer

## Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle ### Fourth Independent Review (HEAD: `9c4655fd`) The head SHA has not changed since the three prior reviews. **All previously identified blockers remain unresolved.** This review confirms the outstanding issues and reiterates the required fixes. --- ### 🔴 Hard Blockers (Unchanged) #### 1. Merge Conflicts — PR is not mergeable `mergeable: false`. This PR carries commit `5f0a5451` from PR #1107 (`feat(server): ASGI endpoint via uvicorn`), which is itself still open with `mergeable: false`. **PR #1107 must merge first**, then this branch must be rebased onto `master` to isolate only the container-specific changes. Until this dependency is resolved, this PR cannot be merged regardless of code quality. #### 2. Prohibited `# type: ignore` Suppressions (11 instances) CONTRIBUTING.md strictly prohibits `# type: ignore` in any form. All 11 instances remain across 4 files: | File | Line(s) | Suppression | Fix | |------|---------|-------------|-----| | `features/steps/container_lifecycle_steps.py` | 421 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/container_lifecycle_steps.py` | 841 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/container_lifecycle_steps.py` | 913, 925 | `[arg-type]` | Use `typing.cast()` or `object()` sentinel | | `features/steps/server_lifecycle_steps.py` | 13 | `[import-untyped]` | Remove — `container_lifecycle_steps.py` imports behave without this | | `features/steps/server_lifecycle_steps.py` | 53 | `[arg-type]` | Use `cast()` or pass through `Any`-typed variable | | `robot/helper_container_lifecycle_server.py` | 189, 195, 201, 207 | `[union-attr]` | Add `assert container is not None` before `.status` access | | `robot/helper_server_lifecycle.py` | 113 | `[operator]` | Type `_COMMANDS` as `dict[str, Callable[[], None]]` | #### 3. Files Exceeding 500-Line Limit - **`features/steps/container_lifecycle_steps.py`**: **929 lines** (nearly 2× the limit). Split into separate step files per concern. - **`src/cleveragents/infrastructure/server/containers/container_manager.py`**: **524 lines**. Extract `ManagedContainer` model and `ContainerStatus` enum into a separate `models.py` module. #### 4. Empty PR Body The PR description is empty. CONTRIBUTING.md requires a detailed description with a closing keyword (e.g., `Closes #865`). --- ### 🔴 Correctness Bug (Unchanged) #### 5. KB Memory Parsing Bug in `_parse_memory_string` `resource_limits.py` line 168: ```python for suffix, multiplier in (("gb", 1024), ("mb", 1), ("kb", 1)): ``` The `"kb"` multiplier is `1` (same as MB), which is **incorrect**. `"512kb"` would be parsed as **512 MB** instead of approximately **0.5 MB**. --- ### 🟡 Design Issues (Unchanged) #### 6. Encapsulation Violation in HealthMonitor `health_monitor.py:293` calls `self._manager._transition(...)`, directly accessing a private method of `ContainerManager`. #### 7. Weak Typing for `run_command` Parameters Both `ContainerManager.__init__` and `HealthMonitor.__init__` accept `run_command: Any`. Define a proper Protocol instead. --- ### Summary This is the fourth review with the same findings. The head SHA (`9c4655fd`) has not changed since the first review. All 4 hard blockers, 1 correctness bug, and 2 design issues remain. Please address these before requesting another review. --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-pr-self-reviewer
freemo commented 2026-04-05 06:18:01 +00:00
Author
Owner
Copy link

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775369700]

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: ca-continuous-pr-reviewer

🔒 Review claimed by reviewer-pool-1 [claim-token: reviewer-pool-1-1121-1775369700] --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: ca-continuous-pr-reviewer
HAL9000 requested changes 2026-04-08 17:49:10 +00:00
HAL9000 left a comment
Copy link

Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle

Fifth Independent Review (HEAD: 9c4655fd) — Focus: Resource Management, Memory Leaks, Cleanup Patterns

This review provides a new perspective focused on resource management, memory leaks, and cleanup patterns — areas not covered by the four prior reviews. The HEAD SHA has not changed since those reviews, so all previously identified blockers (merge conflicts, # type: ignore suppressions, oversized files, empty PR body) remain unresolved and are summarized briefly below.

🔴 Previously Identified Hard Blockers (Still Unresolved)

These were identified in reviews #2–#5 and remain unchanged:

  1. Merge conflicts — mergeable: false. Depends on PR #1107 merging first.
  2. 11 # type: ignore suppressions — Prohibited by CONTRIBUTING.md.
  3. 2 files over 500-line limit — container_lifecycle_steps.py (929 lines), container_manager.py (524 lines).
  4. Empty PR body — Missing closing keyword (Closes #865) and description.
  5. KB memory parsing bug — resource_limits.py treats "kb" multiplier as 1 (same as MB).

🔴 NEW: Resource Management & Memory Leak Issues

1. CRITICAL: Destroyed containers never removed from registry — unbounded memory leak

  • Location: container_manager.py — destroy() method (lines ~340–380)
  • Issue: When destroy() is called, the container transitions to DESTROYED status but remains in self._containers forever. Since DESTROYED is a terminal state (no valid outbound transitions), these entries are dead weight that accumulate indefinitely.
  • Impact: In a long-running server managing many containers over time, the _containers dict grows without bound. Each ManagedContainer holds a DevcontainerSpec (with mounts, env vars, features) and a ResourceLimits — non-trivial memory per entry.
  • Required fix: Either:
    • (a) Remove the container from self._containers after successful destroy, or
    • (b) Add a purge_destroyed() method that cleans up terminal entries, or
    • (c) Automatically evict DESTROYED entries after a configurable TTL
# Option (a) — simplest fix in destroy():
def destroy(self, name: str) -> ManagedContainer:
    # ... existing destroy logic ...
    self._transition(name, ContainerStatus.DESTROYED)
    with self._lock:
        destroyed = self._containers.pop(name)  # Remove from registry
    logger.info("Container '%s' destroyed and removed from registry", name)
    return destroyed

2. SIGNIFICANT: Dead health monitor threads leak in tracking dictionaries

  • Location: health_monitor.py — _health_loop() (lines ~270–300)
  • Issue: When a health check fails, the _health_loop calls break and the thread exits. However, self._stop_events[container_name] and self._threads[container_name] still hold references to the dead thread and its stop event. These are only cleaned up if someone explicitly calls stop_monitoring().
  • Impact: Over time, if containers become unhealthy and their health loops exit, the monitor accumulates dead Thread and Event objects in its tracking dicts. The monitored_containers property will report containers that are no longer actually being monitored.
  • Required fix: Clean up tracking state when the health loop exits naturally:
def _health_loop(self, container_name: str, stop_event: threading.Event) -> None:
    try:
        while not stop_event.is_set():
            stop_event.wait(self._interval)
            if stop_event.is_set():
                break
            result = self.probe(container_name)
            if not result.healthy:
                # ... existing error handling ...
                break
    finally:
        # Clean up tracking state when loop exits for any reason
        with self._lock:
            self._stop_events.pop(container_name, None)
            self._threads.pop(container_name, None)

3. SIGNIFICANT: No lifecycle management protocol — no way to ensure cleanup

  • Location: Both container_manager.py and health_monitor.py
  • Issue: Neither ContainerManager nor HealthMonitor implements __enter__/__exit__ (context manager protocol) or a shutdown()/close() method. If a ContainerManager instance is garbage collected while containers are still running, those Docker containers become orphaned on the host with no way to track or clean them up.
  • Impact: In error scenarios (server crash, unhandled exception), running containers are leaked at the Docker level. The HealthMonitor has stop_all() but there's no equivalent for ContainerManager.
  • Required fix: Add context manager support and a shutdown() method:
class ContainerManager:
    def shutdown(self) -> None:
        """Stop and destroy all managed containers."""
        for name in list(self._containers):
            container = self._containers[name]
            if container.status == ContainerStatus.RUNNING:
                try:
                    self.stop(name)
                except RuntimeError:
                    pass
            if container.status in (ContainerStatus.CREATED, ContainerStatus.STOPPED, ContainerStatus.ERROR):
                try:
                    self.destroy(name)
                except (RuntimeError, ValueError):
                    pass

    def __enter__(self) -> ContainerManager:
        return self

    def __exit__(self, *exc: object) -> None:
        self.shutdown()

4. SIGNIFICANT: No way to retry failed container creation

  • Location: container_manager.py — create() method (lines ~220–280)
  • Issue: If create() fails (e.g., Docker timeout, network error), the container transitions to ERROR state but remains in self._containers. Calling create() again with the same name raises ValueError("Container 'X' already exists"). There is no remove(), unregister(), or retry_create() method.
  • Impact: A transient failure during container creation permanently blocks that container name. The only recovery path is to create a new ContainerManager instance entirely.
  • Required fix: Add a remove() method for containers in terminal/error states:
def remove(self, name: str) -> None:
    """Remove a container from the registry (must be in DESTROYED or ERROR state)."""
    with self._lock:
        container = self._containers.get(name)
        if container is None:
            raise ValueError(f"Container '{name}' not found")
        if container.status not in (ContainerStatus.DESTROYED, ContainerStatus.ERROR):
            raise ValueError(
                f"Cannot remove container in '{container.status.value}' state"
            )
        del self._containers[name]

🟡 NEW: Correctness Issues Related to Resource Management

5. Race condition: HealthMonitor can override DESTROYED state via _transition

  • Location: health_monitor.py:298 → container_manager.py _transition() method
  • Issue: The _transition() method does not validate state transitions — it directly sets container.status = target. This is by design for internal use, but the HealthMonitor calls self._manager._transition(container_name, ContainerStatus.ERROR, ...) from a background thread. If a container is being destroyed concurrently:
    1. destroy() validates transition and starts docker rm
    2. Health monitor's _health_loop calls probe() → gets error → calls _transition(name, ERROR)
    3. Container is now in ERROR instead of DESTROYED
    4. destroy() then calls _transition(name, DESTROYED) — but the state machine is now inconsistent
  • Impact: Container state can become corrupted under concurrent access, potentially leaving Docker resources in an inconsistent state.
  • Required fix: _transition() should validate the transition, or the HealthMonitor should use a public method that validates (this also addresses the encapsulation violation noted in prior reviews).

6. destroy() swallows Docker rm failures — potential Docker-level resource leak

  • Location: container_manager.py — destroy() method (lines ~370–380)
  • Issue: If docker rm -f fails or throws an exception, the error is logged as a warning but the container is still transitioned to DESTROYED. This means the manager believes the container is gone, but the Docker container may still be running/existing on the host.
  • Impact: Over time, orphaned Docker containers accumulate on the host, consuming resources (memory, CPU, disk, network ports).
  • Suggested improvement: At minimum, set an error_message on the container when rm fails so callers can detect the inconsistency. Consider keeping the container in ERROR state if rm fails, allowing retry.

✅ What's Good (Resource Management Perspective)

  • Thread safety: Both ContainerManager and HealthMonitor use threading.RLock consistently for concurrent access protection.
  • Bounded probe history: HealthMonitor._probes is trimmed at 1000 entries (to 500), preventing unbounded growth.
  • Daemon threads: Health monitor threads are daemon threads, so they won't prevent process exit.
  • Timeout on all subprocess calls: All run_command calls have explicit timeouts (300s for create, 60s for start/stop/rm, 10s for inspect), preventing indefinite hangs.
  • Pluggable command runner: The DI pattern for run_command makes testing clean and avoids real Docker dependencies in tests.

Summary of All Outstanding Issues

# Severity Issue Source
1 🔴 BLOCKER Merge conflicts (mergeable: false) Prior reviews
2 🔴 BLOCKER 11 # type: ignore suppressions Prior reviews
3 🔴 BLOCKER 2 files over 500-line limit Prior reviews
4 🔴 BLOCKER Empty PR body (no closing keyword) Prior reviews
5 🔴 CRITICAL KB memory parsing bug Prior reviews
6 🔴 CRITICAL Destroyed containers never removed — memory leak NEW
7 🟡 SIGNIFICANT Dead health monitor threads leak in tracking dicts NEW
8 🟡 SIGNIFICANT No lifecycle management protocol (context manager/shutdown) NEW
9 🟡 SIGNIFICANT No way to retry failed container creation NEW
10 🟡 SIGNIFICANT Race condition: HealthMonitor can corrupt container state NEW
11 🟡 MODERATE Encapsulation violation (_transition access) Prior reviews
12 🟡 MODERATE Weak typing for run_command (Any) Prior reviews
13 🟠 MINOR destroy() swallows Docker rm failures NEW

Recommended Priority for Fixes

  1. Rebase onto master (after PR #1107 merges)
  2. Fix the memory leak in destroy() — add container removal or purge mechanism
  3. Fix health monitor thread cleanup in _health_loop
  4. Add shutdown()/context manager to ContainerManager
  5. Add remove() method for error/destroyed containers
  6. Fix _transition to validate state changes (also fixes encapsulation violation)
  7. Remove all # type: ignore suppressions
  8. Split oversized files
  9. Add PR body with Closes #865
  10. Fix KB parsing bug

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-self-reviewer

## Code Review: REQUEST CHANGES — feat(server): container/devcontainer support lifecycle ### Fifth Independent Review (HEAD: `9c4655fd`) — Focus: Resource Management, Memory Leaks, Cleanup Patterns This review provides a **new perspective** focused on **resource management, memory leaks, and cleanup patterns** — areas not covered by the four prior reviews. The HEAD SHA has not changed since those reviews, so all previously identified blockers (merge conflicts, `# type: ignore` suppressions, oversized files, empty PR body) remain unresolved and are summarized briefly below. --- ### 🔴 Previously Identified Hard Blockers (Still Unresolved) These were identified in reviews #2–#5 and remain unchanged: 1. **Merge conflicts** — `mergeable: false`. Depends on PR #1107 merging first. 2. **11 `# type: ignore` suppressions** — Prohibited by CONTRIBUTING.md. 3. **2 files over 500-line limit** — `container_lifecycle_steps.py` (929 lines), `container_manager.py` (524 lines). 4. **Empty PR body** — Missing closing keyword (`Closes #865`) and description. 5. **KB memory parsing bug** — `resource_limits.py` treats `"kb"` multiplier as `1` (same as MB). --- ### 🔴 NEW: Resource Management & Memory Leak Issues #### 1. **CRITICAL: Destroyed containers never removed from registry — unbounded memory leak** - **Location**: `container_manager.py` — `destroy()` method (lines ~340–380) - **Issue**: When `destroy()` is called, the container transitions to `DESTROYED` status but **remains in `self._containers` forever**. Since `DESTROYED` is a terminal state (no valid outbound transitions), these entries are dead weight that accumulate indefinitely. - **Impact**: In a long-running server managing many containers over time, the `_containers` dict grows without bound. Each `ManagedContainer` holds a `DevcontainerSpec` (with mounts, env vars, features) and a `ResourceLimits` — non-trivial memory per entry. - **Required fix**: Either: - (a) Remove the container from `self._containers` after successful destroy, or - (b) Add a `purge_destroyed()` method that cleans up terminal entries, or - (c) Automatically evict `DESTROYED` entries after a configurable TTL ```python # Option (a) — simplest fix in destroy(): def destroy(self, name: str) -> ManagedContainer: # ... existing destroy logic ... self._transition(name, ContainerStatus.DESTROYED) with self._lock: destroyed = self._containers.pop(name) # Remove from registry logger.info("Container '%s' destroyed and removed from registry", name) return destroyed ``` #### 2. **SIGNIFICANT: Dead health monitor threads leak in tracking dictionaries** - **Location**: `health_monitor.py` — `_health_loop()` (lines ~270–300) - **Issue**: When a health check fails, the `_health_loop` calls `break` and the thread exits. However, `self._stop_events[container_name]` and `self._threads[container_name]` **still hold references** to the dead thread and its stop event. These are only cleaned up if someone explicitly calls `stop_monitoring()`. - **Impact**: Over time, if containers become unhealthy and their health loops exit, the monitor accumulates dead `Thread` and `Event` objects in its tracking dicts. The `monitored_containers` property will report containers that are no longer actually being monitored. - **Required fix**: Clean up tracking state when the health loop exits naturally: ```python def _health_loop(self, container_name: str, stop_event: threading.Event) -> None: try: while not stop_event.is_set(): stop_event.wait(self._interval) if stop_event.is_set(): break result = self.probe(container_name) if not result.healthy: # ... existing error handling ... break finally: # Clean up tracking state when loop exits for any reason with self._lock: self._stop_events.pop(container_name, None) self._threads.pop(container_name, None) ``` #### 3. **SIGNIFICANT: No lifecycle management protocol — no way to ensure cleanup** - **Location**: Both `container_manager.py` and `health_monitor.py` - **Issue**: Neither `ContainerManager` nor `HealthMonitor` implements `__enter__`/`__exit__` (context manager protocol) or a `shutdown()`/`close()` method. If a `ContainerManager` instance is garbage collected while containers are still running, those Docker containers become **orphaned** on the host with no way to track or clean them up. - **Impact**: In error scenarios (server crash, unhandled exception), running containers are leaked at the Docker level. The `HealthMonitor` has `stop_all()` but there's no equivalent for `ContainerManager`. - **Required fix**: Add context manager support and a `shutdown()` method: ```python class ContainerManager: def shutdown(self) -> None: """Stop and destroy all managed containers.""" for name in list(self._containers): container = self._containers[name] if container.status == ContainerStatus.RUNNING: try: self.stop(name) except RuntimeError: pass if container.status in (ContainerStatus.CREATED, ContainerStatus.STOPPED, ContainerStatus.ERROR): try: self.destroy(name) except (RuntimeError, ValueError): pass def __enter__(self) -> ContainerManager: return self def __exit__(self, *exc: object) -> None: self.shutdown() ``` #### 4. **SIGNIFICANT: No way to retry failed container creation** - **Location**: `container_manager.py` — `create()` method (lines ~220–280) - **Issue**: If `create()` fails (e.g., Docker timeout, network error), the container transitions to `ERROR` state but remains in `self._containers`. Calling `create()` again with the same name raises `ValueError("Container 'X' already exists")`. There is no `remove()`, `unregister()`, or `retry_create()` method. - **Impact**: A transient failure during container creation permanently blocks that container name. The only recovery path is to create a new `ContainerManager` instance entirely. - **Required fix**: Add a `remove()` method for containers in terminal/error states: ```python def remove(self, name: str) -> None: """Remove a container from the registry (must be in DESTROYED or ERROR state).""" with self._lock: container = self._containers.get(name) if container is None: raise ValueError(f"Container '{name}' not found") if container.status not in (ContainerStatus.DESTROYED, ContainerStatus.ERROR): raise ValueError( f"Cannot remove container in '{container.status.value}' state" ) del self._containers[name] ``` --- ### 🟡 NEW: Correctness Issues Related to Resource Management #### 5. **Race condition: HealthMonitor can override DESTROYED state via `_transition`** - **Location**: `health_monitor.py:298` → `container_manager.py` `_transition()` method - **Issue**: The `_transition()` method does **not validate state transitions** — it directly sets `container.status = target`. This is by design for internal use, but the `HealthMonitor` calls `self._manager._transition(container_name, ContainerStatus.ERROR, ...)` from a background thread. If a container is being destroyed concurrently: 1. `destroy()` validates transition and starts `docker rm` 2. Health monitor's `_health_loop` calls `probe()` → gets error → calls `_transition(name, ERROR)` 3. Container is now in `ERROR` instead of `DESTROYED` 4. `destroy()` then calls `_transition(name, DESTROYED)` — but the state machine is now inconsistent - **Impact**: Container state can become corrupted under concurrent access, potentially leaving Docker resources in an inconsistent state. - **Required fix**: `_transition()` should validate the transition, or the `HealthMonitor` should use a public method that validates (this also addresses the encapsulation violation noted in prior reviews). #### 6. **`destroy()` swallows Docker rm failures — potential Docker-level resource leak** - **Location**: `container_manager.py` — `destroy()` method (lines ~370–380) - **Issue**: If `docker rm -f` fails or throws an exception, the error is logged as a warning but the container is still transitioned to `DESTROYED`. This means the manager believes the container is gone, but the Docker container may still be running/existing on the host. - **Impact**: Over time, orphaned Docker containers accumulate on the host, consuming resources (memory, CPU, disk, network ports). - **Suggested improvement**: At minimum, set an `error_message` on the container when rm fails so callers can detect the inconsistency. Consider keeping the container in `ERROR` state if rm fails, allowing retry. --- ### ✅ What's Good (Resource Management Perspective) - **Thread safety**: Both `ContainerManager` and `HealthMonitor` use `threading.RLock` consistently for concurrent access protection. - **Bounded probe history**: `HealthMonitor._probes` is trimmed at 1000 entries (to 500), preventing unbounded growth. - **Daemon threads**: Health monitor threads are daemon threads, so they won't prevent process exit. - **Timeout on all subprocess calls**: All `run_command` calls have explicit timeouts (300s for create, 60s for start/stop/rm, 10s for inspect), preventing indefinite hangs. - **Pluggable command runner**: The DI pattern for `run_command` makes testing clean and avoids real Docker dependencies in tests. --- ### Summary of All Outstanding Issues | # | Severity | Issue | Source | |---|----------|-------|--------| | 1 | 🔴 BLOCKER | Merge conflicts (`mergeable: false`) | Prior reviews | | 2 | 🔴 BLOCKER | 11 `# type: ignore` suppressions | Prior reviews | | 3 | 🔴 BLOCKER | 2 files over 500-line limit | Prior reviews | | 4 | 🔴 BLOCKER | Empty PR body (no closing keyword) | Prior reviews | | 5 | 🔴 CRITICAL | KB memory parsing bug | Prior reviews | | 6 | 🔴 CRITICAL | **Destroyed containers never removed — memory leak** | **NEW** | | 7 | 🟡 SIGNIFICANT | **Dead health monitor threads leak in tracking dicts** | **NEW** | | 8 | 🟡 SIGNIFICANT | **No lifecycle management protocol (context manager/shutdown)** | **NEW** | | 9 | 🟡 SIGNIFICANT | **No way to retry failed container creation** | **NEW** | | 10 | 🟡 SIGNIFICANT | **Race condition: HealthMonitor can corrupt container state** | **NEW** | | 11 | 🟡 MODERATE | Encapsulation violation (`_transition` access) | Prior reviews | | 12 | 🟡 MODERATE | Weak typing for `run_command` (`Any`) | Prior reviews | | 13 | 🟠 MINOR | `destroy()` swallows Docker rm failures | **NEW** | ### Recommended Priority for Fixes 1. Rebase onto master (after PR #1107 merges) 2. Fix the memory leak in `destroy()` — add container removal or purge mechanism 3. Fix health monitor thread cleanup in `_health_loop` 4. Add `shutdown()`/context manager to `ContainerManager` 5. Add `remove()` method for error/destroyed containers 6. Fix `_transition` to validate state changes (also fixes encapsulation violation) 7. Remove all `# type: ignore` suppressions 8. Split oversized files 9. Add PR body with `Closes #865` 10. Fix KB parsing bug --- **Automated by CleverAgents Bot** Supervisor: PR Review | Agent: pr-self-reviewer
HAL9000 force-pushed feature/m9-container-lifecycle from 9c4655fd5c
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 32s
Details
CI / lint (pull_request) Successful in 4m55s
Details
CI / quality (pull_request) Successful in 5m33s
Details
CI / typecheck (pull_request) Successful in 5m45s
Details
CI / security (pull_request) Successful in 5m55s
Details
CI / unit_tests (pull_request) Successful in 7m48s
Details
CI / integration_tests (pull_request) Successful in 7m51s
Details
CI / docker (pull_request) Successful in 1m2s
Details
CI / e2e_tests (pull_request) Successful in 10m18s
Details
CI / coverage (pull_request) Successful in 11m33s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Successful in 1h1m0s
Details
to 7d6de18afb
Some checks failed
CI / lint (pull_request) Successful in 59s
Details
CI / typecheck (pull_request) Failing after 1m3s
Details
CI / security (pull_request) Successful in 1m13s
Details
CI / quality (pull_request) Successful in 52s
Details
CI / push-validation (pull_request) Successful in 37s
Details
CI / helm (pull_request) Successful in 42s
Details
CI / build (pull_request) Successful in 44s
Details
CI / integration_tests (pull_request) Failing after 4m55s
Details
CI / unit_tests (pull_request) Failing after 6m9s
Details
CI / coverage (pull_request) Has been skipped
Details
CI / docker (pull_request) Has been skipped
Details
CI / status-check (pull_request) Failing after 3s
Details
2026-05-29 04:30:35 +00:00 Compare
fix(server): correct A2aRequest.method usage and A2A test payloads
Some checks failed
CI / lint (pull_request) Successful in 51s
Details
CI / quality (pull_request) Successful in 50s
Details
CI / typecheck (pull_request) Successful in 1m22s
Details
CI / security (pull_request) Successful in 1m23s
Details
CI / push-validation (pull_request) Successful in 38s
Details
CI / helm (pull_request) Successful in 39s
Details
CI / build (pull_request) Successful in 46s
Details
CI / integration_tests (pull_request) Successful in 4m0s
Details
CI / unit_tests (pull_request) Successful in 6m41s
Details
CI / docker (pull_request) Successful in 1m33s
Details
CI / coverage (pull_request) Failing after 13m1s
Details
CI / status-check (pull_request) Failing after 3s
Details
c551760d9f 
asgi_app.py logged a2a_request.operation (no such attribute on
A2aRequest) instead of a2a_request.method — Pyright error.

Both the Behave step definition and the Robot integration helper
sent {"operation": ...} in the JSON body, but A2aRequest requires
{"method": ...} per JSON-RPC 2.0, causing validation to return 400.

The health-check handler returned {"status": "healthy"} while every
test asserted "ok"; align the handler to return "ok".

The A2A response status assertion checked data.get("status") directly
on the JSON-RPC envelope instead of data["result"]["status"]; fixed
in both the Behave step and the Robot helper.
Some checks failed
CI / lint (pull_request) Successful in 51s
Required
Details
CI / quality (pull_request) Successful in 50s
Required
Details
CI / typecheck (pull_request) Successful in 1m22s
Required
Details
CI / security (pull_request) Successful in 1m23s
Required
Details
CI / push-validation (pull_request) Successful in 38s
Details
CI / helm (pull_request) Successful in 39s
Details
CI / build (pull_request) Successful in 46s
Required
Details
CI / integration_tests (pull_request) Successful in 4m0s
Required
Details
CI / unit_tests (pull_request) Successful in 6m41s
Required
Details
CI / docker (pull_request) Successful in 1m33s
Required
Details
CI / coverage (pull_request) Failing after 13m1s
Required
Details
CI / status-check (pull_request) Failing after 3s
Details
This pull request has changes conflicting with the target branch.
  • features/server_lifecycle.feature
  • features/steps/server_lifecycle_steps.py
  • robot/helper_server_lifecycle.py
  • src/cleveragents/infrastructure/server/__init__.py
  • src/cleveragents/infrastructure/server/asgi_app.py
  • src/cleveragents/infrastructure/server/server_lifecycle.py
  • vulture_whitelist.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/m9-container-lifecycle:feature/m9-container-lifecycle
git switch feature/m9-container-lifecycle
Sign in to join this conversation.
Reviewers
No reviewers
hamza.khyari
brent.edwards
HAL9000
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
v3.8.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
freemo
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
cleveragents/cleveragents-core!1121
No description provided.