UAT: BuiltInSandboxStrategyAdapter.diff() always returns empty DiffView — no actual diff computation between sandbox and live resource #2589

Open
opened 2026-04-03 19:02:52 +00:00 by freemo · 1 comment
Owner

Bug Report

What Was Tested

BuiltInSandboxStrategyAdapter.diff() in src/cleveragents/infrastructure/sandbox/strategy_adapter.py.

Expected Behavior (from spec)

The spec requires that agents plan diff <PLAN_ID> shows a unified diff of changes between the sandbox and the live resource. The diff() method on the sandbox strategy adapter should compute and return the actual file-level differences.

From the spec's agents plan diff output:

╭─ Diff Summary ─────────────────────────────────╮
│ Plan: 01HXM8C2ZK4Q7C2B3F2R4VYV6J               │
│ Files Changed: 2                               │
│ Insertions: 12                                 │
│ Deletions: 4                                   │
╰────────────────────────────────────────────────╯

╭─ Patch Preview ──────────────────────────────────╮
│ --- a/src/auth/session.py                        │
│ +++ b/src/auth/session.py                        │
│ @@ -12,4 +12,10 @@                               │
│ - import jwt                                     │
│ + import sessionlib                              │
╰──────────────────────────────────────────────────╯

Actual Behavior

BuiltInSandboxStrategyAdapter.diff() always returns an empty DiffView with no entries:

# src/cleveragents/infrastructure/sandbox/strategy_adapter.py
def diff(self, ref: SandboxRef) -> DiffView:
    return DiffView(
        sandbox_id=ref.sandbox_id,
        entries=[],  # Always empty!
        summary="Built-in strategy; diff computed at commit time.",
    )

This means any code path that calls adapter.diff() to show sandbox changes will always receive an empty diff, even when the sandbox contains significant modifications.

Code Location

src/cleveragents/infrastructure/sandbox/strategy_adapter.pydiff() method.

Root Cause

The adapter has access to the underlying Sandbox instance via self._sandboxes[ref.sandbox_id]. The sandbox's context.sandbox_path and context.original_path are available. The _fs_utils.compute_diff() function already implements the diff logic used by CopyOnWriteSandbox.commit() and OverlaySandbox.commit(). The adapter simply needs to call compute_diff(sandbox_path, original_path) and build DiffEntry objects from the result.

Impact

  • agents plan diff cannot show actual changes for plans using built-in sandbox strategies through the adapter
  • The diff() method is part of the SandboxStrategyProtocol and is expected to return meaningful data
  • Any tooling or UI that calls diff() to preview changes before apply will always show "no changes"

Fix

Implement diff() using the existing compute_diff() utility:

from cleveragents.infrastructure.sandbox._fs_utils import compute_diff

def diff(self, ref: SandboxRef) -> DiffView:
    sandbox = self._sandboxes.get(ref.sandbox_id)
    if sandbox is None or sandbox.context is None:
        return DiffView(sandbox_id=ref.sandbox_id, entries=[], summary="No sandbox")
    
    ctx = sandbox.context
    if not os.path.isdir(ctx.sandbox_path) or not os.path.isdir(ctx.original_path):
        return DiffView(sandbox_id=ref.sandbox_id, entries=[], summary="Paths unavailable")
    
    changed, added, deleted = compute_diff(ctx.sandbox_path, ctx.original_path)
    entries = [
        DiffEntry(path=p, operation="modified") for p in changed
    ] + [
        DiffEntry(path=p, operation="added") for p in added
    ] + [
        DiffEntry(path=p, operation="deleted") for p in deleted
    ]
    return DiffView(sandbox_id=ref.sandbox_id, entries=entries, summary=f"{len(entries)} changes")

Note: This fix applies to filesystem-based sandboxes. TransactionSandbox (database) and NoSandbox have no meaningful filesystem diff.

Metadata

  • Commit message: fix(sandbox): implement diff() in BuiltInSandboxStrategyAdapter
  • Branch: fix/sandbox-adapter-diff-implementation
  • Parent Epic: #358 (Corrections + Subplans + Checkpoints M4)

Subtasks

  • Implement diff() using compute_diff() for filesystem-based strategies
  • Handle TransactionSandbox and NoSandbox cases gracefully
  • Add unit tests for diff() with actual file changes

Definition of Done

  • BuiltInSandboxStrategyAdapter.diff() returns non-empty DiffView when sandbox contains file changes
  • DiffEntry objects correctly classify files as modified, added, or deleted
  • TransactionSandbox and NoSandbox cases return empty diff with appropriate summary
  • Unit tests cover the diff computation path

Automated by CleverAgents Bot
Supervisor: UAT Testing | Agent: ca-uat-tester

