UAT: Custom sandbox strategy protocol (`SandboxStrategyProtocol`) requires 9 methods inconsistent with built-in `Sandbox` protocol (5 methods) #3514

New issue

Open

opened 2026-04-05 18:45:41 +00:00 by freemo · 0 comments

freemo commented

2026-04-05 18:45:41 +00:00

Owner

Metadata

Branch: fix/sandbox-strategy-protocol-inconsistency
Commit Message: fix(sandbox): align SandboxStrategyProtocol with built-in Sandbox protocol
Milestone: None (backlog — see backlog note below)
Parent Epic: #358

Backlog note: This issue was discovered during autonomous operation
on milestone v3.3.0. It does not block milestone completion and has been
placed in the backlog for human review and future milestone assignment.

Summary

There are two incompatible sandbox protocols in the codebase:

The built-in Sandbox protocol (src/cleveragents/infrastructure/sandbox/protocol.py) with 5 methods: create, get_path, commit, rollback, cleanup
The custom strategy SandboxStrategyProtocol (src/cleveragents/domain/models/core/sandbox_strategy.py) with 9 methods: create, read, write, diff, commit, rollback, checkpoint, restore_checkpoint, cleanup

The SandboxStrategyRegistry validates custom strategies against the 9-method protocol, but all built-in sandbox implementations (GitWorktreeSandbox, CopyOnWriteSandbox, OverlaySandbox, TransactionSandbox, NoSandbox) only implement the 5-method protocol. This means:

Built-in sandboxes cannot be registered as custom strategies
Custom strategies must implement a completely different interface than built-in sandboxes
The checkpoint and restore_checkpoint methods in the custom protocol overlap with the CheckpointService but are not integrated with it

Spec Reference

The spec describes custom sandbox strategies as an extension mechanism:

Custom Sandbox Strategies: Resource types can define custom sandbox strategies by specifying a module and class name in their configuration. Custom strategies are validated against the SandboxStrategyProtocol at registration time.

The spec does not explicitly define two separate protocols — this is an implementation inconsistency.

Current Behavior

Built-in protocol (protocol.py):

class Sandbox(Protocol):
    def create(self, plan_id: str) -> SandboxContext: ...
    def get_path(self, resource_path: str) -> str: ...
    def commit(self, message: str | None = None) -> CommitResult: ...
    def rollback(self) -> None: ...
    def cleanup(self) -> None: ...

Custom strategy protocol (sandbox_strategy.py):

class SandboxStrategyProtocol(Protocol):
    def create(self, plan_id: str, resource: Resource) -> SandboxRef: ...
    def read(self, ref: SandboxRef, path: str) -> bytes: ...
    def write(self, ref: SandboxRef, path: str, content: bytes) -> DiffEntry: ...
    def diff(self, ref: SandboxRef) -> DiffView: ...
    def commit(self, ref: SandboxRef) -> None: ...
    def rollback(self, ref: SandboxRef) -> None: ...
    def checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: ...
    def restore_checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: ...
    def cleanup(self, ref: SandboxRef) -> None: ...

The SandboxStrategyRegistry._validate_protocol() checks for all 9 methods. The SandboxFactory only handles built-in strategies and does not use the registry for routing.

Impact

Custom sandbox strategy authors must implement a completely different interface than built-in sandboxes
The checkpoint and restore_checkpoint methods in SandboxStrategyProtocol duplicate the CheckpointService functionality without integration
The SandboxFactory does not use SandboxStrategyRegistry to route custom strategies — the registry is effectively unused in the production code path
Documentation/spec alignment issue: the two protocols serve different purposes but are not clearly distinguished

Expected Behavior

Either:

Unify the two protocols so custom strategies implement the same interface as built-in sandboxes (with optional checkpoint methods)
Clearly document that custom strategies use a different (richer) interface and update SandboxFactory to route to registered custom strategies
Remove the checkpoint/restore_checkpoint methods from SandboxStrategyProtocol and delegate to CheckpointService instead

Code Locations

src/cleveragents/infrastructure/sandbox/protocol.py — built-in Sandbox protocol
src/cleveragents/domain/models/core/sandbox_strategy.py — SandboxStrategyProtocol
src/cleveragents/infrastructure/sandbox/strategy_registry.py — SandboxStrategyRegistry
src/cleveragents/infrastructure/sandbox/factory.py — SandboxFactory (does not use registry)

Subtasks

Investigate and decide on the correct unification approach (options 1, 2, or 3 above)
Update SandboxStrategyProtocol to align with the chosen approach
Update SandboxStrategyRegistry._validate_protocol() to reflect the updated protocol
Update SandboxFactory to route custom strategies through SandboxStrategyRegistry
Ensure built-in sandbox implementations satisfy the updated protocol (if unified)
Remove or integrate checkpoint/restore_checkpoint with CheckpointService as appropriate
Update documentation and spec references to clearly distinguish protocol roles
Add/update tests covering custom strategy registration and routing via SandboxFactory

Definition of Done

A single, consistent sandbox protocol interface is defined and documented
SandboxStrategyRegistry validates against the correct, unified protocol
SandboxFactory correctly routes custom strategies through SandboxStrategyRegistry
Built-in sandbox implementations satisfy the protocol (no interface mismatch)
checkpoint/restore_checkpoint are either removed or properly integrated with CheckpointService
All nox stages pass
Coverage >= 97%

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-new-issue-creator

