cleveragents/cleveragents-core

Fork 3

fix(sandbox): ensure atomic apply — commit_all must be all-or-nothing #925

New issue

Closed

opened 2026-03-14 00:11:19 +00:00 by freemo · 1 comment

freemo commented

2026-03-14 00:11:19 +00:00

Owner

Background

The specification (line 45844) requires: "The Apply phase is an atomic operation — either all sandbox changes are committed... or none are."

The current SandboxManager.commit_all() (infrastructure/sandbox/manager.py:202-247) performs partial commits — if one sandbox commit fails, it continues with others. The docstring explicitly says "partial commit is allowed."

This violates the spec's atomicity guarantee. A failed commit should roll back any already-committed sandboxes from the same batch.

Acceptance Criteria

commit_all() is atomic: either all sandboxes in the batch commit successfully, or none do
On partial failure, already-committed sandboxes are rolled back (or a pre-commit validation ensures all can succeed before committing any)
Error reporting clearly indicates which sandbox failed and what was rolled back
Existing tests pass; new tests verify atomicity guarantee

Metadata

Suggested commit message: fix(sandbox): make commit_all atomic per specification
Suggested branch name: fix/atomic-sandbox-commit

Definition of Done

Code merged to main, SandboxManager.commit_all() enforces all-or-nothing semantics.

## Background The specification (line 45844) requires: "The Apply phase is an atomic operation — either all sandbox changes are committed... or none are." The current `SandboxManager.commit_all()` (`infrastructure/sandbox/manager.py:202-247`) performs **partial commits** — if one sandbox commit fails, it continues with others. The docstring explicitly says "partial commit is allowed." This violates the spec's atomicity guarantee. A failed commit should roll back any already-committed sandboxes from the same batch. ## Acceptance Criteria - [ ] `commit_all()` is atomic: either all sandboxes in the batch commit successfully, or none do - [ ] On partial failure, already-committed sandboxes are rolled back (or a pre-commit validation ensures all can succeed before committing any) - [ ] Error reporting clearly indicates which sandbox failed and what was rolled back - [ ] Existing tests pass; new tests verify atomicity guarantee ## Metadata - **Suggested commit message:** `fix(sandbox): make commit_all atomic per specification` - **Suggested branch name:** `fix/atomic-sandbox-commit` ## Definition of Done Code merged to `main`, `SandboxManager.commit_all()` enforces all-or-nothing semantics.

freemo added the

labels

2026-03-14 00:11:58 +00:00

freemo added this to the v3.5.0 milestone

2026-03-14 00:12:06 +00:00

CoreRasurae was assigned by freemo

2026-03-14 04:27:11 +00:00

CoreRasurae referenced this issue from a commit

2026-03-24 03:39:19 +00:00

fix(sandbox): make commit_all atomic per specification

~~CoreRasurae referenced this issue 2026-03-24 03:39:44 +00:00~~

fix(sandbox): make commit_all atomic per specification #1146

CoreRasurae commented

2026-03-24 03:44:48 +00:00

Member

Implementation Notes

Summary

Changed SandboxManager.commit_all() from partial-commit semantics to all-or-nothing atomic operation per specification requirement. On partial failure, already-committed sandboxes are rolled back.

Files Modified (10 files)

Core: protocol.py (COMMITTED → ROLLED_BACK transition), manager.py (atomic commit_all + _rollback_committed helper)
Strategies: copy_on_write.py, git_worktree.py, overlay.py, transaction_sandbox.py — all updated to support rollback from COMMITTED state
Tests: sandbox_manager_coverage.feature (3 new atomic commit scenarios), sandbox_integration.robot (new integration test)

Design Decisions

Chose Rollback-on-Failure approach: Pre-validation cannot guarantee commit success (disk full, merge conflicts). Two-phase commit too invasive (requires protocol change). Rollback naturally extends existing architecture.
Per-strategy rollback:
- CoW/Overlay: Pre-commit backup directory using os.mkdir + raw file copy, restore on rollback
- Git Worktree: Records pre-merge HEAD commit, git reset --hard on rollback
- Transaction: Logs warning (DB COMMIT irreversible), transitions state for protocol consistency
Avoided test interference: Used os.mkdir (not os.makedirs) and raw byte I/O (not shutil.copy2) to avoid existing test patches.

Quality Gate Results

Stage	Result
lint	PASSED
typecheck	PASSED (0 errors)
unit_tests	PASSED (463 features, 12,294 scenarios)
integration_tests	PASSED (37 sandbox tests)
coverage	98% (exceeds 97% threshold)

PR

PR #1146 — fix/atomic-sandbox-commit → master
Commit: f23f0bc7

## Implementation Notes ### Summary Changed `SandboxManager.commit_all()` from partial-commit semantics to all-or-nothing atomic operation per specification requirement. On partial failure, already-committed sandboxes are rolled back. ### Files Modified (10 files) - **Core**: `protocol.py` (COMMITTED → ROLLED_BACK transition), `manager.py` (atomic `commit_all` + `_rollback_committed` helper) - **Strategies**: `copy_on_write.py`, `git_worktree.py`, `overlay.py`, `transaction_sandbox.py` — all updated to support rollback from COMMITTED state - **Tests**: `sandbox_manager_coverage.feature` (3 new atomic commit scenarios), `sandbox_integration.robot` (new integration test) ### Design Decisions 1. **Chose Rollback-on-Failure approach**: Pre-validation cannot guarantee commit success (disk full, merge conflicts). Two-phase commit too invasive (requires protocol change). Rollback naturally extends existing architecture. 2. **Per-strategy rollback**: - CoW/Overlay: Pre-commit backup directory using `os.mkdir` + raw file copy, restore on rollback - Git Worktree: Records pre-merge HEAD commit, `git reset --hard` on rollback - Transaction: Logs warning (DB COMMIT irreversible), transitions state for protocol consistency 3. **Avoided test interference**: Used `os.mkdir` (not `os.makedirs`) and raw byte I/O (not `shutil.copy2`) to avoid existing test patches. ### Quality Gate Results | Stage | Result | |---|---| | lint | PASSED | | typecheck | PASSED (0 errors) | | unit_tests | PASSED (463 features, 12,294 scenarios) | | integration_tests | PASSED (37 sandbox tests) | | coverage | **98%** (exceeds 97% threshold) | ### PR PR #1146 — `fix/atomic-sandbox-commit` → `master` Commit: `f23f0bc7`

CoreRasurae added

and removed

labels

2026-03-24 03:45:06 +00:00

CoreRasurae referenced this issue from a pull request that will close it,

2026-03-24 03:46:51 +00:00

fix(sandbox): make commit_all atomic per specification #1146

CoreRasurae referenced this issue