## Bug Report ### What Was Tested `BuiltInSandboxStrategyAdapter.diff()` in `src/cleveragents/infrastructure/sandbox/strategy_adapter.py`. ### Expected Behavior (from spec) The spec requires that `agents plan diff <PLAN_ID>` shows a unified diff of changes between the sandbox and the live resource. The `diff()` method on the sandbox strategy adapter should compute and return the actual file-level differences. From the spec's `agents plan diff` output: ``` ╭─ Diff Summary ─────────────────────────────────╮ │ Plan: 01HXM8C2ZK4Q7C2B3F2R4VYV6J │ │ Files Changed: 2 │ │ Insertions: 12 │ │ Deletions: 4 │ ╰────────────────────────────────────────────────╯ ╭─ Patch Preview ──────────────────────────────────╮ │ --- a/src/auth/session.py │ │ +++ b/src/auth/session.py │ │ @@ -12,4 +12,10 @@ │ │ - import jwt │ │ + import sessionlib │ ╰──────────────────────────────────────────────────╯ ``` ### Actual Behavior `BuiltInSandboxStrategyAdapter.diff()` always returns an empty `DiffView` with no entries: ```python # src/cleveragents/infrastructure/sandbox/strategy_adapter.py def diff(self, ref: SandboxRef) -> DiffView: return DiffView( sandbox_id=ref.sandbox_id, entries=[], # Always empty! summary="Built-in strategy; diff computed at commit time.", ) ``` This means any code path that calls `adapter.diff()` to show sandbox changes will always receive an empty diff, even when the sandbox contains significant modifications. ### Code Location `src/cleveragents/infrastructure/sandbox/strategy_adapter.py` — `diff()` method. ### Root Cause The adapter has access to the underlying `Sandbox` instance via `self._sandboxes[ref.sandbox_id]`. The sandbox's `context.sandbox_path` and `context.original_path` are available. The `_fs_utils.compute_diff()` function already implements the diff logic used by `CopyOnWriteSandbox.commit()` and `OverlaySandbox.commit()`. The adapter simply needs to call `compute_diff(sandbox_path, original_path)` and build `DiffEntry` objects from the result. ### Impact - `agents plan diff` cannot show actual changes for plans using built-in sandbox strategies through the adapter - The `diff()` method is part of the `SandboxStrategyProtocol` and is expected to return meaningful data - Any tooling or UI that calls `diff()` to preview changes before apply will always show "no changes" ### Fix Implement `diff()` using the existing `compute_diff()` utility: ```python from cleveragents.infrastructure.sandbox._fs_utils import compute_diff def diff(self, ref: SandboxRef) -> DiffView: sandbox = self._sandboxes.get(ref.sandbox_id) if sandbox is None or sandbox.context is None: return DiffView(sandbox_id=ref.sandbox_id, entries=[], summary="No sandbox") ctx = sandbox.context if not os.path.isdir(ctx.sandbox_path) or not os.path.isdir(ctx.original_path): return DiffView(sandbox_id=ref.sandbox_id, entries=[], summary="Paths unavailable") changed, added, deleted = compute_diff(ctx.sandbox_path, ctx.original_path) entries = [ DiffEntry(path=p, operation="modified") for p in changed ] + [ DiffEntry(path=p, operation="added") for p in added ] + [ DiffEntry(path=p, operation="deleted") for p in deleted ] return DiffView(sandbox_id=ref.sandbox_id, entries=entries, summary=f"{len(entries)} changes") ``` Note: This fix applies to filesystem-based sandboxes. `TransactionSandbox` (database) and `NoSandbox` have no meaningful filesystem diff. ### Metadata - **Commit message**: `fix(sandbox): implement diff() in BuiltInSandboxStrategyAdapter` - **Branch**: `fix/sandbox-adapter-diff-implementation` - **Parent Epic**: #358 (Corrections + Subplans + Checkpoints M4) ### Subtasks - [ ] Implement `diff()` using `compute_diff()` for filesystem-based strategies - [ ] Handle `TransactionSandbox` and `NoSandbox` cases gracefully - [ ] Add unit tests for `diff()` with actual file changes ### Definition of Done - `BuiltInSandboxStrategyAdapter.diff()` returns non-empty `DiffView` when sandbox contains file changes - `DiffEntry` objects correctly classify files as `modified`, `added`, or `deleted` - `TransactionSandbox` and `NoSandbox` cases return empty diff with appropriate summary - Unit tests cover the diff computation path --- **Automated by CleverAgents Bot** Supervisor: UAT Testing | Agent: ca-uat-tester
freemo added this to the v3.7.0 milestone 2026-04-04 20:34:18 +00:00
Author
Owner

Issue triaged by project owner:

  • State: Verified
  • Priority: High — The sandbox diff() method returns empty results, making agents plan diff non-functional for built-in sandbox strategies. This is a functional bug, not just a cosmetic issue.
  • Milestone: v3.7.0
  • MoSCoW: Should have — Diff functionality is important for users to preview changes before apply. The fix is well-scoped (existing compute_diff() utility can be reused).
  • Parent Epic: #397 (Server & Autonomy Infrastructure) — sandbox infrastructure. Note: original parent #358 is closed.

Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: ca-project-owner

Issue triaged by project owner: - **State**: Verified - **Priority**: High — The sandbox diff() method returns empty results, making `agents plan diff` non-functional for built-in sandbox strategies. This is a functional bug, not just a cosmetic issue. - **Milestone**: v3.7.0 - **MoSCoW**: Should have — Diff functionality is important for users to preview changes before apply. The fix is well-scoped (existing `compute_diff()` utility can be reused). - **Parent Epic**: #397 (Server & Autonomy Infrastructure) — sandbox infrastructure. Note: original parent #358 is closed. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: ca-project-owner
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#397 Epic: Server & Autonomy Infrastructure
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core#2589
No description provided.