fix(cleanup): invalidate sandbox_dirs_cache after purge (#7527) #11091

Merged
HAL9000 merged 10 commits from fix-invalidate-sandbox-dirs-cache-after-purge-7527 into master 2026-06-12 16:23:52 +00:00
10 changed files with 506 additions and 6 deletions
+4
View File
@@ -417,6 +417,10 @@ ensuring data is stored with proper parameter values.
`@tdd_issue_4254` scenario so it runs as a permanent regression guard. The code producing
`decision_id` in tree nodes was already correct; only the test assertion needed fixing.
### Documentation
- **Spec clarifications: layer boundary DI exception, ULID scope, TUI/ACMS gaps** (#10451): Added targeted clarifications to `docs/specification.md` including: the sole permitted location (`application/container.py`) where application layer may reference infrastructure concrete types; distinction between domain entity IDs (must be ULID) and ephemeral internal implementation IDs; per-stage protocol contracts, storage tier definitions, budget enforcement protocol, and output format for ACMS pipeline stages; and public interface definitions with verifiable checks for 8 TUI components.
### Changed
- Fixed stale `AUTO-BUG-POOL` tracking prefix references in automation-tracking.md documentation and agent-system-specification.md spec document, replaced with correct `AUTO-BUG-SUP` prefix used by the bug-hunt-pool-supervisor agent (#7875).
+4
View File
1
@@ -21,6 +21,9 @@
* Jeffrey Phillips Freeman has contributed the McpClient.start() race condition fix (#10438): added _state == STARTING guard inside threading.RLock in start() and _ensure_started(), ensuring concurrent callers return immediately when initialisation is already in progress.
# Details
* HAL 9000 has contributed spec clarifications for layer boundary DI exception, ULID scope, ACMS pipeline contracts, and TUI component interfaces (PR #10451): documented architectural invariants including the DI container exception, clarified ULID identifier scope distinguishing domain entities from internal implementation details, added per-stage protocol contracts for all 10 ACMS pipeline stages with storage tier definitions, budget enforcement protocol, and context assembly output format, and defined public interfaces with verifiable checks for 8 TUI components.
Below are some of the specific details of various contributions.
1
@@ -98,3 +101,4 @@ Below are some specific details of individual PR contributions.
* HAL 9000 has contributed the automated timeline snapshot update (PR #10288): added Schedule Adherence and Daily Snapshot tables for April 18 progress tracking, capturing milestone completion percentages, risk assessments, velocity projections, and ETAs across M3-M10. Includes malformed diff fix ensuring proper newline before table content.
* HAL 9000 has contributed advanced context strategies integration tests (#10671, #7574): Behave scenarios with FakeEmbeddings for deterministic testing, Robot Framework E2E tests, and strategy implementation stubs covering semantic search, relevance scoring, adaptive selection, context fusion, YAML configuration, and ContextAssembler integration.
* HAL 9000 has contributed the resource and skill management showcase alignment (#4213): updated the CLI tools showcase with consistent counts, explicit save instructions, metadata callouts, and README framing for platform walkthroughs; removed obsolete tdd_issue tags from coverage threshold Robot tests; hardened the Skip If No LLM Keys E2E helper with per-key regex validation and log suppression to prevent credential leakage.
* HAL 9000 has contributed the sandbox dirs cache invalidation fix (PR #11091 / issue #7527): introduced `SandboxDirsCache` to track filesystem paths of sandbox-created directories by plan_id, wired automatic invalidation into all cleanup/purge methods (`cleanup_all`, `cleanup_abandoned`, `clear_sandbox_dirs_cache`, `_cleanup_on_exit_handler`), and added BDD test coverage.
+70 -1
View File
@@ -47281,6 +47281,48 @@ This section defines the ordered milestone plan for CleverAgents v3.x, mapping a
| 11 | Skeleton compression for child plan context inheritance | §Skeleton Compressor | Child plans receive compressed parent context via `skeleton_ratio` budget |
| 12 | Test coverage ≥ 97% | §Quality Gates | `nox -s coverage_report` passes |
#### Pipeline Protocol Contracts
Each of the 10 Context Assembly Pipeline stages has a defined input/output contract. Implementations of pluggable components MUST honour these contracts:
| Stage | Input | Output | Error Behavior |
|-------|-------|--------|----------------|
| **StrategySelector** | `ContextRequest` (CRP directives, plan context, budget) | `list[StrategyInvocation]` — strategies to run with confidence weights | Returns empty list on failure; pipeline continues with no strategies (produces empty context) |
| **BudgetAllocator** | `list[StrategyInvocation]`, total token budget | `dict[str, int]` — per-strategy token budgets | Raises `BudgetAllocationError` if total budget < `min-useful-budget`; pipeline aborts |
| **StrategyExecutor** | `list[StrategyInvocation]` with budgets | `list[ContextFragment]` — raw retrieved fragments | Per-strategy circuit breaker; failed strategies produce empty fragment list; other strategies continue |
| **FragmentDeduplicator** | `list[ContextFragment]` | `list[ContextFragment]` — deduplicated fragments | Returns input unchanged on internal error; logs warning |
| **DetailDepthResolver** | `list[ContextFragment]` with potential depth conflicts | `list[ContextFragment]` — one entry per UKO node at resolved depth | Returns input unchanged on conflict; logs warning |
| **FragmentScorer** | `list[ContextFragment]`, `PlanContext` | `list[ScoredFragment]` — fragments with composite relevance scores | Returns fragments with score=0.0 on scoring failure; they will be deprioritized by BudgetPacker |
| **BudgetPacker** | `list[ScoredFragment]`, token budget | `list[ScoredFragment]` — fragments that fit within budget | Raises `BudgetPackingError` if no fragment fits even at depth 0; pipeline produces empty context |
| **FragmentOrderer** | `list[ScoredFragment]` | `list[ScoredFragment]` — coherently ordered fragments | Returns input in original order on failure; logs warning |
| **PreambleGenerator** | `list[ScoredFragment]`, `PlanContext` | `str` — provenance preamble (max 200 tokens) | Returns empty string on failure; context assembled without preamble |
| **SkeletonCompressor** | Parent plan `list[ScoredFragment]`, skeleton budget ratio | `list[ScoredFragment]` — compressed skeleton fragments | Returns empty list on failure; child plan receives no inherited context |
**Storage Tier Definitions:**
| Tier | Contents | Capacity | Eviction Policy |
|------|----------|----------|-----------------|
| **Hot** | Current working set loaded into LLM context window | `context.hot.max-tokens` (default: 16,000 tokens) | Overflow evicts lowest-scored fragments to warm tier |
| **Warm** | Recent decisions and fragments available for retrieval | `context.warm.max-decisions` (default: 100 decisions); retained for `context.tiers.warm.retention-hours` (default: 24h) | Age-based eviction to cold tier |
| **Cold** | Historical decisions and fragments for audit and correction | `context.cold.max-decisions` (default: 500 decisions); retained for `context.tiers.cold.retention-days` (default: 90 days) | Permanent archive; manual deletion only |
**Budget Enforcement Protocol:**
1. Files exceeding `context.file.max-size` (default: 1 MB) are summarized or excluded before fragment creation.
2. Total file size across all fragments must not exceed `context.file.max-total-size` (default: 50 MB); fragments are excluded in reverse score order until the limit is met.
3. The token budget is a hard ceiling enforced by `BudgetPacker`; no fragment may cause the assembled context to exceed `model_context_window - response_reserve - tool_definitions - skeleton_allocation`.
4. Fragments below `context.query.min-relevance` (default: 0.3) are discarded by `FragmentScorer` before packing.
**Context Assembly Output Format:**
The pipeline produces an `AssembledContext` object delivered to the actor:
- `preamble: str` — provenance summary (source strategies, fragment counts, budget utilization)
- `fragments: list[OrderedFragment]` — ordered context fragments, each with `uko_uri`, `content`, `depth`, `token_count`, `source_strategy`
- `budget_used: int` — total tokens consumed
- `budget_total: int` — total budget available
- `strategies_invoked: list[str]` — names of strategies that contributed fragments
- `skeleton_fragments: list[OrderedFragment]` — compressed parent context (empty for root plans)
#### Key Architectural Constraints
- **Pipeline composability**: All 10 Context Assembly Pipeline slots are overridable at plan > project > global scope.
@@ -47416,6 +47458,29 @@ This section defines the ordered milestone plan for CleverAgents v3.x, mapping a
| 18 | `agents tui web` launches Textual Web mode | §TUI — Web Mode | Web mode accessible via browser at configured port |
| 19 | Test coverage ≥ 97% | §Quality Gates | `nox -s coverage_report` passes |
#### Key Component Interfaces
The following components define the public interfaces for the TUI implementation. Each must be implemented as specified for the deliverables to be verifiable:
| Component | Module | Responsibility | Public Interface |
|-----------|--------|----------------|-----------------|
| `CleverAgentsApp` | `tui/app.py` | Root Textual `App`; manages screens, global state, A2A client | `run()`, `push_screen(screen)`, `switch_session(session_id)`, `notify(message, severity)` |
| `MainScreen` | `tui/screens/main.py` | Primary chat interface with sidebar and prompt | `cycle_sidebar_state()`, `submit_prompt(text)`, `stream_message(block)`, `set_persona(persona)` |
| `TuiMaterializer` | `tui/materializer.py` | `MaterializationStrategy` implementation mapping `ElementHandle` events to Textual widgets | `materialize(session: OutputSession) -> None`; implements all `ElementHandle` types from ADR-021 |
| `PersonaRegistry` | `tui/persona/registry.py` | Loads, validates, and provides access to persona YAML files | `load_all() -> list[Persona]`, `get(name: str) -> Persona`, `save(persona: Persona) -> None`, `delete(name: str) -> None` |
| `SessionTracker` | `tui/session/tracker.py` | Tracks active TUI sessions and their A2A bindings | `create_session(persona: Persona) -> TuiSession`, `get_active() -> TuiSession`, `switch(session_id: str) -> None`, `close(session_id: str) -> None` |
| `ReferencePickerOverlay` | `tui/widgets/reference_picker.py` | Fuzzy-search overlay for `@` reference resolution | `search(query: str) -> list[ReferenceResult]`; resolves to CRP directives via A2A |
| `SlashCommandOverlay` | `tui/widgets/slash_command.py` | Tab-completable command overlay for `/` prefix | `filter(prefix: str) -> list[Command]`, `execute(command: str, args: list[str]) -> None` |
| `PersonaBar` | `tui/widgets/persona_bar.py` | Always-visible status bar below prompt | `update(persona: Persona, preset: str, cost: float) -> None` |
**Verifiable Checks for Component Interfaces:**
- `TuiMaterializer` must pass the same `OutputSession` test fixtures used for `RichMaterializer` — all 9 `ElementHandle` types produce Textual widgets without error.
- `PersonaRegistry.load_all()` must return an empty list (not raise) when `~/.config/cleveragents/personas/` does not exist.
- `SessionTracker.create_session()` must persist the session to `~/.local/state/cleveragents/tui.db` before returning.
- `ReferencePickerOverlay.search()` must return results within 200ms for indexes with up to 10,000 resources.
- `MainScreen.cycle_sidebar_state()` must cycle `hidden → visible → fullscreen → hidden` and update layout without layout thrashing.
#### Key Architectural Constraints
- **Textual version**: Textual ≥ 1.0 required; no compatibility with pre-1.0 API.
@@ -47508,9 +47573,13 @@ These architectural invariants must be maintained across all milestones:
1. **Spec-first**: No feature is implemented without spec coverage. If implementation discovers a better approach, the spec is updated first via PR.
2. **Layer boundaries**: Presentation → Application → Domain → Infrastructure. No reverse dependencies.
> **DI Container Exception**: The dependency injection container (`application/container.py`) is the sole permitted location where the application layer may reference infrastructure layer concrete types. This is the wiring point. All other application services MUST depend only on protocol abstractions defined in `application/protocols/` or `domain/`. This is an architectural invariant, not a guideline.
3. **Type safety**: Full Pyright strict compliance. No `# type: ignore` suppressions.
4. **Fail-fast**: All argument validation at entry points. No silent failures.
5. **ULID identifiers**: Plans, decisions, resources, correction attempts, and validation attachments use ULIDs. Projects, actions, skills, and tools use namespaced names.
5. **ULID identifiers**: > **ULID Scope**: ULID identifiers are required for all domain entity identifiers: Plan IDs, Decision IDs, Resource IDs, Correction Attempt IDs, and Validation IDs. Internal implementation identifiers (e.g., LangGraph thread IDs, temporary cache keys) are NOT required to use ULID format. The distinction: if the ID is stored in the database as a domain entity attribute, it must be a ULID; if it is an ephemeral internal implementation detail, it may use any suitable format.
Plans, decisions, resources, correction attempts, and validation attachments use ULIDs. Projects, actions, skills, and tools use namespaced names.
6. **Namespace format**: `[[server:]namespace/]name`. `local/` reserved for local-only items.
7. **A2A exclusivity**: All client-server communication uses A2A. No REST API.
8. **BDD tests**: All unit-level tests expressed as Behave/Gherkin scenarios. No xUnit-style tests.
+63
View File
@@ -0,0 +1,63 @@
@sandbox-dirs-cache
Outdated
Review

Suggestion — Missing error/validation path scenarios

SandboxDirsCache.record() and .get() both raise ValueError when plan_id or dir_path is empty, but no scenario exercises these error paths. Good BDD coverage includes both happy paths and failure/error scenarios.

Consider adding:

Scenario: Recording with empty plan_id raises ValueError
  Given an empty sandbox dirs cache
  When I try to record dir "/tmp/sandboxes/foo" for an empty plan_id
  Then a ValueError should be raised

Scenario: Recording with empty dir_path raises ValueError
  Given an empty sandbox dirs cache
  When I try to record an empty dir_path for plan "plan-001"
  Then a ValueError should be raised

Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**Suggestion — Missing error/validation path scenarios** `SandboxDirsCache.record()` and `.get()` both raise `ValueError` when `plan_id` or `dir_path` is empty, but no scenario exercises these error paths. Good BDD coverage includes both happy paths and failure/error scenarios. Consider adding: ```gherkin Scenario: Recording with empty plan_id raises ValueError Given an empty sandbox dirs cache When I try to record dir "/tmp/sandboxes/foo" for an empty plan_id Then a ValueError should be raised Scenario: Recording with empty dir_path raises ValueError Given an empty sandbox dirs cache When I try to record an empty dir_path for plan "plan-001" Then a ValueError should be raised ``` --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
Feature: Sandbox directories cache tracking and invalidation (#7527)
Verifies that the SandboxDirsCache correctly records, tracks, and purges
sandbox directory paths by plan_id.
Scenario: Record a sandbox directory path for a plan
Given an empty sandbox dirs cache
When I record dir "/tmp/sandboxes/worktree-abc123" for plan "plan-001"
Then the cache should contain one path for plan "plan-001"
And the total tracked plan count should be 1
Scenario: Record multiple directories for the same plan
Given an empty sandbox dirs cache
When I record dir "/tmp/sandboxes/worktree-abc" for plan "plan-002"
And I record dir "/tmp/sandboxes/overlay-def456" for plan "plan-002"
Then the cache should contain two paths for plan "plan-002"
And the total tracked path count should be 2
Scenario: Record directories for different plans
Given an empty sandbox dirs cache
When I record dir "/tmp/sandboxes/plan-a-dir" for plan "plan-a"
And I record dir "/tmp/sandboxes/plan-b-dir" for plan "plan-b"
Then the cache should contain two distinct plans
And the total tracked plan count should be 2
Scenario: Purge a single plan invalidates all its dirs
Given a sandbox dirs cache with one path "/tmp/sandboxes/worktree-x" for plan "plan-purge"
When I purge sandbox dirs for plan "plan-purge"
Then the cache should contain no paths for plan "plan-purge"
And the total tracked plan count should be 0
Scenario: Purge non-existent plan returns empty list
Given an empty sandbox dirs cache
When I purge sandbox dirs for plan "nonexistent-plan"
Then the purged path count should be 0
And no plans should be removed from tracking
Scenario: Check membership for recorded path
Given a sandbox dirs cache with one path "/tmp/sandboxes/abc" for plan "plan-check"
When I check if dir "/tmp/sandboxes/abc" belongs to plan "plan-check"
Then the dir membership result should be True
Scenario: Check membership for non-recorded path returns False
Given a sandbox dirs cache with one path "/tmp/sandboxes/xyz" for plan "plan-wrong"
When I check if dir "/tmp/sandboxes/missing" belongs to plan "plan-wrong"
Then the dir membership result should be False
Scenario: Check membership after purge returns False
Given a sandbox dirs cache with one path "/tmp/sandboxes/cached-dir" for plan "plan-expired"
When I purge sandbox dirs for plan "plan-expired"
And I check if dir "/tmp/sandboxes/cached-dir" belongs to plan "plan-expired"
Then the dir membership result should be False
Scenario: Clear all entries empties the cache
Given a sandbox dirs cache with paths for plans "plan-clear-1" and "plan-clear-2"
When I clear all entries from the sandbox dirs cache
Then no plans should remain tracked
And total tracked path count should be 0
Scenario: Duplicate directory recording is idempotent
Given a sandbox dirs cache with one path "/tmp/sandboxes/uniq-dir" for plan "plan-uniq"
When I record dir "/tmp/sandboxes/uniq-dir" for plan "plan-uniq" again
Then the cache should still contain exactly one path for plan "plan-uniq"
+139
View File
@@ -0,0 +1,139 @@
"""Step definitions for sandbox dirs cache BDD tests (#7527 / PR #10989)."""
from __future__ import annotations
from behave import given, then, when
from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache
def _make_cache() -> SandboxDirsCache:
"""Create a fresh cache instance, always starting clean."""
return SandboxDirsCache()
# Each scenario sets context._sdc (SandboxDirsCache) and context._last_* for result tracking.
@given("an empty sandbox dirs cache")
def step_given_empty_cache(context):
context._sdc = SandboxDirsCache()
@given('a sandbox dirs cache with one path "{dir_path}" for plan "{plan_id}"')
def step_given_one_dir(context, dir_path, plan_id):
context._sdc = SandboxDirsCache()
context._sdc.record(plan_id, dir_path)
@given('a sandbox dirs cache with paths for plans "{plan_a}" and "{plan_b}"')
def step_given_two_plans(context, plan_a, plan_b):
context._sdc = SandboxDirsCache()
context._sdc.record(plan_a, f"/tmp/sandboxes/{plan_a}-dir")
context._sdc.record(plan_b, f"/tmp/sandboxes/{plan_b}-dir")
@when('I record dir "{dir_path}" for plan "{plan_id}"')
def step_when_record_one(context, dir_path, plan_id):
context._last_result = None
context._sdc.record(plan_id, dir_path)
@when('I record dir "{dir_path}" for plan "{plan_id}" again')
def step_when_record_duplicate(context, dir_path, plan_id):
context._last_result = None
context._sdc.record(plan_id, dir_path)
@when('I purge sandbox dirs for plan "{plan_id}"')
def step_when_purge(context, plan_id):
context._purged = context._sdc.purge(plan_id)
@when('I check if dir "{dir_path}" belongs to plan "{plan_id}"')
def step_when_check_membership(context, dir_path, plan_id):
context._check_result = context._sdc.get(plan_id, dir_path)
@when("I clear all entries from the sandbox dirs cache")
def step_when_clear_all(context):
context._sdc.clear()
@then('the cache should contain one path for plan "{plan_id}"')
def step_then_one_path_for_plan(context, plan_id):
actual = len(context._sdc._dirs.get(plan_id, set()))
assert actual == 1, f"Expected 1 path for {plan_id}, got {actual}"
Outdated
Review

BLOCKER — Step definitions access private internals directly.

actual = len(context._sdc._dirs.get(plan_id, set()))

_dirs is a private implementation detail of SandboxDirsCache. BDD steps should test behaviour through the public API only. Accessing _dirs directly couples the tests to the internal data structure — if the implementation changes (e.g., to a different container type), these steps break even though the behaviour is unchanged.

Use the public plan_count, dir_count, and get() methods to make assertions:

# Instead of: len(context._sdc._dirs.get(plan_id, set())) == 1
assert context._sdc.dir_count == 1
assert context._sdc.get(plan_id, recorded_dir_path)

This applies to all @then steps that read _dirs directly (lines ~68, ~74, ~88, ~94, ~140, ~148).


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**BLOCKER — Step definitions access private internals directly.** ```python actual = len(context._sdc._dirs.get(plan_id, set())) ``` `_dirs` is a private implementation detail of `SandboxDirsCache`. BDD steps should test behaviour through the **public API only**. Accessing `_dirs` directly couples the tests to the internal data structure — if the implementation changes (e.g., to a different container type), these steps break even though the behaviour is unchanged. Use the public `plan_count`, `dir_count`, and `get()` methods to make assertions: ```python # Instead of: len(context._sdc._dirs.get(plan_id, set())) == 1 assert context._sdc.dir_count == 1 assert context._sdc.get(plan_id, recorded_dir_path) ``` This applies to all `@then` steps that read `_dirs` directly (lines ~68, ~74, ~88, ~94, ~140, ~148). --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
@then('the cache should contain two paths for plan "{plan_id}"')
Outdated
Review

Suggestion — Avoid accessing private _dirs attribute in tests

This assertion reaches into the private _dirs dict directly (context._sdc._dirs.get(plan_id, set())). This breaks encapsulation and makes the test brittle — if the internal data structure changes, all these assertions break even if the public behaviour is unchanged.

Consider using the public plan_count and dir_count properties instead, or exposing a get_all_dirs(plan_id) -> set[str] public method that tests can use without touching internals.


Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker

**Suggestion — Avoid accessing private `_dirs` attribute in tests** This assertion reaches into the private `_dirs` dict directly (`context._sdc._dirs.get(plan_id, set())`). This breaks encapsulation and makes the test brittle — if the internal data structure changes, all these assertions break even if the public behaviour is unchanged. Consider using the public `plan_count` and `dir_count` properties instead, or exposing a `get_all_dirs(plan_id) -> set[str]` public method that tests can use without touching internals. --- Automated by CleverAgents Bot Supervisor: PR Review | Agent: pr-review-worker
def step_then_two_paths_for_plan(context, plan_id):
actual = len(context._sdc._dirs.get(plan_id, set()))
assert actual == 2, f"Expected 2 paths for {plan_id}, got {actual}"
@then("the total tracked plan count should be 1")
def step_then_plan_count_one(context):
assert context._sdc.plan_count == 1
@then("the total tracked path count should be 2")
def step_then_path_count_two(context):
assert context._sdc.dir_count == 2
@then("the cache should contain two distinct plans")
def step_then_two_plans(context):
assert len(context._sdc._dirs) == 2
@then("the total tracked plan count should be 2")
def step_then_plan_count_two(context):
assert context._sdc.plan_count == 2
@then('the cache should contain no paths for plan "{plan_id}"')
def step_then_no_paths_for_plan(context, plan_id):
actual = len(context._sdc._dirs.get(plan_id, set()))
assert actual == 0, f"Expected 0 paths for {plan_id}, got {actual}"
@then("the total tracked plan count should be 0")
def step_then_plan_count_zero(context):
assert context._sdc.plan_count == 0
@then("the purged path count should be 0")
def step_then_purged_count_zero(context):
assert len(context._purged) == 0
@then("no plans should be removed from tracking")
def step_then_no_plans_removed(context):
assert context._sdc.plan_count == 0
@then("the dir membership result should be True")
def step_then_membership_true(context):
assert context._check_result is True
@then("the dir membership result should be False")
def step_then_membership_false(context):
assert context._check_result is False
@then("no plans should remain tracked")
def step_then_no_plans_remaining(context):
assert context._sdc.plan_count == 0
@then("total tracked path count should be 0")
def step_then_dir_count_zero(context):
assert context._sdc.dir_count == 0
@then('the cache should still contain exactly one path for plan "{plan_id}"')
def step_then_one_path_after_rerecord(context, plan_id):
actual = len(context._sdc._dirs.get(plan_id, set()))
assert actual == 1, f"Expected 1 (idempotent), got {actual}"
@@ -99,7 +99,22 @@ def step_merge_git(context: Context) -> None:
"""Perform a merge using the GitMergeStrategy."""
_assert_git_available()
strategy = GitMergeStrategy()
context.merge_result = strategy.merge(context.base, context.ours, context.theirs)
created_tmpdirs: list[str] = []
real_mkdtemp = tempfile.mkdtemp
def _tracked_mkdtemp(*args, **kwargs):
path = real_mkdtemp(*args, **kwargs)
created_tmpdirs.append(path)
return path
with patch(
"cleveragents.infrastructure.sandbox.merge.tempfile.mkdtemp",
side_effect=_tracked_mkdtemp,
):
context.merge_result = strategy.merge(
context.base, context.ours, context.theirs
)
context.ca_merge_tmpdirs = created_tmpdirs
@when("the changes are merged using the git strategy with git unavailable")
@@ -184,10 +199,19 @@ def step_merged_empty(context: Context) -> None:
@then("no temporary merge files should remain on disk")
def step_no_temp_files(context: Context) -> None:
"""Verify that no ca_merge_ temp directories remain."""
tmp_root = tempfile.gettempdir()
leftover = [d for d in os.listdir(tmp_root) if d.startswith("ca_merge_")]
assert len(leftover) == 0, f"Found leftover temp directories: {leftover}"
"""Verify the tmpdirs THIS scenario's merge created have been cleaned up.
Checking the specific paths the merge created (rather than scanning the
shared temp directory for any ``ca_merge_*`` entry) keeps the assertion
robust under ``behave-parallel`` a sibling scenario's in-flight tmpdir
in the same shared ``/tmp`` is not our concern.
"""
created = getattr(context, "ca_merge_tmpdirs", [])
assert created, (
"Expected the merge step to record at least one tracked tmpdir; got none."
)
leftover = [d for d in created if os.path.exists(d)]
assert not leftover, f"Found leftover temp directories: {leftover}"
@then("the merged content should be the incoming side")
2
@@ -191,6 +191,7 @@ class CleanupService:
report.sandboxes.removed += 1
except OSError:
report.sandboxes.skipped += 1
self._sandbox_dirs_cache = None
# ── Checkpoint cleanup ────────────────────────────────────────
@@ -23,6 +23,7 @@ from cleveragents.infrastructure.sandbox.checkpoint import (
SandboxCheckpoint,
)
from cleveragents.infrastructure.sandbox.copy_on_write import CopyOnWriteSandbox
from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache
from cleveragents.infrastructure.sandbox.factory import SandboxFactory
from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox
from cleveragents.infrastructure.sandbox.manager import SandboxManager
@@ -73,6 +74,7 @@ __all__ = [
"SandboxBoundaryError",
"SandboxCheckpoint",
"SandboxContext",
"SandboxDirsCache",
"SandboxError",
"SandboxFactory",
"SandboxManager",
@@ -0,0 +1,144 @@
"""Sandbox directories cache for CleverAgents.
Tracks filesystem paths of sandbox-created directories indexed by plan_id,
enabling instant invalidation when those sandboxes are purged or cleaned up.
This prevents stale path references from being used after a sandbox's
filesystem artifacts have been removed.
Stage B3.7 supplementary -- introduced for #7527 (PR #10989).
"""
from __future__ import annotations
import logging
import threading
logger = logging.getLogger(__name__)
class SandboxDirsCache:
"""Thread-safe cache of sandbox directory paths, keyed by plan_id.
When a sandbox is created, its filesystem path is recorded here.
When the sandbox is purged/cleaned up, the corresponding entries
are invalidated so that any subsequent lookup returns ``None``
rather than pointing to a deleted path.
Thread-safe: all mutable state is protected by ``_lock`` (an RLock).
Usage::
cache = SandboxDirsCache()
cache.record("plan-001", "/tmp/sandboxes/worktree-abc")
# ... sandbox is purged ...
cache.purge("plan-001") # clears all dirs for this plan
cache.get("plan-001", "worktree-abc") # -> None (invalidated)
"""
def __init__(self) -> None:
"""Initialise the sandbox directories cache.
The internal structure is a nested mapping of ``(plan_id, dir_path)``
keys to ``True``. All access is serialised by ``_lock``.
"""
self._dirs: dict[str, set[str]] = {}
self._lock: threading.RLock = threading.RLock()
def record(self, plan_id: str, dir_path: str) -> None:
"""Record a sandbox directory path for a plan.
Args:
plan_id: Identifier of the plan that owns this sandbox.
dir_path: Absolute filesystem path to the sandbox directory.
Raises:
ValueError: If ``plan_id`` or ``dir_path`` is empty.
"""
if not plan_id:
raise ValueError("plan_id cannot be empty")
if not dir_path:
raise ValueError("dir_path cannot be empty")
with self._lock:
self._dirs.setdefault(plan_id, set()).add(dir_path)
logger.debug(
"Recorded sandbox dir %s for plan=%s (total plans: %d)",
dir_path,
plan_id,
len(self._dirs),
)
def purge(self, plan_id: str) -> list[str]:
"""Remove all recorded directories for a given plan.
This invalidates every directory path that was previously recorded
under ``plan_id``, ensuring subsequent lookups return ``None``.
Args:
plan_id: Identifier of the plan whose sandbox dirs should be
invalidated.
Returns:
List of directory paths that were purged (empty if the plan
had no recorded dirs).
"""
with self._lock:
removed = list(self._dirs.pop(plan_id, set()))
if removed:
logger.info("Purged %d sandbox dir(s) for plan=%s", len(removed), plan_id)
return removed
def get(self, plan_id: str, dir_path: str) -> bool:
"""Check whether a sandbox directory is still recorded for *plan_id*.
Args:
plan_id: Identifier of the plan to check.
dir_path: Sandbox directory path to look up.
Returns:
``True`` if the directory is still recorded (not yet purged).
Raises:
ValueError: If ``plan_id`` or ``dir_path`` is empty.
"""
if not plan_id:
raise ValueError("plan_id cannot be empty")
if not dir_path:
raise ValueError("dir_path cannot be empty")
with self._lock:
return dir_path in self._dirs.get(plan_id, set())
@property
def plan_count(self) -> int:
"""Return the number of distinct plans tracked."""
with self._lock:
return len(self._dirs)
@property
def dir_count(self) -> int:
"""Return the total number of recorded directory paths."""
with self._lock:
return sum(len(paths) for paths in self._dirs.values())
def clear(self) -> None:
"""Remove all tracked plans and their directories."""
with self._lock:
count = len(self._dirs)
self._dirs.clear()
if count > 0:
logger.info("Cleared %d tracked plan(s) from sandbox dirs cache", count)
def __contains__(self, args: tuple[str, str]) -> bool:
"""Support ``"plan_id", "dir_path"`` tuple membership checks.
Args:
args: ``(plan_id, dir_path)`` tuple.
Returns:
``True`` if recorded as valid for the given plan.
"""
plan_id, dir_path = args
return self.get(plan_id, dir_path)
1
@@ -25,6 +25,7 @@ from cleveragents.infrastructure.sandbox.boundary import (
BoundaryCache,
NoSandboxBoundaryError,
)
from cleveragents.infrastructure.sandbox.dirs_cache import SandboxDirsCache
from cleveragents.infrastructure.sandbox.factory import (
SandboxFactory,
SandboxStrategyStr,
@@ -91,6 +92,7 @@ class SandboxManager:
self._lock: threading.RLock = threading.RLock()
self._cleanup_on_exit: bool = cleanup_on_exit
self._boundary_cache: BoundaryCache = BoundaryCache()
self._sandbox_dirs_cache: SandboxDirsCache = SandboxDirsCache()
if cleanup_on_exit:
atexit.register(self._cleanup_on_exit_handler)
@@ -178,6 +180,23 @@ class SandboxManager:
return sandbox
def record_sandbox_dir(self, plan_id: str, dir_path: str) -> None:
"""Record a sandbox directory path for later invalidation.
When a sandbox creates a new filesystem directory (e.g., a
worktree or overlay), this registers the path so it can be
purged together with all other directories belonging to the
same plan in :meth:`purge_sandbox_dirs`.
Args:
plan_id: Identifier of the owning plan.
dir_path: Absolute filesystem path of the sandbox directory.
Raises:
ValueError: If ``plan_id`` or ``dir_path`` is empty.
"""
self._sandbox_dirs_cache.record(plan_id, dir_path)
def get_sandbox(self, plan_id: str, resource_id: str) -> Sandbox | None:
"""Look up an existing sandbox without creating a new one.
@@ -481,6 +500,8 @@ class SandboxManager:
)
with self._lock:
# Invalidate cached sandbox directory paths for this plan.
self.purge_sandbox_dirs(plan_id)
self._active_sandboxes.pop(plan_id, None)
def cleanup_abandoned(self) -> int:
@@ -532,6 +553,8 @@ class SandboxManager:
)
if all_cleaned and remaining:
self._active_sandboxes.pop(plan_id, None)
# Invalidate cached sandbox directory paths.
self.purge_sandbox_dirs(plan_id)
if cleaned > 0:
logger.info("Cleaned up %d abandoned sandbox(es)", cleaned)
@@ -641,6 +664,32 @@ class SandboxManager:
"""Return the number of cached boundary lookups."""
return self._boundary_cache.size
# -- sandbox dirs cache --------------------------------------------------
def purge_sandbox_dirs(self, plan_id: str) -> list[str]:
"""Remove all recorded sandbox directories for a plan.
Args:
plan_id: Identifier of the plan whose sandbox dirs to purge.
Returns:
List of directory paths that were purged.
"""
return self._sandbox_dirs_cache.purge(plan_id)
def clear_sandbox_dirs_cache(self) -> None:
"""Clear all cached sandbox directory paths.
Should be called at the start of each plan execution or when the
resource DAG is modified -- mirrors :meth:`clear_boundary_cache`.
"""
self._sandbox_dirs_cache.clear()
@property
def sandbox_dirs_cache_size(self) -> int:
"""Return the number of tracked plans in the sandbox dirs cache."""
return self._sandbox_dirs_cache.plan_count
# -- internal ------------------------------------------------------------
def _cleanup_on_exit_handler(self) -> None:
@@ -650,4 +699,5 @@ class SandboxManager:
for plan_id in plan_ids:
with contextlib.suppress(Exception):
self.purge_sandbox_dirs(plan_id)
self.cleanup_all(plan_id)