## Metadata - **Branch**: `fix/sandbox-strategy-protocol-inconsistency` - **Commit Message**: `fix(sandbox): align SandboxStrategyProtocol with built-in Sandbox protocol` - **Milestone**: None (backlog — see backlog note below) - **Parent Epic**: #358 > **Backlog note:** This issue was discovered during autonomous operation > on milestone v3.3.0. It does not block milestone completion and has been > placed in the backlog for human review and future milestone assignment. ## Summary There are two incompatible sandbox protocols in the codebase: 1. The built-in `Sandbox` protocol (`src/cleveragents/infrastructure/sandbox/protocol.py`) with 5 methods: `create`, `get_path`, `commit`, `rollback`, `cleanup` 2. The custom strategy `SandboxStrategyProtocol` (`src/cleveragents/domain/models/core/sandbox_strategy.py`) with 9 methods: `create`, `read`, `write`, `diff`, `commit`, `rollback`, `checkpoint`, `restore_checkpoint`, `cleanup` The `SandboxStrategyRegistry` validates custom strategies against the 9-method protocol, but all built-in sandbox implementations (`GitWorktreeSandbox`, `CopyOnWriteSandbox`, `OverlaySandbox`, `TransactionSandbox`, `NoSandbox`) only implement the 5-method protocol. This means: - Built-in sandboxes **cannot** be registered as custom strategies - Custom strategies must implement a completely different interface than built-in sandboxes - The `checkpoint` and `restore_checkpoint` methods in the custom protocol overlap with the `CheckpointService` but are not integrated with it ## Spec Reference The spec describes custom sandbox strategies as an extension mechanism: > **Custom Sandbox Strategies**: Resource types can define custom sandbox strategies by specifying a module and class name in their configuration. Custom strategies are validated against the `SandboxStrategyProtocol` at registration time. The spec does not explicitly define two separate protocols — this is an implementation inconsistency. ## Current Behavior **Built-in protocol** (`protocol.py`): ```python class Sandbox(Protocol): def create(self, plan_id: str) -> SandboxContext: ... def get_path(self, resource_path: str) -> str: ... def commit(self, message: str | None = None) -> CommitResult: ... def rollback(self) -> None: ... def cleanup(self) -> None: ... ``` **Custom strategy protocol** (`sandbox_strategy.py`): ```python class SandboxStrategyProtocol(Protocol): def create(self, plan_id: str, resource: Resource) -> SandboxRef: ... def read(self, ref: SandboxRef, path: str) -> bytes: ... def write(self, ref: SandboxRef, path: str, content: bytes) -> DiffEntry: ... def diff(self, ref: SandboxRef) -> DiffView: ... def commit(self, ref: SandboxRef) -> None: ... def rollback(self, ref: SandboxRef) -> None: ... def checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: ... def restore_checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: ... def cleanup(self, ref: SandboxRef) -> None: ... ``` The `SandboxStrategyRegistry._validate_protocol()` checks for all 9 methods. The `SandboxFactory` only handles built-in strategies and does not use the registry for routing. ## Impact - Custom sandbox strategy authors must implement a completely different interface than built-in sandboxes - The `checkpoint` and `restore_checkpoint` methods in `SandboxStrategyProtocol` duplicate the `CheckpointService` functionality without integration - The `SandboxFactory` does not use `SandboxStrategyRegistry` to route custom strategies — the registry is effectively unused in the production code path - Documentation/spec alignment issue: the two protocols serve different purposes but are not clearly distinguished ## Expected Behavior Either: 1. Unify the two protocols so custom strategies implement the same interface as built-in sandboxes (with optional checkpoint methods) 2. Clearly document that custom strategies use a different (richer) interface and update `SandboxFactory` to route to registered custom strategies 3. Remove the `checkpoint`/`restore_checkpoint` methods from `SandboxStrategyProtocol` and delegate to `CheckpointService` instead ## Code Locations - `src/cleveragents/infrastructure/sandbox/protocol.py` — built-in `Sandbox` protocol - `src/cleveragents/domain/models/core/sandbox_strategy.py` — `SandboxStrategyProtocol` - `src/cleveragents/infrastructure/sandbox/strategy_registry.py` — `SandboxStrategyRegistry` - `src/cleveragents/infrastructure/sandbox/factory.py` — `SandboxFactory` (does not use registry) ## Subtasks - [ ] Investigate and decide on the correct unification approach (options 1, 2, or 3 above) - [ ] Update `SandboxStrategyProtocol` to align with the chosen approach - [ ] Update `SandboxStrategyRegistry._validate_protocol()` to reflect the updated protocol - [ ] Update `SandboxFactory` to route custom strategies through `SandboxStrategyRegistry` - [ ] Ensure built-in sandbox implementations satisfy the updated protocol (if unified) - [ ] Remove or integrate `checkpoint`/`restore_checkpoint` with `CheckpointService` as appropriate - [ ] Update documentation and spec references to clearly distinguish protocol roles - [ ] Add/update tests covering custom strategy registration and routing via `SandboxFactory` ## Definition of Done - [ ] A single, consistent sandbox protocol interface is defined and documented - [ ] `SandboxStrategyRegistry` validates against the correct, unified protocol - [ ] `SandboxFactory` correctly routes custom strategies through `SandboxStrategyRegistry` - [ ] Built-in sandbox implementations satisfy the protocol (no interface mismatch) - [ ] `checkpoint`/`restore_checkpoint` are either removed or properly integrated with `CheckpointService` - [ ] All nox stages pass - [ ] Coverage >= 97% --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-new-issue-creator

freemo added the

labels

2026-04-05 18:45:51 +00:00

freemo added a new dependency

2026-04-05 18:45:57 +00:00

#358 Epic: Corrections + Subplans + Checkpoints (M4)