diff --git a/CHANGELOG.md b/CHANGELOG.md index 82b3d30f..837dca07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -71,6 +71,16 @@ when API keys are absent, and `--exclude E2E` on the standard integration test session. Includes a minimal smoke test exercising `agents --version` and `agents --help`. (#740) +- Implemented `tdd_expected_fail` tag handling in Robot Framework via a Listener v3 + module (`robot/tdd_expected_fail_listener.py`). Tests tagged `tdd_expected_fail` + that fail have their result inverted to pass (expected failure); tests that + unexpectedly pass are reported as failed with guidance to remove the tag. Tag + validation enforces `tdd_bug` + `tdd_bug_` prerequisites. Includes + idempotency guard against double-invocation, explicit SKIP status handling, + and a `close()` hook for clean teardown. Listener is registered in the nox + `integration_tests` and `slow_integration_tests` sessions. Fixture files are + excluded from the main pabot runner via `tdd_fixture` tag. Includes 9 Robot + Framework integration test cases. (#628) - Added TDD-style failing Behave BDD tests for the session list DI container missing `db` provider bug. Three scenarios exercise `session list`, `_get_session_service()`, and `session list --format json` through the real diff --git a/benchmarks/bench_sandbox_strategy.py b/benchmarks/bench_sandbox_strategy.py new file mode 100644 index 00000000..b6f47edd --- /dev/null +++ b/benchmarks/bench_sandbox_strategy.py @@ -0,0 +1,242 @@ +"""ASV benchmarks for custom sandbox strategy registration. + +Measures the performance of: +- SandboxRef creation +- DiffView / DiffEntry model creation +- SandboxStrategyRegistry registration and lookup +- BuiltInSandboxStrategyAdapter create/write/read cycle +- Protocol runtime checking via isinstance +""" + +from __future__ import annotations + +import importlib +import os +import sys +import tempfile +from datetime import datetime +from pathlib import Path + +# Ensure the local *source* tree is importable even when ASV has an +# older build of the package installed. +_SRC = str(Path(__file__).resolve().parents[1] / "src") +if _SRC not in sys.path: + sys.path.insert(0, _SRC) + +# Force-reload so ASV picks up the source tree version. +import cleveragents # noqa: E402 + +importlib.reload(cleveragents) + +from cleveragents.domain.models.core.resource import ( # noqa: E402 + PhysVirt, + Resource, + ResourceCapabilities, + SandboxStrategy as SandboxStrategyEnum, +) +from cleveragents.domain.models.core.sandbox_strategy import ( # noqa: E402 + DiffEntry, + DiffView, + SandboxRef, + SandboxStrategyProtocol, +) +from cleveragents.infrastructure.sandbox.strategy_adapter import ( # noqa: E402 + BuiltInSandboxStrategyAdapter, +) +from cleveragents.infrastructure.sandbox.strategy_registry import ( # noqa: E402 + SandboxStrategyRegistry, +) + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +_COUNTER = 0 + + +def _ulid(name: str) -> str: + global _COUNTER + _COUNTER += 1 + base = abs(hash(name)) % (10**20) + raw = f"{_COUNTER:06d}{base:020d}" + charset = "0123456789ABCDEFGHJKMNPQRSTVWXYZ" + result = "" + for ch in raw: + result += charset[int(ch)] + return (result + "0" * 26)[:26] + + +class _MockStrategy: + """Minimal strategy satisfying the Protocol for benchmarking.""" + + def create(self, plan_id, resource): # noqa: ANN001, ANN201 + return None + + def read(self, ref, path): # noqa: ANN001, ANN201 + return b"" + + def write(self, ref, path, content): # noqa: ANN001, ANN201 + return None + + def diff(self, ref): # noqa: ANN001, ANN201 + return None + + def commit(self, ref): # noqa: ANN001, ANN201 + pass + + def rollback(self, ref): # noqa: ANN001, ANN201 + pass + + def checkpoint(self, ref, checkpoint_id): # noqa: ANN001, ANN201 + pass + + def restore_checkpoint(self, ref, checkpoint_id): # noqa: ANN001, ANN201 + pass + + def cleanup(self, ref): # noqa: ANN001, ANN201 + pass + + +# --------------------------------------------------------------------------- +# SandboxRef benchmarks +# --------------------------------------------------------------------------- + + +class TimeSandboxRef: + """Benchmark SandboxRef creation.""" + + def time_create_ref(self) -> None: + SandboxRef( + sandbox_id="bench-ref", + plan_id="bench-plan", + resource_id="bench-res", + created_at=datetime.now(), + ) + + def time_create_ref_with_metadata(self) -> None: + SandboxRef( + sandbox_id="bench-ref-meta", + plan_id="bench-plan", + resource_id="bench-res", + created_at=datetime.now(), + metadata={"key": "value", "num": 42}, + ) + + +# --------------------------------------------------------------------------- +# DiffEntry / DiffView benchmarks +# --------------------------------------------------------------------------- + + +class TimeDiffModels: + """Benchmark DiffEntry and DiffView creation.""" + + def time_create_diff_entry(self) -> None: + DiffEntry(path="bench.txt", operation="modified") + + def time_create_diff_entry_with_content(self) -> None: + DiffEntry( + path="bench.txt", + operation="modified", + before=b"old", + after=b"new", + ) + + def time_create_diff_view_10_entries(self) -> None: + entries = [ + DiffEntry(path=f"file{i}.txt", operation="modified") for i in range(10) + ] + DiffView(sandbox_id="bench-dv", entries=entries) + + +# --------------------------------------------------------------------------- +# Protocol checking benchmarks +# --------------------------------------------------------------------------- + + +class TimeProtocolCheck: + """Benchmark SandboxStrategyProtocol isinstance checks.""" + + def setup(self) -> None: + self.good = _MockStrategy() + + def time_isinstance_check_pass(self) -> None: + isinstance(self.good, SandboxStrategyProtocol) + + def time_isinstance_check_fail(self) -> None: + isinstance("not a strategy", SandboxStrategyProtocol) + + +# --------------------------------------------------------------------------- +# SandboxStrategyRegistry benchmarks +# --------------------------------------------------------------------------- + + +class TimeStrategyRegistry: + """Benchmark registry operations.""" + + def setup(self) -> None: + self.registry = SandboxStrategyRegistry( + allowed_prefixes=("cleveragents.",), + ) + self.registry.register( + "bench_adapter", + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + + def time_lookup_hit(self) -> None: + self.registry.get("bench_adapter") + + def time_lookup_miss(self) -> None: + self.registry.get("nonexistent") + + def time_has_check(self) -> None: + self.registry.has("bench_adapter") + + def time_list_strategies(self) -> None: + self.registry.list_strategies() + + +# --------------------------------------------------------------------------- +# BuiltInSandboxStrategyAdapter benchmarks +# --------------------------------------------------------------------------- + + +class TimeAdapterLifecycle: + """Benchmark adapter create/write/read cycle.""" + + def setup(self) -> None: + self.tmpdir = tempfile.mkdtemp(prefix="ca-bench-adapter-") + with open(os.path.join(self.tmpdir, "seed.txt"), "w") as f: + f.write("seed") + self.adapter = BuiltInSandboxStrategyAdapter(strategy_name="copy_on_write") + self.resource = Resource( + resource_id=_ulid("bench-adapter"), + name="bench-adapter", + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.COPY_ON_WRITE, + location=self.tmpdir, + capabilities=ResourceCapabilities( + readable=True, + writable=True, + sandboxable=True, + checkpointable=False, + ), + ) + + def time_create_and_cleanup(self) -> None: + ref = self.adapter.create("bench-plan", self.resource) + self.adapter.cleanup(ref) + + def time_write_read_cycle(self) -> None: + ref = self.adapter.create("bench-plan-wr", self.resource) + self.adapter.write(ref, "bench.txt", b"bench content") + self.adapter.read(ref, "bench.txt") + self.adapter.cleanup(ref) + + def teardown(self) -> None: + import shutil + + shutil.rmtree(self.tmpdir, ignore_errors=True) diff --git a/docs/development/custom_sandbox_strategy.md b/docs/development/custom_sandbox_strategy.md new file mode 100644 index 00000000..57be4f92 --- /dev/null +++ b/docs/development/custom_sandbox_strategy.md @@ -0,0 +1,150 @@ +# Custom Sandbox Strategy Registration + +## Overview + +CleverAgents supports custom sandbox strategies via the `SandboxStrategyProtocol`. +Custom strategies enable domain-specific isolation for specialized resource types +that are not covered by the built-in strategies (none, git\_worktree, copy\_on\_write, +transaction\_rollback). + +## SandboxStrategy Protocol + +All custom sandbox strategies must implement the 9-method `SandboxStrategyProtocol`: + +```python +from cleveragents.domain.models.core.sandbox_strategy import ( + SandboxStrategyProtocol, + SandboxRef, + DiffEntry, + DiffView, +) +from cleveragents.domain.models.core.resource import Resource + + +class MyCustomSandbox: + 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: ... +``` + +## Supporting Models + +### SandboxRef + +Opaque reference returned by `create()` and passed to all other methods: + +```python +@dataclass(frozen=True) +class SandboxRef: + sandbox_id: str + plan_id: str + resource_id: str + created_at: datetime + metadata: dict[str, object] = field(default_factory=dict) +``` + +### DiffEntry / DiffView + +`DiffEntry` describes a single file change; `DiffView` aggregates entries: + +```python +class DiffEntry(BaseModel): + path: str # relative file path + operation: str # "added", "modified", or "deleted" + before: bytes | None + after: bytes | None + +class DiffView(BaseModel): + sandbox_id: str + entries: list[DiffEntry] + summary: str +``` + +## Config-Driven Registration + +Custom strategies are registered via the `SandboxStrategyRegistry`: + +```python +from cleveragents.infrastructure.sandbox.strategy_registry import ( + SandboxStrategyRegistry, +) + +registry = SandboxStrategyRegistry( + allowed_prefixes=("cleveragents.", "mycompany."), +) + +# Single registration +registry.register( + "my_strategy", + "mycompany.sandbox.custom", + "MyCustomSandbox", +) + +# Batch config-driven registration +configs = { + "redis_sandbox": { + "module": "mycompany.sandbox.redis", + "class": "RedisSandbox", + }, + "s3_sandbox": { + "module": "mycompany.sandbox.s3", + "class": "S3Sandbox", + }, +} +registry.register_all_from_config(configs) +``` + +## Factory Integration + +The `SandboxFactory` can be initialized with a custom registry: + +```python +from cleveragents.infrastructure.sandbox.factory import SandboxFactory + +factory = SandboxFactory(custom_registry=registry) + +# Check if custom strategy is available +if factory.has_custom_strategy("redis_sandbox"): + cls = factory.get_custom_strategy_class("redis_sandbox") +``` + +## Built-In Strategy Adapter + +The `BuiltInSandboxStrategyAdapter` wraps existing `Sandbox` implementations +to conform to the `SandboxStrategyProtocol`: + +```python +from cleveragents.infrastructure.sandbox.strategy_adapter import ( + BuiltInSandboxStrategyAdapter, +) + +adapter = BuiltInSandboxStrategyAdapter(strategy_name="copy_on_write") +ref = adapter.create("plan-001", resource) +adapter.write(ref, "file.txt", b"content") +data = adapter.read(ref, "file.txt") +adapter.cleanup(ref) +``` + +## Protocol Validation + +All strategies are validated against the Protocol at registration time. +The registry checks that all 9 required methods are present as callable +attributes: + +```python +# This will raise ProtocolMismatchError if the class +# doesn't implement all required methods: +registry.register("bad", "mymodule", "IncompleteClass") +``` + +## Security + +Module imports are restricted to a configurable prefix allowlist +(default: `("cleveragents.",)`). Only modules whose fully-qualified +name starts with an allowed prefix may be dynamically imported. diff --git a/docs/timeline.md b/docs/timeline.md index dd23b768..8374add2 100644 --- a/docs/timeline.md +++ b/docs/timeline.md @@ -1948,13 +1948,13 @@ This section provides a high-level overview of the CleverAgents implementation r ### Current Status Summary -As of Day 34 (2026-03-14), the project has closed **~580 of ~853 total issues** (68%) across 9 milestones. The project is running **3-4 days behind** the original schedule — improving trend since Day 32. Open PR count: **50** (up from 31 — extensive E2E and integration test PRs opened). Of 50 open PRs, **~25 are clean** (mergeable, no conflicts) and **~25 have merge conflicts**. **Major Day 34 developments**: (1) **All 100 previously unassigned issues now assigned** to developers by domain expertise — the single largest schedule risk (48% of open items had no owner) is now eliminated. (2) **All 9 bug/TDD dependency links verified and documented** — every bug↔TDD pair now has bidirectional dependency comments. (3) **TDD issue #950 created** for new bug #932 (plan apply missing --yes flag), assigned to Brent, with full TDD workflow setup. (4) **Bug priority labels fixed** — #821, #822, #823, #932 escalated to Priority/Critical. (5) **PM status comments posted on 13+ active PRs** with synthesized review state, action items, and reviewer assignments. (6) **Bug #658 fix merged** — PR #784 delivered, admin closure requested (Forgejo dependency metadata blocks API close). (7) **PR #673 (Robot TDD handler)** identified as Tier 1 blocker — 5 Critical bugs (#838-#842) waiting on this merge. PM ruling: accept with process-debt follow-up if split infeasible. (8) **2 TDD PRs (#929, #930)** from Brent now labeled Critical + In Review with reviewers assigned. +As of Day 36 (2026-03-16), the project has closed **~585 of ~865 total issues** (68%) across 9 milestones. The project is running **3-4 days behind** the original schedule — stable trend. Open PR count: **50** (~38 mergeable, ~12 with conflicts). **12 open bugs** — the highest count of the project — with 7 at Priority/Critical. **Major Day 34 developments**: (1) **All 100 previously unassigned issues now assigned** to developers by domain expertise — the single largest schedule risk (48% of open items had no owner) is now eliminated. (2) **All 9 bug/TDD dependency links verified and documented** — every bug↔TDD pair now has bidirectional dependency comments. (3) **TDD issue #950 created** for new bug #932 (plan apply missing --yes flag), assigned to Brent, with full TDD workflow setup. (4) **Bug priority labels fixed** — #821, #822, #823, #932 escalated to Priority/Critical. (5) **PM status comments posted on 13+ active PRs** with synthesized review state, action items, and reviewer assignments. (6) **Bug #658 fix merged** — PR #784 delivered, admin closure requested (Forgejo dependency metadata blocks API close). (7) **PR #673 (Robot TDD handler)** identified as Tier 1 blocker — 5 Critical bugs (#838-#842) waiting on this merge. PM ruling: accept with process-debt follow-up if split infeasible. (8) **2 TDD PRs (#929, #930)** from Brent now labeled Critical + In Review with reviewers assigned. **M1, M2 are fully complete. M3 has 43 open items** including 2 active bugs (#647 — TDD PR #670 stalled on Aditya, #620 — fix PR #640 has regression report), TDD infrastructure (#673 blocking 5 Critical bugs), and the mock removal chain. **M4 has 20 open items** including bugs #822 (TDD PR #929 in review) and #932 (TDD #950 just created). **M5 has 21 open items** including bug #821 (TDD #840 unassigned→now assigned to Brent). **M6 has 59 open items** — the largest active milestone with bugs #823 (TDD PR #930 in review), complex features (fix-then-revalidate #711, output rendering #812, diagnostic dashboard #791), and 4 resource type PRs (#661-#664, all conflicted). **M7 has 59 open items** (due Mar 28). **M8 has ~15 items** (TUI, no deadline). **M9 has ~17 items** (server, no deadline). The March 28 M7 deadline is safe if rebase debt is resolved this week. -!!! warning "Schedule Risk: 3-4 Days Behind (Improving) — Rebase Debt + PR #673 Critical Path" +!!! warning "Schedule Risk: 3-4 Days Behind (Stable) — 12 Open Bugs + Mass Assignment Needed" - The project is running **3-4 days behind** the original schedule, improving from 4-5 days on Day 32. **Day 34 critical blockers**: (1) **PR #673 (Robot TDD handler)** has REQUEST_CHANGES from Hamza (scope bundling) — blocks 5 Critical bug TDD pipelines (#838-#842). PM ruling: merge with process-debt follow-up. @hurui200320 must respond within 24h. (2) **PR #670 (TDD for bug #647)** — Aditya unresponsive 24+ hours, Day 34 EOD deadline set by Jeff. Left as-is per user directive. (3) **Rebase debt**: ~15 PRs have merge conflicts (5 from CoreRasurae, 7 from Jeff, 4 from Hamza). Until these are rebased, the merge pipeline is constrained. (4) **100 issues now assigned** — eliminates the ownership gap but creates workload distribution risk. Developer load: Luis (25+ issues), Rui (18+ issues), Hamza (15+ issues), Jeff (20+ issues, mostly M8/M9), Brent (12+ issues), Aditya (14+ issues). + The project is running **3-4 days behind** the original schedule, stable since Day 34. **Day 36 critical concerns**: (1) **12 open bugs** (highest count ever) — 3 newly filed (#967-#969, plan lifecycle CLI), 1 confirmed regression (#980, skill add persistence), plus 8 previously tracked. All have TDD counterparts created. (2) **PR #670 (TDD for bug #647) finally unblocked** — Aditya responded Day 36 after 5-day stall and formal escalation. Needs re-review and merge. (3) **~100+ issues in v3.2.0-v3.5.0 still unassigned** — mass assignment needed. (4) **Rebase debt**: 12 PRs have merge conflicts. (5) **M3 is 18 days past target** with 60+ open items including 8 bugs. ### Parallel Workstreams @@ -2055,16 +2055,19 @@ The following major areas are fully or substantially implemented: The majority of the specification's domain models, services, CLI commands, and infrastructure modules are now implemented. M1 and M2 are fully complete. M3-M6 are the active development front. Day 34 brought bulk issue assignment (100 issues assigned), TDD dependency verification, and comprehensive PR triage. The following areas still require completion: -- **Bug fixes (8 open bugs + 1 pending closure)**: - - \#647 (Container.resolve crash, M3) — TDD PR #670 stalled, Aditya unresponsive, Day 34 EOD deadline - - \#620 (Skill registration broken, M3) — Fix PR #640 submitted but regression reported by Brent - - \#658 (E2E mocks, M6) — **Fix merged** (PR #784), awaiting admin closure (Forgejo dependency block) - - \#797 (actor list causes DB update, M3) — TDD #841 assigned to Aditya, waiting - - \#783 (init --yes requires input, M3) — TDD #842 assigned to Rui, waiting - - \#821 (Context tier no runtime logic, M5) — TDD #840 assigned to Brent, waiting - - \#822 (Checkpoint rollback simulated, M4) — **TDD PR #929 in review** (Brent, reviewer: Hamza) - - \#823 (Subplan spawn no orchestration, M6) — **TDD PR #930 in review** (Brent, reviewer: Luis) - - \#932 (Plan apply missing --yes flag, M4) — TDD #950 created, assigned to Brent, bug assigned to Rui +- **Bug fixes (12 open bugs — highest count, all with TDD counterparts)**: + - \#647 (Container.resolve crash, M3) — TDD PR #670 **unblocked** (Aditya responded Day 36). Needs re-review and merge. + - \#620 (Skill registration broken, M3) — Regression confirmed. New bug #980 + TDD #981 created. + - \#980 **NEW** (Skill add regression, M3) — Cross-process persistence fails. TDD #981 (Hamza), fix assigned to Luis. + - \#967 **NEW** (Plan execute only transitions state, M3) — TDD #977 (Brent), fix assigned to Jeff. + - \#968 **NEW** (Plan explain expects decision_id, M3) — TDD #978 (Brent), fix assigned to Jeff. + - \#969 **NEW** (Plan correct expects decision_id, M3) — TDD #979 (Brent), fix assigned to Jeff. + - \#932 (Plan apply missing --yes flag, M4) — TDD PR #958 open, needs rebase. Fix assigned to Rui. + - \#822 (Checkpoint rollback simulated, M4) — TDD PR #929 in review, Hamza review 2+ days overdue. + - \#821 (Context tier no runtime logic, M5) — TDD #840 not started (Brent). **Stalled.** + - \#823 (Subplan spawn no orchestration, M6) — TDD #838 complete. **Fix can start** (Jeff). + - \#797 (actor list causes DB update, M3) — TDD #841 not started. Priority exception: Low. + - \#783 (init --yes requires input, M3) — TDD #842 not started. Priority exception: Low. - **TDD infrastructure (M3, CRITICAL PATH)**: Robot handler (#628, Rui — **PR #673 mergeable, REQUEST_CHANGES from Hamza on scope bundling**). This blocks 5 Critical bug TDD pipelines (#838-#842). PM ruling: merge with process-debt follow-up if split infeasible within 4h. - **Rebase debt (~15 PRs)**: CoreRasurae (5 PRs: #656, #736, #804, #806, #807), Jeff (7 PRs: #669, #668, #703, #708, #713, #720, #722), Hamza (4 PRs: #661-#664). All have merge conflicts blocking merge. - **M4 remaining (v3.3.0, 20 open)**: Secret masking (#656, Luis — conflicted, 4+ self-reviews done), bug #822 TDD in review, bug #932 TDD just created, plus 5 CLI/feature items newly assigned. @@ -4294,3 +4297,63 @@ Milestone per developer as of Day 32 (2026-03-12). Note: Milestone stats unchang | Rui | -1d behind (~3/8) | **active** | MEDIUM | Bug #647 blocked by TDD PR #670 (Aditya). PR #673 Robot TDD — PM ruling to merge, awaiting Hamza. PR #812 (output rendering). Bugs #797 (actor list), #932 (plan apply). | | Mike/Brian | N/A | standby | LOW | none | +### 2026-03-16 (Day 36 since kickoff on 2026-02-09) + +- Timeline reference: Day 7/M1 = 2026-02-15, Day 14/M2 = 2026-02-22, Day 18/M3 = 2026-02-26, Day 22/M4 = 2026-03-02, Day 26/M5 = 2026-03-06, Day 30/M6 = 2026-03-10 +- Summary (Team | Status | Risk | Notes): Team ~3-4d behind schedule (stable). Mar 28 deadline for M7 remains safe. 12 open bugs — highest count yet. 3 Critical bugs (#967-#969) newly filed with TDD counterparts created. Bug #620 regression confirmed, new bug #980 + TDD #981 created. PR #670 (Aditya) finally responded after 5-day stall. PR #958 (TDD for #932) needs rebase. ~150 issues still unassigned in v3.2.0-v3.5.0. #974 closed (State/Wont Do). + +> **Status**: **Project ~3-4d behind schedule (stable). Mar 28 M7 deadline safe.** **12 open bugs** (7 Critical). **3 new M3 Critical bugs** (#967, #968, #969 — plan execute/explain/correct CLI issues) with TDD counterparts #977-#979 created and assigned. **Bug #620 regression confirmed** — new bug #980 + TDD #981 created. **PR #670 finally unblocked** (Aditya responded Day 36 after 5-day stall). **PR #958** (TDD for #932) needs rebase. **#974 closed** (State/Wont Do). ~150 issues in v3.2.0-v3.5.0 still need assignment. + +- Milestone forecast (Target -> ETA | Delta | Risk): + - M1 (2026-02-15) -> Done | +13d actual | COMPLETE + - M2 (2026-02-22) -> Done | +6d actual | COMPLETE + - M3 (2026-02-26) -> **ETA 2026-03-19** | +22d | **CRITICAL** — 60+ open issues. **8 open bugs** (#647, #620/980, #967, #968, #969, #797, #783). Bug #647 TDD PR #670 finally responded (Aditya), needs re-review. 3 new plan lifecycle bugs need TDD tests written first. Bug #620 regression has new TDD #981 counterpart. ~30 unassigned issues. M3 cannot close until all Critical bugs resolved. + - M4 (2026-03-02) -> **ETA 2026-03-18** | +16d | **MEDIUM** — 21 open issues. Feature-complete, gate passed. Bug #822 (checkpoint rollback) TDD PR #929 awaiting Hamza review (2+ days overdue). Bug #932 TDD PR #958 needs rebase. Secret masking PR #656 conflicted. + - M5 (2026-03-06) -> **ETA 2026-03-19** | +13d | **MEDIUM** — 37 open issues. Gate passed. Bug #821 (ACMS tiers) TDD #840 not started. Many feature issues unassigned. + - M6 (2026-03-10) -> **ETA 2026-03-22** | +12d | **MEDIUM** — 60 open issues. Bug #823 (subplan spawn) TDD complete, fix ready to start. Estimation, output rendering, CLI polish PRs active. Large parallel workload. + - M7 (2026-03-28) -> ETA 2026-03-28 | 0d | GREEN — 54 open issues. Resource types, event taxonomy, container execution. 12 working days remain. + - M8 (v3.7.0) | No deadline | DEFERRED — TUI + - M9 (v3.8.0) | No deadline | DEFERRED — Server +- Track forecast (Track | Status | ETA | Risk | Blocking): + - Track A (Plan lifecycle) | **complete** | Done | LOW | 3 new bugs (#967-#969) affect plan CLI commands — blocked by TDD issues + - Track B (Resources + sandbox) | **complete** | Done | LOW | Bug #647 TDD PR #670 finally unblocked + - Track C (Actors/tools/skills/MCP) | **complete** | Done | **MEDIUM** | Bug #620 regression confirmed, new bug #980 created + - Track D (Decisions/validations) | **complete** | Done | LOW | none + - Track E (Corrections/subplans) | **complete** | Done | LOW | Bug #823 TDD complete, fix ready + - Track F (ACMS/context) | **complete** | Done | LOW | Bug #821 TDD #840 not started + - Track Q (Quality automation) | **near complete** | ETA 2026-03-18 | **MEDIUM** | PR #673 (Robot TDD) scope concern resolved. Mock removal 75%. + - Track T (Testing) | strong progress | ETA ongoing | **MEDIUM** | 5 TDD issues active. E2E test suites need assignment. +- Developer forecast (Name | Days Ahead/Behind | Availability | Risk | Focus): + - Jeff | +10d ahead | available | LOW | Assigned to bug fixes #967, #968, #969 (plan lifecycle expert). Next: investigate #620 regression root cause. Complete #699 (unittest.mock removal). + - Luis | +1d | **active** | **MEDIUM** | PR #711 (Fix-then-Revalidate), PR #656 (secret masking, conflicted), PR #712 (event taxonomy). Assigned bug #980 fix. + - Hamza | -1d behind | available | **MEDIUM** | TDD #981 assigned (skill regression test). Review of PR #929 overdue by 2+ days. UKO Layer 2 PR #657 merged Day 33. 4 resource PRs conflicted. + - Aditya | 0d | **active** | **HIGH** | PR #670 finally responded Day 36 after formal escalation. Must complete PR fixes and get merge. Then: bug #783 (init --yes), estimation PR #528. + - Brent | +1d | **active** | LOW | TDD #977-#979 assigned (plan lifecycle TDD tests). PR #958 needs rebase. Then: TDD #840 (ACMS tiers), M6 CLI polish. + - Rui | -1d behind | **active** | MEDIUM | Bug #932 fix (plan apply --yes, after TDD #950/PR #958 merges). PR #673 Robot TDD. PR #812 output rendering. + - Mike/Brian | N/A | standby | LOW | none +- Task inventory (Milestone x Developer): + + | Milestone | Jeff | Aditya | Luis | Hamza | Brent | Rui | Unassigned | Total | + |-----------|------|--------|------|-------|-------|-----|------------|-------| + | M3 (v3.2.0) | 12/16 | 0/2 | 2/5 | 4/4 | 9/25 | 0/3 | 7/7 | 34/62 | + | M4 (v3.3.0) | 9/10 | 0/1 | 4/6 | 5/7 | 5/8 | 0/2 | 3/4 | 26/38 | + | M5 (v3.4.0) | 22/22 | 1/2 | 2/2 | 5/5 | 3/4 | 0/1 | 4/4 | 37/40 | + | M6 (v3.5.0) | 14/17 | 0/5 | 6/9 | 0/6 | 5/8 | 1/2 | 1/1 | 27/48 | + | **Total (M3-M6)** | **57/65** | **1/10** | **14/22** | **14/22** | **22/45** | **1/8** | **15/16** | **124/188** | + +- Open bugs: **12 open bug issues** total. + - #647 (v3.2.0): TDD #648 exists, PR #670 unblocked (Aditya responded Day 36). **TDD in progress — PR needs re-review and merge.** + - #620 (v3.2.0): REGRESSION. Fix PR #640 merged but bug persists. New regression bug #980 created with TDD #981. **TDD not started (assigned to Hamza).** + - #967 (v3.2.0): NEW. Plan execute only transitions state. TDD #977 created (Brent). **TDD not started.** + - #968 (v3.2.0): NEW. Plan explain expects decision_id. TDD #978 created (Brent). **TDD not started.** + - #969 (v3.2.0): NEW. Plan correct expects decision_id. TDD #979 created (Brent). **TDD not started.** + - #932 (v3.3.0): Plan apply missing --yes flag. TDD #950 exists, PR #958 open but needs rebase. **TDD in review — PR needs rebase.** + - #822 (v3.3.0): Checkpoint rollback simulated. TDD PR #929 in review, awaiting Hamza. **TDD in review — peer review 2+ days overdue.** + - #821 (v3.4.0): ACMS tier service no runtime logic. TDD #840 not started (Brent). **TDD not started — stalled.** + - #823 (v3.5.0): Subplan spawn simulated. TDD #838 CLOSED (complete). **Fix can start — assigned to freemo.** + - #797 (v3.2.0): Actor list causes DB update. TDD #841 not started. Priority exception: Low. **TDD not started.** + - #783 (v3.2.0): Init --yes requires input. TDD #842 not started. Priority exception: Low. **TDD not started.** + - #980 (v3.2.0): NEW REGRESSION. Skill add persistence fails cross-process. TDD #981 created (Hamza). **TDD not started.** + + **Stalled TDD pipelines:** #840 (bug #821, 2+ days with no progress), #841/#842 (low priority exceptions — expected slower cadence). **Review delays:** PR #929 (Hamza review 2+ days overdue), PR #958 (needs rebase). + diff --git a/features/extensibility/custom_sandbox_strategy.feature b/features/extensibility/custom_sandbox_strategy.feature new file mode 100644 index 00000000..39874456 --- /dev/null +++ b/features/extensibility/custom_sandbox_strategy.feature @@ -0,0 +1,193 @@ +@extensibility @custom_sandbox_strategy +Feature: Custom Sandbox Strategy Registration via SandboxStrategy Protocol + As a CleverAgents developer + I want to register custom sandbox strategies via configuration + So that specialized resource types can use domain-specific isolation + + # --------------------------------------------------------------------------- + # SandboxRef model + # --------------------------------------------------------------------------- + + @sandbox_ref + Scenario: SandboxRef is a frozen dataclass + Given a SandboxRef with sandbox_id "sb-001" and plan_id "plan-001" + Then the SandboxRef sandbox_id should be "sb-001" + And the SandboxRef plan_id should be "plan-001" + And the SandboxRef should be immutable + + @sandbox_ref + Scenario: SandboxRef carries metadata + Given a SandboxRef with metadata key "backend" value "redis" + Then the SandboxRef metadata should contain key "backend" + + # --------------------------------------------------------------------------- + # DiffView / DiffEntry models + # --------------------------------------------------------------------------- + + @diff_view + Scenario: DiffEntry creation with required fields + Given a DiffEntry with path "src/main.py" and operation "modified" + Then the DiffEntry path should be "src/main.py" + And the DiffEntry operation should be "modified" + + @diff_view + Scenario: DiffView aggregates entries + Given a DiffView with 3 entries + Then the DiffView should have 3 entries + And the DiffView should have a sandbox_id + + @diff_view + Scenario: DiffEntry rejects empty path + When I attempt to create a DiffEntry with empty path + Then a sandbox strategy validation error should be raised + + # --------------------------------------------------------------------------- + # SandboxStrategyProtocol + # --------------------------------------------------------------------------- + + @sandbox_strategy_protocol + Scenario: SandboxStrategyProtocol is runtime_checkable + Given the SandboxStrategyProtocol is available + Then it should be a runtime_checkable Protocol + + @sandbox_strategy_protocol + Scenario: A class implementing all 9 methods satisfies the Protocol + Given a mock class implementing all 9 SandboxStrategy methods + Then the mock class should satisfy SandboxStrategyProtocol + + @sandbox_strategy_protocol + Scenario: A class missing methods does not satisfy the Protocol + Given a class missing the checkpoint method + Then the class should not satisfy SandboxStrategyProtocol + + # --------------------------------------------------------------------------- + # SandboxStrategyRegistry + # --------------------------------------------------------------------------- + + @strategy_registry + Scenario: Register a valid custom strategy + Given a SandboxStrategyRegistry with test allowed prefixes + And a valid custom strategy class in "cleveragents.infrastructure.sandbox.strategy_adapter" + When I register the strategy as "test_adapter" + Then the registry should contain strategy "test_adapter" + And listing strategies should include "test_adapter" + + @strategy_registry + Scenario: Registering a non-protocol class raises ProtocolMismatchError + Given a SandboxStrategyRegistry with test allowed prefixes + When I attempt to register a non-protocol class as "bad_strategy" + Then a sandbox ProtocolMismatchError should be raised + + @strategy_registry + Scenario: Register with empty name raises ValueError + Given a SandboxStrategyRegistry with test allowed prefixes + When I attempt to register a strategy with empty name + Then a sandbox ValueError should be raised + + @strategy_registry + Scenario: Get unregistered strategy returns None + Given an empty SandboxStrategyRegistry + When I look up strategy "nonexistent" + Then the strategy lookup result should be None + + @strategy_registry + Scenario: Register all from config dictionary + Given a SandboxStrategyRegistry with test allowed prefixes + And a config dictionary with 2 custom strategies + When I register all from config + Then 2 strategies should be registered + + @strategy_registry + Scenario: Clear removes all strategies + Given a SandboxStrategyRegistry with one registered strategy + When I clear the sandbox strategy registry + Then the sandbox strategy registry should be empty + + # --------------------------------------------------------------------------- + # BuiltInSandboxStrategyAdapter + # --------------------------------------------------------------------------- + + @adapter + Scenario: Adapter create returns SandboxRef + Given a BuiltInSandboxStrategyAdapter for "none" strategy + And a test Resource with location + When I call adapter.create with plan "plan-001" + Then I should get a SandboxRef with plan_id "plan-001" + + @adapter + Scenario: Adapter write creates a DiffEntry + Given a BuiltInSandboxStrategyAdapter for "copy_on_write" strategy + And a test Resource with a temporary directory + And the adapter sandbox is created for plan "plan-write" + When I call adapter.write with path "test.txt" and content "hello" + Then I should get a DiffEntry with operation "added" + + @adapter + Scenario: Adapter read returns file content + Given a BuiltInSandboxStrategyAdapter for "copy_on_write" strategy + And a test Resource with a temporary directory + And the adapter sandbox is created for plan "plan-read" + And I write "data" to "read_test.txt" via the adapter + When I call adapter.read for "read_test.txt" + Then I should get content "data" + + @adapter + Scenario: Adapter diff returns DiffView + Given a BuiltInSandboxStrategyAdapter for "none" strategy + And a test Resource with location + And the adapter sandbox is created for plan "plan-diff" + When I call adapter.diff + Then I should get a DiffView + + @adapter + Scenario: Adapter checkpoint and restore + Given a BuiltInSandboxStrategyAdapter for "copy_on_write" strategy + And a test Resource with a temporary directory + And the adapter sandbox is created for plan "plan-cp" + And I write "v1" to "cp_test.txt" via the adapter + And I checkpoint as "cp-1" + And I write "v2" to "cp_test.txt" via the adapter + When I restore checkpoint "cp-1" + Then reading "cp_test.txt" should return "v1" + + @adapter + Scenario: Adapter cleanup releases resources + Given a BuiltInSandboxStrategyAdapter for "none" strategy + And a test Resource with location + And the adapter sandbox is created for plan "plan-cleanup" + When I call adapter.cleanup + Then the adapter should have no tracked sandboxes + + # --------------------------------------------------------------------------- + # CustomStrategyConfig + # --------------------------------------------------------------------------- + + @config + Scenario: CustomStrategyConfig rejects empty name + When I attempt to create a CustomStrategyConfig with empty name + Then a sandbox ValueError should be raised + + @config + Scenario: CustomStrategyConfig rejects empty module + When I attempt to create a CustomStrategyConfig with empty module + Then a sandbox ValueError should be raised + + @config + Scenario: CustomStrategyConfig rejects empty class_name + When I attempt to create a CustomStrategyConfig with empty class_name + Then a sandbox ValueError should be raised + + # --------------------------------------------------------------------------- + # Factory custom strategy integration + # --------------------------------------------------------------------------- + + @factory_integration + Scenario: Factory reports custom strategy availability + Given a SandboxFactory with a custom registry containing "my_custom" + Then the factory should report "my_custom" as a custom strategy + And the factory should not report "nonexistent" as a custom strategy + + @factory_integration + Scenario: Factory without registry reports no custom strategies + Given a SandboxFactory without a custom registry + Then the factory should not report "anything" as a custom strategy diff --git a/features/mocks/__init__.py b/features/mocks/__init__.py index ad872956..43b8f366 100644 --- a/features/mocks/__init__.py +++ b/features/mocks/__init__.py @@ -11,6 +11,11 @@ from .mock_ai_provider import MockAIProvider from .mock_mcp_transport import MockMCPTransport from .transient_fail_audit_service import TransientFailAuditService +# NOTE: tdd_test_helpers is NOT re-exported here because it imports +# ``behave.model.Status`` which is unavailable in ASV benchmark +# environments. Import directly: ``from features.mocks.tdd_test_helpers +# import make_mock_scenario`` + __all__ = [ "FakeProviderInfo", "FakeProviderRegistry", diff --git a/features/mocks/tdd_test_helpers.py b/features/mocks/tdd_test_helpers.py new file mode 100644 index 00000000..e1add58d --- /dev/null +++ b/features/mocks/tdd_test_helpers.py @@ -0,0 +1,65 @@ +"""Shared mock helpers for TDD tag validation tests. + +Provides ``make_mock_scenario`` — a factory for lightweight mock +``Scenario`` objects used by both the Behave step definitions +(``features/steps/tdd_tag_validation_steps.py``) and the Robot +Framework integration test helper (``robot/helper_tdd_tag_validation.py``). + +Centralising the mock builder eliminates duplication and ensures both +test suites exercise ``apply_tdd_inversion`` with identically-shaped +mock objects. + +See CONTRIBUTING.md > TDD Bug Test Tags for the three-tag specification. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from behave.model import Status + + +def make_mock_scenario( + tags: list[str], + steps_passed: bool = True, + hook_failed: bool = False, + was_dry_run: bool = False, + step_exception: BaseException | None = None, +) -> MagicMock: + """Build a lightweight mock ``Scenario`` for ``apply_tdd_inversion`` tests. + + Args: + tags: The effective tags to place on the scenario. + steps_passed: When ``True`` every step has ``Status.passed``. + When ``False`` the first step has ``Status.failed`` with + *step_exception* (defaulting to ``AssertionError``). + hook_failed: Simulates an infrastructure/hook error. + was_dry_run: Simulates ``--dry-run`` mode. + step_exception: The exception attached to the failed step when + *steps_passed* is ``False``. Defaults to ``AssertionError``. + + Returns: + A ``MagicMock`` configured to look like a Behave ``Scenario``. + """ + scenario = MagicMock() + scenario.effective_tags = tags + scenario.name = "mock-scenario" + scenario.hook_failed = hook_failed + scenario.was_dry_run = was_dry_run + + mock_step = MagicMock() + if steps_passed: + mock_step.status = Status.passed + mock_step.exception = None + else: + mock_step.status = Status.failed + mock_step.exception = ( + step_exception + if step_exception is not None + else AssertionError("simulated assertion failure") + ) + scenario.all_steps = [mock_step] + return scenario + + +__all__: list[str] = ["make_mock_scenario"] diff --git a/features/steps/custom_sandbox_strategy_steps.py b/features/steps/custom_sandbox_strategy_steps.py new file mode 100644 index 00000000..7283fedb --- /dev/null +++ b/features/steps/custom_sandbox_strategy_steps.py @@ -0,0 +1,602 @@ +"""Behave step definitions for Custom Sandbox Strategy Registration. + +Covers SandboxRef, DiffView, DiffEntry, SandboxStrategyProtocol, +SandboxStrategyRegistry, BuiltInSandboxStrategyAdapter, +CustomStrategyConfig, and factory integration. + +Based on issue #586. +""" + +from __future__ import annotations + +import os +import tempfile +from datetime import datetime +from typing import Any + +from behave import given, then, when # type: ignore[import-untyped] +from behave.runner import Context # type: ignore[import-untyped] +from pydantic import ValidationError + +from cleveragents.domain.models.core.resource import ( + PhysVirt, + Resource, + ResourceCapabilities, +) +from cleveragents.domain.models.core.resource import ( + SandboxStrategy as SandboxStrategyEnum, +) +from cleveragents.domain.models.core.sandbox_strategy import ( + DiffEntry, + DiffView, + SandboxRef, + SandboxStrategyProtocol, +) +from cleveragents.infrastructure.plugins.exceptions import ( + ProtocolMismatchError, +) +from cleveragents.infrastructure.sandbox.factory import SandboxFactory +from cleveragents.infrastructure.sandbox.strategy_adapter import ( + BuiltInSandboxStrategyAdapter, +) +from cleveragents.infrastructure.sandbox.strategy_registry import ( + CustomStrategyConfig, + SandboxStrategyRegistry, +) + +# --------------------------------------------------------------------------- +# ULID helper +# --------------------------------------------------------------------------- + +_COUNTER = 0 + + +def _ulid(name: str) -> str: + global _COUNTER + _COUNTER += 1 + base = abs(hash(name)) % (10**20) + raw = f"{_COUNTER:06d}{base:020d}" + charset = "0123456789ABCDEFGHJKMNPQRSTVWXYZ" + result = "" + for ch in raw: + result += charset[int(ch)] + return (result + "0" * 26)[:26] + + +def _make_resource(name: str, location: str | None = None) -> Resource: + return Resource( + resource_id=_ulid(name), + name=name, + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.NONE, + location=location, + capabilities=ResourceCapabilities( + readable=True, writable=True, sandboxable=True, checkpointable=False + ), + ) + + +# --------------------------------------------------------------------------- +# SandboxRef +# --------------------------------------------------------------------------- + + +@given('a SandboxRef with sandbox_id "{sid}" and plan_id "{pid}"') +def step_create_sandbox_ref(context: Context, sid: str, pid: str) -> None: + context.sandbox_ref = SandboxRef( + sandbox_id=sid, + plan_id=pid, + resource_id="res-001", + created_at=datetime.now(), + ) + + +@then('the SandboxRef sandbox_id should be "{expected}"') +def step_sandbox_ref_sid(context: Context, expected: str) -> None: + assert context.sandbox_ref.sandbox_id == expected + + +@then('the SandboxRef plan_id should be "{expected}"') +def step_sandbox_ref_pid(context: Context, expected: str) -> None: + assert context.sandbox_ref.plan_id == expected + + +@then("the SandboxRef should be immutable") +def step_sandbox_ref_frozen(context: Context) -> None: + import dataclasses + + assert dataclasses.is_dataclass(context.sandbox_ref) + try: + context.sandbox_ref.sandbox_id = "mutated" # type: ignore[misc] + raise AssertionError("Expected FrozenInstanceError") + except (AttributeError, dataclasses.FrozenInstanceError): + pass + + +@given('a SandboxRef with metadata key "{key}" value "{value}"') +def step_sandbox_ref_metadata(context: Context, key: str, value: str) -> None: + context.sandbox_ref = SandboxRef( + sandbox_id="sb-meta", + plan_id="plan-meta", + resource_id="res-meta", + created_at=datetime.now(), + metadata={key: value}, + ) + + +@then('the SandboxRef metadata should contain key "{key}"') +def step_sandbox_ref_meta_key(context: Context, key: str) -> None: + assert key in context.sandbox_ref.metadata + + +# --------------------------------------------------------------------------- +# DiffEntry / DiffView +# --------------------------------------------------------------------------- + + +@given('a DiffEntry with path "{path}" and operation "{op}"') +def step_create_diff_entry(context: Context, path: str, op: str) -> None: + context.diff_entry = DiffEntry(path=path, operation=op) + + +@then('the DiffEntry path should be "{expected}"') +def step_diff_entry_path(context: Context, expected: str) -> None: + assert context.diff_entry.path == expected + + +@then('the DiffEntry operation should be "{expected}"') +def step_diff_entry_op(context: Context, expected: str) -> None: + assert context.diff_entry.operation == expected + + +@given("a DiffView with {count:d} entries") +def step_create_diff_view(context: Context, count: int) -> None: + entries = [ + DiffEntry(path=f"file{i}.txt", operation="modified") for i in range(count) + ] + context.diff_view = DiffView( + sandbox_id="dv-001", + entries=entries, + ) + + +@then("the DiffView should have {count:d} entries") +def step_diff_view_count(context: Context, count: int) -> None: + assert len(context.diff_view.entries) == count + + +@then("the DiffView should have a sandbox_id") +def step_diff_view_sid(context: Context) -> None: + assert context.diff_view.sandbox_id + + +@when("I attempt to create a DiffEntry with empty path") +def step_create_diff_entry_empty(context: Context) -> None: + context.sandbox_error = None + try: + DiffEntry(path="", operation="modified") + except ValidationError as exc: + context.sandbox_error = exc + + +@then("a sandbox strategy validation error should be raised") +def step_sandbox_validation_error(context: Context) -> None: + assert context.sandbox_error is not None + + +# --------------------------------------------------------------------------- +# SandboxStrategyProtocol +# --------------------------------------------------------------------------- + + +@given("the SandboxStrategyProtocol is available") +def step_protocol_available(context: Context) -> None: + context.protocol = SandboxStrategyProtocol + + +@then("it should be a runtime_checkable Protocol") +def step_protocol_runtime_checkable(context: Context) -> None: + assert hasattr(context.protocol, "__protocol_attrs__") or hasattr( + context.protocol, "_is_runtime_protocol" + ) + + +class _FullMockStrategy: + """Mock satisfying all 9 SandboxStrategy methods.""" + + def create(self, plan_id: str, resource: Any) -> Any: + return None + + def read(self, ref: Any, path: str) -> bytes: + return b"" + + def write(self, ref: Any, path: str, content: bytes) -> Any: + return None + + def diff(self, ref: Any) -> Any: + return None + + def commit(self, ref: Any) -> None: + pass + + def rollback(self, ref: Any) -> None: + pass + + def checkpoint(self, ref: Any, checkpoint_id: str) -> None: + pass + + def restore_checkpoint(self, ref: Any, checkpoint_id: str) -> None: + pass + + def cleanup(self, ref: Any) -> None: + pass + + +class _PartialMockStrategy: + """Missing checkpoint method.""" + + def create(self, plan_id: str, resource: Any) -> Any: + return None + + def read(self, ref: Any, path: str) -> bytes: + return b"" + + def write(self, ref: Any, path: str, content: bytes) -> Any: + return None + + def diff(self, ref: Any) -> Any: + return None + + def commit(self, ref: Any) -> None: + pass + + def rollback(self, ref: Any) -> None: + pass + + # Missing: checkpoint, restore_checkpoint + + def cleanup(self, ref: Any) -> None: + pass + + +@given("a mock class implementing all 9 SandboxStrategy methods") +def step_full_mock(context: Context) -> None: + context.mock_cls = _FullMockStrategy + + +@then("the mock class should satisfy SandboxStrategyProtocol") +def step_mock_satisfies(context: Context) -> None: + instance = context.mock_cls() + assert isinstance(instance, SandboxStrategyProtocol) + + +@given("a class missing the checkpoint method") +def step_partial_mock(context: Context) -> None: + context.partial_cls = _PartialMockStrategy + + +@then("the class should not satisfy SandboxStrategyProtocol") +def step_partial_fails(context: Context) -> None: + instance = context.partial_cls() + assert not isinstance(instance, SandboxStrategyProtocol) + + +# --------------------------------------------------------------------------- +# SandboxStrategyRegistry +# --------------------------------------------------------------------------- + + +@given("a SandboxStrategyRegistry with test allowed prefixes") +def step_registry_with_prefixes(context: Context) -> None: + context.strategy_registry = SandboxStrategyRegistry( + allowed_prefixes=("cleveragents.",) + ) + + +@given("an empty SandboxStrategyRegistry") +def step_empty_registry(context: Context) -> None: + context.strategy_registry = SandboxStrategyRegistry( + allowed_prefixes=("cleveragents.",) + ) + + +@given( + "a valid custom strategy class in " + '"cleveragents.infrastructure.sandbox.strategy_adapter"' +) +def step_valid_strategy_class(context: Context) -> None: + context.strategy_module = "cleveragents.infrastructure.sandbox.strategy_adapter" + context.strategy_class = "BuiltInSandboxStrategyAdapter" + + +@when('I register the strategy as "{name}"') +def step_register_strategy(context: Context, name: str) -> None: + context.strategy_registry.register( + name, context.strategy_module, context.strategy_class + ) + + +@then('the registry should contain strategy "{name}"') +def step_registry_contains(context: Context, name: str) -> None: + assert context.strategy_registry.has(name) + + +@then('listing strategies should include "{name}"') +def step_registry_list(context: Context, name: str) -> None: + assert name in context.strategy_registry.list_strategies() + + +@when('I attempt to register a non-protocol class as "{name}"') +def step_register_nonprotocol(context: Context, name: str) -> None: + context.sandbox_error = None + try: + context.strategy_registry.register( + name, + "cleveragents.infrastructure.sandbox.protocol", + "SandboxError", + ) + except ProtocolMismatchError as exc: + context.sandbox_error = exc + + +@then("a sandbox ProtocolMismatchError should be raised") +def step_sandbox_protocol_mismatch(context: Context) -> None: + assert isinstance(context.sandbox_error, ProtocolMismatchError) + + +@when("I attempt to register a strategy with empty name") +def step_register_empty_name(context: Context) -> None: + context.sandbox_error = None + try: + context.strategy_registry.register( + "", + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + except ValueError as exc: + context.sandbox_error = exc + + +@then("a sandbox ValueError should be raised") +def step_sandbox_value_error(context: Context) -> None: + assert isinstance(context.sandbox_error, (ValueError, type(None))) is False or ( + context.sandbox_error is not None + ) + assert context.sandbox_error is not None + + +@when('I look up strategy "{name}"') +def step_lookup_strategy(context: Context, name: str) -> None: + context.strategy_lookup_result = context.strategy_registry.get(name) + + +@then("the strategy lookup result should be None") +def step_strategy_result_none(context: Context) -> None: + assert context.strategy_lookup_result is None + + +@given("a config dictionary with 2 custom strategies") +def step_config_dict(context: Context) -> None: + context.config_dict = { + "adapter1": { + "module": "cleveragents.infrastructure.sandbox.strategy_adapter", + "class": "BuiltInSandboxStrategyAdapter", + }, + "adapter2": { + "module": "cleveragents.infrastructure.sandbox.strategy_adapter", + "class": "BuiltInSandboxStrategyAdapter", + }, + } + + +@when("I register all from config") +def step_register_all_config(context: Context) -> None: + context.registered = context.strategy_registry.register_all_from_config( + context.config_dict + ) + + +@then("{count:d} strategies should be registered") +def step_count_registered(context: Context, count: int) -> None: + assert len(context.registered) == count + + +@given("a SandboxStrategyRegistry with one registered strategy") +def step_registry_one(context: Context) -> None: + context.strategy_registry = SandboxStrategyRegistry( + allowed_prefixes=("cleveragents.",) + ) + context.strategy_registry.register( + "single", + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + + +@when("I clear the sandbox strategy registry") +def step_clear_sandbox_registry(context: Context) -> None: + context.strategy_registry.clear() + + +@then("the sandbox strategy registry should be empty") +def step_sandbox_registry_empty(context: Context) -> None: + assert len(context.strategy_registry.list_strategies()) == 0 + + +# --------------------------------------------------------------------------- +# BuiltInSandboxStrategyAdapter +# --------------------------------------------------------------------------- + + +@given('a BuiltInSandboxStrategyAdapter for "{strategy}" strategy') +def step_adapter(context: Context, strategy: str) -> None: + context.adapter = BuiltInSandboxStrategyAdapter(strategy_name=strategy) # type: ignore[arg-type] + + +@given("a test Resource with location") +def step_resource_with_location(context: Context) -> None: + context.tmpdir = tempfile.mkdtemp(prefix="ca-test-") + context.test_resource = _make_resource("test-res", location=context.tmpdir) + + +@given("a test Resource with a temporary directory") +def step_resource_with_tmpdir(context: Context) -> None: + context.tmpdir = tempfile.mkdtemp(prefix="ca-test-cow-") + # Create a seed file so CoW sandbox has something to copy + with open(os.path.join(context.tmpdir, "seed.txt"), "w") as f: + f.write("seed") + context.test_resource = Resource( + resource_id=_ulid("cow-res"), + name="cow-res", + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.COPY_ON_WRITE, + location=context.tmpdir, + capabilities=ResourceCapabilities( + readable=True, writable=True, sandboxable=True, checkpointable=False + ), + ) + + +@when('I call adapter.create with plan "{plan_id}"') +def step_adapter_create(context: Context, plan_id: str) -> None: + context.adapter_ref = context.adapter.create(plan_id, context.test_resource) + + +@given('the adapter sandbox is created for plan "{plan_id}"') +def step_adapter_create_given(context: Context, plan_id: str) -> None: + context.adapter_ref = context.adapter.create(plan_id, context.test_resource) + + +@then('I should get a SandboxRef with plan_id "{expected}"') +def step_adapter_ref_check(context: Context, expected: str) -> None: + assert isinstance(context.adapter_ref, SandboxRef) + assert context.adapter_ref.plan_id == expected + + +@when('I call adapter.write with path "{path}" and content "{content}"') +def step_adapter_write(context: Context, path: str, content: str) -> None: + context.write_result = context.adapter.write( + context.adapter_ref, path, content.encode() + ) + + +@given('I write "{content}" to "{path}" via the adapter') +def step_adapter_write_given(context: Context, content: str, path: str) -> None: + context.adapter.write(context.adapter_ref, path, content.encode()) + + +@then('I should get a DiffEntry with operation "{expected}"') +def step_adapter_write_op(context: Context, expected: str) -> None: + assert isinstance(context.write_result, DiffEntry) + assert context.write_result.operation == expected + + +@when('I call adapter.read for "{path}"') +def step_adapter_read(context: Context, path: str) -> None: + context.read_result = context.adapter.read(context.adapter_ref, path) + + +@then('I should get content "{expected}"') +def step_adapter_read_content(context: Context, expected: str) -> None: + assert context.read_result == expected.encode() + + +@when("I call adapter.diff") +def step_adapter_diff(context: Context) -> None: + context.diff_result = context.adapter.diff(context.adapter_ref) + + +@then("I should get a DiffView") +def step_adapter_diff_check(context: Context) -> None: + assert isinstance(context.diff_result, DiffView) + + +@given('I checkpoint as "{cp_id}"') +def step_adapter_checkpoint(context: Context, cp_id: str) -> None: + context.adapter.checkpoint(context.adapter_ref, cp_id) + + +@when('I restore checkpoint "{cp_id}"') +def step_adapter_restore(context: Context, cp_id: str) -> None: + context.adapter.restore_checkpoint(context.adapter_ref, cp_id) + + +@then('reading "{path}" should return "{expected}"') +def step_adapter_read_after_restore(context: Context, path: str, expected: str) -> None: + data = context.adapter.read(context.adapter_ref, path) + assert data == expected.encode(), f"Expected {expected!r}, got {data!r}" + + +@when("I call adapter.cleanup") +def step_adapter_cleanup(context: Context) -> None: + context.adapter.cleanup(context.adapter_ref) + + +@then("the adapter should have no tracked sandboxes") +def step_adapter_no_sandboxes(context: Context) -> None: + assert len(context.adapter._sandboxes) == 0 + + +# --------------------------------------------------------------------------- +# CustomStrategyConfig +# --------------------------------------------------------------------------- + + +@when("I attempt to create a CustomStrategyConfig with empty name") +def step_config_empty_name(context: Context) -> None: + context.sandbox_error = None + try: + CustomStrategyConfig(name="", module="mod", class_name="Cls") + except ValueError as exc: + context.sandbox_error = exc + + +@when("I attempt to create a CustomStrategyConfig with empty module") +def step_config_empty_module(context: Context) -> None: + context.sandbox_error = None + try: + CustomStrategyConfig(name="x", module="", class_name="Cls") + except ValueError as exc: + context.sandbox_error = exc + + +@when("I attempt to create a CustomStrategyConfig with empty class_name") +def step_config_empty_class(context: Context) -> None: + context.sandbox_error = None + try: + CustomStrategyConfig(name="x", module="mod", class_name="") + except ValueError as exc: + context.sandbox_error = exc + + +# --------------------------------------------------------------------------- +# Factory custom strategy integration +# --------------------------------------------------------------------------- + + +@given('a SandboxFactory with a custom registry containing "{name}"') +def step_factory_with_registry(context: Context, name: str) -> None: + registry = SandboxStrategyRegistry(allowed_prefixes=("cleveragents.",)) + registry.register( + name, + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + context.factory = SandboxFactory(custom_registry=registry) + + +@given("a SandboxFactory without a custom registry") +def step_factory_no_registry(context: Context) -> None: + context.factory = SandboxFactory() + + +@then('the factory should report "{name}" as a custom strategy') +def step_factory_has_custom(context: Context, name: str) -> None: + assert context.factory.has_custom_strategy(name) + + +@then('the factory should not report "{name}" as a custom strategy') +def step_factory_no_custom(context: Context, name: str) -> None: + assert not context.factory.has_custom_strategy(name) diff --git a/features/steps/tdd_tag_validation_steps.py b/features/steps/tdd_tag_validation_steps.py index d9f5f464..4308d06a 100644 --- a/features/steps/tdd_tag_validation_steps.py +++ b/features/steps/tdd_tag_validation_steps.py @@ -23,6 +23,7 @@ from features.environment import ( should_invert_result, validate_tdd_tags, ) +from features.mocks.tdd_test_helpers import make_mock_scenario # --------------------------------------------------------------------------- # Tag-set construction helpers @@ -96,46 +97,9 @@ def step_then_invert_false(context: Context) -> None: # --------------------------------------------------------------------------- # apply_tdd_inversion unit-test helpers # --------------------------------------------------------------------------- - - -def _make_mock_scenario( - tags: list[str], - steps_passed: bool = True, - hook_failed: bool = False, - was_dry_run: bool = False, - step_exception: BaseException | None = None, -) -> MagicMock: - """Build a lightweight mock ``Scenario`` for ``apply_tdd_inversion`` tests. - - Args: - tags: The effective tags to place on the scenario. - steps_passed: When ``True`` every step has ``Status.passed``. - When ``False`` the first step has ``Status.failed`` with - *step_exception* (defaulting to ``AssertionError``). - hook_failed: Simulates an infrastructure/hook error. - was_dry_run: Simulates ``--dry-run`` mode. - step_exception: The exception attached to the failed step when - *steps_passed* is ``False``. Defaults to ``AssertionError``. - """ - scenario = MagicMock() - scenario.effective_tags = tags - scenario.name = "mock-scenario" - scenario.hook_failed = hook_failed - scenario.was_dry_run = was_dry_run - - mock_step = MagicMock() - if steps_passed: - mock_step.status = Status.passed - mock_step.exception = None - else: - mock_step.status = Status.failed - mock_step.exception = ( - step_exception - if step_exception is not None - else AssertionError("simulated assertion failure") - ) - scenario.all_steps = [mock_step] - return scenario +# The mock scenario builder is in ``features/mocks/tdd_test_helpers.py`` +# (imported above as ``make_mock_scenario``) to avoid duplication with +# ``robot/helper_tdd_tag_validation.py``. # --------------------------------------------------------------------------- @@ -149,7 +113,7 @@ def _make_mock_scenario( ) def step_given_expected_failure_scenario(context: Context) -> None: """Create a mock scenario with a failed step (AssertionError) and TDD tags.""" - context.tdd_mock_scenario = _make_mock_scenario( + context.tdd_mock_scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_998", "tdd_expected_fail"], steps_passed=False, step_exception=AssertionError("simulated bug assertion failure"), @@ -202,7 +166,7 @@ def step_then_error_messages_cleared(context: Context) -> None: @given("tdd inversion a mock scenario that passes with tdd_expected_fail tags") def step_given_unexpected_pass_scenario(context: Context) -> None: """Create a mock scenario where steps pass but @tdd_expected_fail is set.""" - context.tdd_mock_scenario = _make_mock_scenario( + context.tdd_mock_scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_998", "tdd_expected_fail"], steps_passed=True, ) @@ -259,7 +223,7 @@ def step_then_last_step_has_error_message(context: Context) -> None: @given("tdd inversion a mock scenario with hook_failed and tdd_expected_fail tags") def step_given_hook_failed_scenario(context: Context) -> None: - context.tdd_mock_scenario = _make_mock_scenario( + context.tdd_mock_scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_998", "tdd_expected_fail"], steps_passed=False, hook_failed=True, @@ -287,7 +251,7 @@ def step_then_result_not_inverted(context: Context) -> None: @given("tdd inversion a mock scenario in dry-run mode with tdd_expected_fail tags") def step_given_dry_run_scenario(context: Context) -> None: - context.tdd_mock_scenario = _make_mock_scenario( + context.tdd_mock_scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_998", "tdd_expected_fail"], steps_passed=True, was_dry_run=True, @@ -308,7 +272,7 @@ def step_then_result_no_inversion(context: Context) -> None: @given("tdd inversion a mock scenario with a RuntimeError and tdd_expected_fail tags") def step_given_non_assertion_scenario(context: Context) -> None: - context.tdd_mock_scenario = _make_mock_scenario( + context.tdd_mock_scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_998", "tdd_expected_fail"], steps_passed=False, step_exception=RuntimeError("connection lost"), diff --git a/noxfile.py b/noxfile.py index 9de22a38..f103e916 100644 --- a/noxfile.py +++ b/noxfile.py @@ -580,6 +580,15 @@ def integration_tests(session: nox.Session): pabot_args, robot_args = _split_pabot_args(session.posargs) parallel_args = _pabot_parallel_args(pabot_args) + # TDD expected-fail listener — inverts results for @tdd_expected_fail + # tagged tests and validates TDD tag combinations. + # See CONTRIBUTING.md > TDD Bug Test Tags. + # Resolved relative to this file (not CWD) so ``nox`` invocations from + # a non-root directory still find the listener module. + tdd_listener = str( + Path(__file__).parent / "robot" / "tdd_expected_fail_listener.py" + ) + session.run( "pabot", *parallel_args, @@ -596,6 +605,8 @@ def integration_tests(session: nox.Session): "xunit.xml", "--variable", f"PYTHON:{venv_python}", + "--listener", + tdd_listener, "--exclude", "slow", "--exclude", @@ -606,8 +617,8 @@ def integration_tests(session: nox.Session): "wip", "--exclude", "E2E", - "--listener", - "robot/tdd_expected_fail_listener.py", + "--exclude", + "tdd_fixture", *robot_args, "robot/", ) @@ -618,6 +629,16 @@ def slow_integration_tests(session: nox.Session): """Run Robot Framework integration tests.""" session.install("-e", ".[tests]") session.env["CLEVERAGENTS_AUTO_APPLY_MIGRATIONS"] = "true" + + # TDD expected-fail listener — inverts results for @tdd_expected_fail + # tagged tests and validates TDD tag combinations. + # See CONTRIBUTING.md > TDD Bug Test Tags. + # Resolved relative to this file (not CWD) so ``nox`` invocations from + # a non-root directory still find the listener module. + tdd_listener = str( + Path(__file__).parent / "robot" / "tdd_expected_fail_listener.py" + ) + session.run( "robot", "--outputdir", @@ -631,7 +652,9 @@ def slow_integration_tests(session: nox.Session): "--xunit", "xunit.xml", "--listener", - "robot/tdd_expected_fail_listener.py", + tdd_listener, + "--exclude", + "tdd_fixture", "robot/", *session.posargs, ) @@ -696,7 +719,7 @@ def e2e_tests(session: nox.Session): "--include", "E2E", "--listener", - "robot/tdd_expected_fail_listener.py", + str(Path(__file__).parent / "robot" / "tdd_expected_fail_listener.py"), *session.posargs, "robot/e2e/", ) diff --git a/robot/_tdd_fixture_runner.py b/robot/_tdd_fixture_runner.py new file mode 100644 index 00000000..07cb5ddb --- /dev/null +++ b/robot/_tdd_fixture_runner.py @@ -0,0 +1,186 @@ +"""Subprocess runner for TDD Robot Framework fixture files. + +Runs ``.robot`` fixture files with the ``tdd_expected_fail_listener`` +active and inspects the resulting ``output.xml`` to determine the final +test status after listener processing. + +Extracted from ``helper_tdd_tag_validation.py`` to keep file lengths +under the 500-line project limit. +""" + +from __future__ import annotations + +__all__: list[str] = [] + +import subprocess +import sys +import tempfile + +# Robot Framework generates the output XML; we parse it to inspect test +# results. The XML is always self-generated (never from untrusted input), +# so the standard library parser is sufficient — no XXE risk. +import xml.etree.ElementTree as ET +from pathlib import Path + +_ROBOT_DIR = Path(__file__).resolve().parent +_LISTENER = str(_ROBOT_DIR / "tdd_expected_fail_listener.py") +_FIXTURES = _ROBOT_DIR / "fixtures" + +_SUBPROCESS_TIMEOUT = 120 # seconds — matches project convention + + +def _extract_message( + test_el: ET.Element, + status_el: ET.Element, +) -> str: + """Extract the test message from an RF output XML status element. + + RF may place the message as element text, a ``message`` attribute + (RF 7.3+), or in a child ```` element. Check all three. + """ + message = status_el.text or "" + if not message: + message = status_el.get("message", "") + if not message: + msg_el = test_el.find(".//msg") + if msg_el is not None and msg_el.text: + message = msg_el.text + return message + + +def run_fixture(fixture_name: str) -> tuple[str, str]: + """Run a fixture ``.robot`` file and return ``(status, message)``. + + Returns the status and message of the *first* test case found in + the Robot output XML. + """ + fixture_path = _FIXTURES / f"{fixture_name}.robot" + if not fixture_path.exists(): + print(f"ERROR: fixture not found: {fixture_path}", file=sys.stderr) + sys.exit(1) + + with tempfile.TemporaryDirectory() as tmpdir: + out_xml = str(Path(tmpdir) / "output.xml") + cmd = [ + sys.executable, + "-m", + "robot", + "--listener", + _LISTENER, + "--outputdir", + tmpdir, + "--loglevel", + "INFO", + "--report", + "NONE", + "--log", + "NONE", + str(fixture_path), + ] + try: + proc = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=_SUBPROCESS_TIMEOUT, + ) + except subprocess.TimeoutExpired: + print( + f"ERROR: fixture {fixture_name!r} timed out after " + f"{_SUBPROCESS_TIMEOUT}s", + file=sys.stderr, + ) + sys.exit(1) + + if not Path(out_xml).exists(): + print( + f"ERROR: Robot did not produce output.xml (rc={proc.returncode})", + file=sys.stderr, + ) + print(f"stderr: {proc.stderr}", file=sys.stderr) + sys.exit(1) + + tree = ET.parse(out_xml) + root = tree.getroot() + test_el = root.find(".//test") + if test_el is None: + return "ERROR", "No test found in output XML" + status_el = test_el.find("status") + if status_el is None: + return "ERROR", "No status element found" + status = status_el.get("status", "UNKNOWN") + message = _extract_message(test_el, status_el) + return status, message + + +def run_multi_fixture( + *fixture_names: str, +) -> dict[str, tuple[str, str]]: + """Run multiple fixture ``.robot`` files in a single Robot invocation. + + Returns a dict mapping test name to ``(status, message)``. Running + multiple fixtures together proves the listener is loaded and + selectively applies (normal tests are unaffected *in the same run* + as ``tdd_expected_fail`` tests). + """ + fixture_paths = [] + for name in fixture_names: + fp = _FIXTURES / f"{name}.robot" + if not fp.exists(): + print(f"ERROR: fixture not found: {fp}", file=sys.stderr) + sys.exit(1) + fixture_paths.append(str(fp)) + + with tempfile.TemporaryDirectory() as tmpdir: + out_xml = str(Path(tmpdir) / "output.xml") + cmd = [ + sys.executable, + "-m", + "robot", + "--listener", + _LISTENER, + "--outputdir", + tmpdir, + "--loglevel", + "INFO", + "--report", + "NONE", + "--log", + "NONE", + *fixture_paths, + ] + try: + proc = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=_SUBPROCESS_TIMEOUT, + ) + except subprocess.TimeoutExpired: + print( + f"ERROR: multi-fixture run timed out after {_SUBPROCESS_TIMEOUT}s", + file=sys.stderr, + ) + sys.exit(1) + + if not Path(out_xml).exists(): + print( + f"ERROR: Robot did not produce output.xml (rc={proc.returncode})", + file=sys.stderr, + ) + print(f"stderr: {proc.stderr}", file=sys.stderr) + sys.exit(1) + + tree = ET.parse(out_xml) + root = tree.getroot() + results: dict[str, tuple[str, str]] = {} + for test_el in root.findall(".//test"): + test_name = test_el.get("name", "UNKNOWN") + status_el = test_el.find("status") + if status_el is None: + results[test_name] = ("ERROR", "No status element found") + continue + status = status_el.get("status", "UNKNOWN") + message = _extract_message(test_el, status_el) + results[test_name] = (status, message) + return results diff --git a/robot/custom_sandbox_strategy.robot b/robot/custom_sandbox_strategy.robot new file mode 100644 index 00000000..8a0d6b6f --- /dev/null +++ b/robot/custom_sandbox_strategy.robot @@ -0,0 +1,69 @@ +*** Settings *** +Documentation Integration tests for custom sandbox strategy registration. +... Covers SandboxStrategyProtocol, SandboxStrategyRegistry, +... BuiltInSandboxStrategyAdapter, CustomStrategyConfig, and +... SandboxFactory custom strategy integration. +... Based on issue #586. +Resource ${CURDIR}/common.resource +Suite Setup Setup Test Environment +Suite Teardown Cleanup Test Environment + +*** Variables *** +${HELPER_SCRIPT} robot/helper_custom_sandbox_strategy.py + +*** Test Cases *** +SandboxRef Creation And Immutability + [Documentation] Verify SandboxRef creation, properties, and frozen semantics + [Tags] sandbox strategy ref + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} sandbox-ref-lifecycle cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} sandbox-ref-lifecycle-ok + +DiffView And DiffEntry Models + [Documentation] Verify DiffView and DiffEntry creation and validation + [Tags] sandbox strategy diff + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} diff-view-lifecycle cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} diff-view-lifecycle-ok + +SandboxStrategyProtocol Runtime Check + [Documentation] Verify SandboxStrategyProtocol is runtime_checkable with 9 methods + [Tags] sandbox strategy protocol + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} protocol-check cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} protocol-check-ok + +SandboxStrategyRegistry Lifecycle + [Documentation] Verify registry register, lookup, config-driven, protocol validation, clear + [Tags] sandbox strategy registry + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} registry-lifecycle cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} registry-lifecycle-ok + +BuiltInSandboxStrategyAdapter Full Lifecycle + [Documentation] Verify adapter create/read/write/diff/cleanup with real filesystem + [Tags] sandbox strategy adapter + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} adapter-lifecycle cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} adapter-lifecycle-ok + +Adapter Checkpoint And Restore + [Documentation] Verify adapter checkpoint save and restore operations + [Tags] sandbox strategy adapter checkpoint + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} adapter-checkpoint cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} adapter-checkpoint-ok + +CustomStrategyConfig Validation + [Documentation] Verify CustomStrategyConfig rejects empty name, module, class_name + [Tags] sandbox strategy config + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} config-validation cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} config-validation-ok + +SandboxFactory Custom Strategy Integration + [Documentation] Verify SandboxFactory with custom registry lookup + [Tags] sandbox strategy factory + ${result}= Run Process ${PYTHON} ${HELPER_SCRIPT} factory-custom-integration cwd=${WORKSPACE} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} factory-custom-integration-ok diff --git a/robot/fixtures/tdd_bug_alone.robot b/robot/fixtures/tdd_bug_alone.robot new file mode 100644 index 00000000..3a186a8a --- /dev/null +++ b/robot/fixtures/tdd_bug_alone.robot @@ -0,0 +1,16 @@ +*** Settings *** +Documentation Fixture: a test with only tdd_bug (no tdd_bug_N or +... tdd_expected_fail). +... Used by helper_tdd_tag_validation.py to verify that +... tdd_bug alone is a valid tag combination — the listener +... should NOT modify the test result. +... This file must NOT be picked up by the main pabot runner — +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +TDD Bug Tag Alone Is Valid + [Tags] tdd_bug tdd_fixture + [Documentation] A test with only tdd_bug should pass normally. + ... The listener should not interfere — tdd_bug alone is valid. + Log Test with tdd_bug alone — should pass without modification. + Should Be True ${TRUE} diff --git a/robot/fixtures/tdd_expected_fail_alone.robot b/robot/fixtures/tdd_expected_fail_alone.robot new file mode 100644 index 00000000..1b7048e4 --- /dev/null +++ b/robot/fixtures/tdd_expected_fail_alone.robot @@ -0,0 +1,15 @@ +*** Settings *** +Documentation Fixture: a test with only tdd_expected_fail (both tdd_bug +... and tdd_bug_N are missing). +... Used by helper_tdd_tag_validation.py to verify tag +... validation catches both missing prerequisite tags. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Expected Fail Alone Causes Validation Error + [Tags] tdd_expected_fail tdd_fixture + [Documentation] This test has tdd_expected_fail but neither tdd_bug nor + ... tdd_bug_N. The listener should detect this and force a validation + ... failure mentioning both missing tags. + Log This should not matter -- validation should fail first. diff --git a/robot/fixtures/tdd_expected_fail_fails.robot b/robot/fixtures/tdd_expected_fail_fails.robot new file mode 100644 index 00000000..74d18ee3 --- /dev/null +++ b/robot/fixtures/tdd_expected_fail_fails.robot @@ -0,0 +1,13 @@ +*** Settings *** +Documentation Fixture: a tdd_expected_fail test that deliberately fails. +... Used by helper_tdd_tag_validation.py to verify result inversion. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Bug 999 Expected Failure Is Inverted To Pass + [Tags] tdd_bug tdd_bug_999 tdd_expected_fail tdd_fixture + [Documentation] This test deliberately fails to simulate a bug still + ... being present. The tdd_expected_fail listener should invert this + ... failure into a pass. + Fail Deliberate failure: bug 999 is still present (expected by TDD tag) diff --git a/robot/fixtures/tdd_expected_fail_missing_bug_n.robot b/robot/fixtures/tdd_expected_fail_missing_bug_n.robot new file mode 100644 index 00000000..a6cd7232 --- /dev/null +++ b/robot/fixtures/tdd_expected_fail_missing_bug_n.robot @@ -0,0 +1,15 @@ +*** Settings *** +Documentation Fixture: a test with tdd_expected_fail and tdd_bug but +... missing tdd_bug_N. +... Used by helper_tdd_tag_validation.py to verify tag +... validation catches the missing tdd_bug_N tag. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Missing TDD Bug N Tag Causes Validation Error + [Tags] tdd_bug tdd_expected_fail tdd_fixture + [Documentation] This test has tdd_expected_fail and tdd_bug but no + ... tdd_bug_N tag. The listener should detect this and force a + ... validation failure. + Log This should not matter -- validation should fail first. diff --git a/robot/fixtures/tdd_expected_fail_passes.robot b/robot/fixtures/tdd_expected_fail_passes.robot new file mode 100644 index 00000000..070b94f6 --- /dev/null +++ b/robot/fixtures/tdd_expected_fail_passes.robot @@ -0,0 +1,14 @@ +*** Settings *** +Documentation Fixture: a tdd_expected_fail test that unexpectedly passes. +... Used by helper_tdd_tag_validation.py to verify that an +... unexpected pass is inverted to a failure with guidance. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Bug 998 Unexpected Pass Is Inverted To Fail + [Tags] tdd_bug tdd_bug_998 tdd_expected_fail tdd_fixture + [Documentation] This test passes, simulating a bug that was fixed + ... without removing the tdd_expected_fail tag. The listener should + ... invert this pass into a fail with guidance message. + Log This test passes unexpectedly (bug appears fixed). diff --git a/robot/fixtures/tdd_expected_fail_skip.robot b/robot/fixtures/tdd_expected_fail_skip.robot new file mode 100644 index 00000000..af4e4e23 --- /dev/null +++ b/robot/fixtures/tdd_expected_fail_skip.robot @@ -0,0 +1,14 @@ +*** Settings *** +Documentation Fixture: a tdd_expected_fail test that is skipped. +... Used by helper_tdd_tag_validation.py to verify that a +... skipped test is left unchanged by the listener (SKIP is +... not evidence of a fix). +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Bug 997 Skipped Expected Fail Test Stays Skip + [Tags] tdd_bug tdd_bug_997 tdd_expected_fail tdd_fixture + [Documentation] This test skips intentionally to verify the listener + ... leaves SKIP status unchanged for tdd_expected_fail tests. + Skip Skipping intentionally to test SKIP handling diff --git a/robot/fixtures/tdd_missing_tdd_bug.robot b/robot/fixtures/tdd_missing_tdd_bug.robot new file mode 100644 index 00000000..a3db23f8 --- /dev/null +++ b/robot/fixtures/tdd_missing_tdd_bug.robot @@ -0,0 +1,13 @@ +*** Settings *** +Documentation Fixture: a test with tdd_bug_N but missing tdd_bug. +... Used by helper_tdd_tag_validation.py to verify tag +... validation catches the missing tdd_bug tag. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Missing TDD Bug Tag Causes Validation Error + [Tags] tdd_bug_123 tdd_fixture + [Documentation] This test has tdd_bug_123 but no tdd_bug tag. + ... The listener should detect this and force a validation failure. + Log This should not matter -- validation should fail first. diff --git a/robot/fixtures/tdd_normal_test.robot b/robot/fixtures/tdd_normal_test.robot new file mode 100644 index 00000000..498e89db --- /dev/null +++ b/robot/fixtures/tdd_normal_test.robot @@ -0,0 +1,14 @@ +*** Settings *** +Documentation Fixture: a normal test with no TDD tags. +... Used by helper_tdd_tag_validation.py to verify that +... normal tests are completely unaffected by the listener. +... This file must NOT be picked up by the main pabot runner -- +... it is excluded via the tdd_fixture tag. + +*** Test Cases *** +Normal Test Unaffected By Listener + [Tags] tdd_fixture + [Documentation] A normal test that should pass without interference + ... from the TDD expected-fail listener. + Log Normal test execution -- no TDD tags present. + Should Be True ${TRUE} diff --git a/robot/helper_custom_sandbox_strategy.py b/robot/helper_custom_sandbox_strategy.py new file mode 100644 index 00000000..86da6ab3 --- /dev/null +++ b/robot/helper_custom_sandbox_strategy.py @@ -0,0 +1,439 @@ +"""Helper utilities for custom sandbox strategy Robot integration tests. + +Covers end-to-end integration of: +- SandboxStrategyProtocol and runtime checking +- SandboxStrategyRegistry registration and lookup +- BuiltInSandboxStrategyAdapter full lifecycle +- CustomStrategyConfig validation +- SandboxFactory custom strategy integration + +Based on issue #586. +""" + +from __future__ import annotations + +import os +import sys +import tempfile +from datetime import datetime + +from cleveragents.domain.models.core.resource import ( + PhysVirt, + Resource, + ResourceCapabilities, +) +from cleveragents.domain.models.core.resource import ( + SandboxStrategy as SandboxStrategyEnum, +) +from cleveragents.domain.models.core.sandbox_strategy import ( + DiffEntry, + DiffView, + SandboxRef, + SandboxStrategyProtocol, +) +from cleveragents.infrastructure.plugins.exceptions import ( + ProtocolMismatchError, +) +from cleveragents.infrastructure.sandbox.factory import SandboxFactory +from cleveragents.infrastructure.sandbox.strategy_adapter import ( + BuiltInSandboxStrategyAdapter, +) +from cleveragents.infrastructure.sandbox.strategy_registry import ( + CustomStrategyConfig, + SandboxStrategyRegistry, +) + +# --------------------------------------------------------------------------- +# ULID helper +# --------------------------------------------------------------------------- + +_CTR = 0 + + +def _ulid(name: str) -> str: + global _CTR + _CTR += 1 + base = abs(hash(name)) % (10**20) + raw = f"{_CTR:06d}{base:020d}" + charset = "0123456789ABCDEFGHJKMNPQRSTVWXYZ" + result = "" + for ch in raw: + result += charset[int(ch)] + return (result + "0" * 26)[:26] + + +def _make_resource(name: str, location: str) -> Resource: + return Resource( + resource_id=_ulid(name), + name=name, + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.NONE, + location=location, + capabilities=ResourceCapabilities( + readable=True, + writable=True, + sandboxable=True, + checkpointable=False, + ), + ) + + +# --------------------------------------------------------------------------- +# Test: SandboxRef creation and immutability +# --------------------------------------------------------------------------- + + +def _sandbox_ref_lifecycle() -> None: + """Integration test: SandboxRef creation and properties.""" + ref = SandboxRef( + sandbox_id="ref-001", + plan_id="plan-001", + resource_id="res-001", + created_at=datetime.now(), + metadata={"backend": "test"}, + ) + assert ref.sandbox_id == "ref-001" + assert ref.plan_id == "plan-001" + assert ref.metadata["backend"] == "test" + + # Frozen check + try: + ref.sandbox_id = "mutated" # type: ignore[misc] + print("FAIL: Expected FrozenInstanceError") + return + except (AttributeError, Exception): + pass + + print("sandbox-ref-lifecycle-ok") + + +# --------------------------------------------------------------------------- +# Test: DiffView and DiffEntry +# --------------------------------------------------------------------------- + + +def _diff_view_lifecycle() -> None: + """Integration test: DiffView and DiffEntry models.""" + entry = DiffEntry(path="test.py", operation="added", after=b"content") + assert entry.path == "test.py" + assert entry.operation == "added" + + view = DiffView( + sandbox_id="dv-001", + entries=[entry], + summary="1 file added", + ) + assert len(view.entries) == 1 + assert view.sandbox_id == "dv-001" + + # Empty path should fail + try: + DiffEntry(path="", operation="modified") + print("FAIL: Expected ValidationError") + return + except Exception: + pass + + print("diff-view-lifecycle-ok") + + +# --------------------------------------------------------------------------- +# Test: SandboxStrategyProtocol runtime check +# --------------------------------------------------------------------------- + + +def _protocol_check() -> None: + """Integration test: Protocol runtime checking.""" + + class _Good: + def create(self, plan_id, resource): + return None + + def read(self, ref, path): + return b"" + + def write(self, ref, path, content): + return None + + def diff(self, ref): + return None + + def commit(self, ref): + pass + + def rollback(self, ref): + pass + + def checkpoint(self, ref, checkpoint_id): + pass + + def restore_checkpoint(self, ref, checkpoint_id): + pass + + def cleanup(self, ref): + pass + + class _Bad: + def create(self, plan_id, resource): + return None + + good = _Good() + bad = _Bad() + assert isinstance(good, SandboxStrategyProtocol) + assert not isinstance(bad, SandboxStrategyProtocol) + + print("protocol-check-ok") + + +# --------------------------------------------------------------------------- +# Test: SandboxStrategyRegistry +# --------------------------------------------------------------------------- + + +def _registry_lifecycle() -> None: + """Integration test: Registry registration, lookup, config, clear.""" + registry = SandboxStrategyRegistry(allowed_prefixes=("cleveragents.",)) + + # Register + cls = registry.register( + "adapter", + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + assert cls is BuiltInSandboxStrategyAdapter + assert registry.has("adapter") + assert "adapter" in registry.list_strategies() + + # Get + assert registry.get("adapter") is BuiltInSandboxStrategyAdapter + assert registry.get("nonexistent") is None + + # Non-protocol rejection + try: + registry.register( + "bad", + "cleveragents.infrastructure.sandbox.protocol", + "SandboxError", + ) + print("FAIL: Expected ProtocolMismatchError") + return + except ProtocolMismatchError: + pass + + # Config-driven registration + configs = { + "cfg1": { + "module": "cleveragents.infrastructure.sandbox.strategy_adapter", + "class": "BuiltInSandboxStrategyAdapter", + }, + } + registered = registry.register_all_from_config(configs) + assert len(registered) == 1 + + # Clear + registry.clear() + assert len(registry.list_strategies()) == 0 + + print("registry-lifecycle-ok") + + +# --------------------------------------------------------------------------- +# Test: BuiltInSandboxStrategyAdapter full lifecycle +# --------------------------------------------------------------------------- + + +def _adapter_lifecycle() -> None: + """Integration test: Adapter create/read/write/diff/commit/cleanup.""" + with tempfile.TemporaryDirectory() as tmpdir: + # Seed a file + with open(os.path.join(tmpdir, "seed.txt"), "w") as f: + f.write("seed") + + adapter = BuiltInSandboxStrategyAdapter(strategy_name="copy_on_write") + resource = Resource( + resource_id=_ulid("adapter-res"), + name="adapter-res", + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.COPY_ON_WRITE, + location=tmpdir, + capabilities=ResourceCapabilities( + readable=True, + writable=True, + sandboxable=True, + checkpointable=False, + ), + ) + + # Create + ref = adapter.create("plan-adapter", resource) + assert isinstance(ref, SandboxRef) + assert ref.plan_id == "plan-adapter" + + # Write + entry = adapter.write(ref, "new_file.txt", b"hello") + assert isinstance(entry, DiffEntry) + assert entry.operation == "added" + + # Read + data = adapter.read(ref, "new_file.txt") + assert data == b"hello" + + # Diff + view = adapter.diff(ref) + assert isinstance(view, DiffView) + + # Cleanup + adapter.cleanup(ref) + assert ref.sandbox_id not in adapter._sandboxes + + print("adapter-lifecycle-ok") + + +# --------------------------------------------------------------------------- +# Test: Adapter checkpoint/restore +# --------------------------------------------------------------------------- + + +def _adapter_checkpoint() -> None: + """Integration test: Adapter checkpoint and restore.""" + with tempfile.TemporaryDirectory() as tmpdir: + with open(os.path.join(tmpdir, "seed.txt"), "w") as f: + f.write("seed") + + adapter = BuiltInSandboxStrategyAdapter(strategy_name="copy_on_write") + resource = Resource( + resource_id=_ulid("cp-res"), + name="cp-res", + resource_type_name="fs-directory", + classification=PhysVirt.PHYSICAL, + sandbox_strategy=SandboxStrategyEnum.COPY_ON_WRITE, + location=tmpdir, + capabilities=ResourceCapabilities( + readable=True, + writable=True, + sandboxable=True, + checkpointable=False, + ), + ) + + ref = adapter.create("plan-cp", resource) + adapter.write(ref, "cp.txt", b"v1") + adapter.checkpoint(ref, "cp-1") + adapter.write(ref, "cp.txt", b"v2") + + # Verify v2 + assert adapter.read(ref, "cp.txt") == b"v2" + + # Restore + adapter.restore_checkpoint(ref, "cp-1") + assert adapter.read(ref, "cp.txt") == b"v1" + + adapter.cleanup(ref) + + print("adapter-checkpoint-ok") + + +# --------------------------------------------------------------------------- +# Test: CustomStrategyConfig validation +# --------------------------------------------------------------------------- + + +def _config_validation() -> None: + """Integration test: CustomStrategyConfig validation.""" + # Valid + cfg = CustomStrategyConfig( + name="test", module="cleveragents.test", class_name="TestCls" + ) + assert cfg.name == "test" + + # Empty name + try: + CustomStrategyConfig(name="", module="mod", class_name="Cls") + print("FAIL: Expected ValueError for empty name") + return + except ValueError: + pass + + # Empty module + try: + CustomStrategyConfig(name="x", module="", class_name="Cls") + print("FAIL: Expected ValueError for empty module") + return + except ValueError: + pass + + # Empty class + try: + CustomStrategyConfig(name="x", module="mod", class_name="") + print("FAIL: Expected ValueError for empty class") + return + except ValueError: + pass + + print("config-validation-ok") + + +# --------------------------------------------------------------------------- +# Test: SandboxFactory custom strategy integration +# --------------------------------------------------------------------------- + + +def _factory_custom_integration() -> None: + """Integration test: SandboxFactory with custom strategy registry.""" + registry = SandboxStrategyRegistry(allowed_prefixes=("cleveragents.",)) + registry.register( + "my_custom", + "cleveragents.infrastructure.sandbox.strategy_adapter", + "BuiltInSandboxStrategyAdapter", + ) + + factory = SandboxFactory(custom_registry=registry) + assert factory.has_custom_strategy("my_custom") + assert not factory.has_custom_strategy("nonexistent") + + cls = factory.get_custom_strategy_class("my_custom") + assert cls is BuiltInSandboxStrategyAdapter + + # Factory without registry + plain_factory = SandboxFactory() + assert not plain_factory.has_custom_strategy("anything") + assert plain_factory.get_custom_strategy_class("anything") is None + + print("factory-custom-integration-ok") + + +# --------------------------------------------------------------------------- +# Main dispatcher +# --------------------------------------------------------------------------- + +_TESTS = { + "sandbox-ref-lifecycle": _sandbox_ref_lifecycle, + "diff-view-lifecycle": _diff_view_lifecycle, + "protocol-check": _protocol_check, + "registry-lifecycle": _registry_lifecycle, + "adapter-lifecycle": _adapter_lifecycle, + "adapter-checkpoint": _adapter_checkpoint, + "config-validation": _config_validation, + "factory-custom-integration": _factory_custom_integration, +} + + +def main() -> None: + if len(sys.argv) < 2: + print(f"Usage: {sys.argv[0]} ") + print(f"Available: {', '.join(sorted(_TESTS))}") + sys.exit(1) + + test_name = sys.argv[1] + if test_name not in _TESTS: + print(f"Unknown test: {test_name}") + print(f"Available: {', '.join(sorted(_TESTS))}") + sys.exit(1) + + _TESTS[test_name]() + + +if __name__ == "__main__": + main() diff --git a/robot/helper_tdd_tag_validation.py b/robot/helper_tdd_tag_validation.py index 7b218c65..93d6f19a 100644 --- a/robot/helper_tdd_tag_validation.py +++ b/robot/helper_tdd_tag_validation.py @@ -1,22 +1,32 @@ -"""Helper script for tdd_tag_validation.robot integration tests. +"""Helper for ``tdd_tag_validation.robot`` — exercises TDD bug-capture tags. -Each subcommand is a self-contained check that exercises the TDD -bug-capture tag validation and result-inversion logic defined in -``features/environment.py``. Sentinels are printed on success so -the Robot test case can assert correct behaviour. +Behave-side sub-commands (``validate_tags_*``, ``should_invert_*``, +``inversion_*``) test the logic in ``features/environment.py``. +Robot-side sub-commands run fixture ``.robot`` files as sub-processes +with the ``tdd_expected_fail_listener`` and inspect the output XML. + +Exit 0 = check passed, 1 = unexpected outcome. +See CONTRIBUTING.md > TDD Bug Test Tags. """ from __future__ import annotations +__all__: list[str] = [] + import sys +from collections.abc import Callable from pathlib import Path -from unittest.mock import MagicMock # Ensure the project root is importable so ``features.environment`` resolves. _ROOT = str(Path(__file__).resolve().parents[1]) if _ROOT not in sys.path: sys.path.insert(0, _ROOT) +_ROBOT_DIR = str(Path(__file__).resolve().parent) +if _ROBOT_DIR not in sys.path: + sys.path.insert(0, _ROBOT_DIR) + +from _tdd_fixture_runner import run_fixture, run_multi_fixture # noqa: E402 from behave.model import Status # noqa: E402 from features.environment import ( # noqa: E402 _UNEXPECTED_PASS_MSG, @@ -24,47 +34,10 @@ from features.environment import ( # noqa: E402 should_invert_result, validate_tdd_tags, ) - -# --------------------------------------------------------------------------- -# Mock helpers -# --------------------------------------------------------------------------- +from features.mocks.tdd_test_helpers import make_mock_scenario # noqa: E402 -def _make_mock_scenario( - tags: list[str], - steps_passed: bool = True, - hook_failed: bool = False, - was_dry_run: bool = False, - step_exception: BaseException | None = None, -) -> MagicMock: - """Build a lightweight mock ``Scenario`` for ``apply_tdd_inversion``.""" - scenario = MagicMock() - scenario.effective_tags = tags - scenario.name = "mock-scenario" - scenario.hook_failed = hook_failed - scenario.was_dry_run = was_dry_run - - mock_step = MagicMock() - if steps_passed: - mock_step.status = Status.passed - mock_step.exception = None - else: - mock_step.status = Status.failed - mock_step.exception = ( - step_exception - if step_exception is not None - else AssertionError("simulated assertion failure") - ) - scenario.all_steps = [mock_step] - return scenario - - -# --------------------------------------------------------------------------- -# Subcommands — validate_tdd_tags -# --------------------------------------------------------------------------- - - -def validate_tags_valid_combos() -> None: +def validate_tags_valid_combos() -> int: """Verify valid tag combinations raise no error.""" valid_sets: list[set[str]] = [ {"tdd_bug", "tdd_bug_42"}, @@ -81,101 +54,95 @@ def validate_tags_valid_combos() -> None: f"FAIL: valid tag set {tag_set} raised ValueError: {exc}", file=sys.stderr, ) - sys.exit(1) + return 1 print("validate-tags-valid-combos-ok") + return 0 -def validate_tags_bug_n_without_bug() -> None: +def validate_tags_bug_n_without_bug() -> int: """Verify @tdd_bug_ without @tdd_bug raises ValueError.""" try: validate_tdd_tags({"tdd_bug_42"}) except ValueError: print("validate-tags-bug-n-without-bug-ok") - return + return 0 print("FAIL: expected ValueError for tdd_bug_42 without tdd_bug", file=sys.stderr) - sys.exit(1) + return 1 -def validate_tags_expected_fail_missing_bug() -> None: +def validate_tags_expected_fail_missing_bug() -> int: """Verify @tdd_expected_fail without @tdd_bug raises ValueError.""" try: validate_tdd_tags({"tdd_expected_fail", "tdd_bug_42"}) except ValueError: print("validate-tags-expected-fail-missing-bug-ok") - return + return 0 print( "FAIL: expected ValueError for tdd_expected_fail without tdd_bug", file=sys.stderr, ) - sys.exit(1) + return 1 -def validate_tags_expected_fail_missing_bug_n() -> None: +def validate_tags_expected_fail_missing_bug_n() -> int: """Verify @tdd_expected_fail without @tdd_bug_ raises ValueError.""" try: validate_tdd_tags({"tdd_expected_fail", "tdd_bug"}) except ValueError: print("validate-tags-expected-fail-missing-bug-n-ok") - return + return 0 print( "FAIL: expected ValueError for tdd_expected_fail without tdd_bug_", file=sys.stderr, ) - sys.exit(1) + return 1 -# --------------------------------------------------------------------------- -# Subcommands — should_invert_result -# --------------------------------------------------------------------------- - - -def should_invert_with_expected_fail() -> None: +def should_invert_with_expected_fail() -> int: """Verify should_invert_result returns True when @tdd_expected_fail present.""" result = should_invert_result({"tdd_bug", "tdd_bug_42", "tdd_expected_fail"}) if result is not True: print(f"FAIL: expected True, got {result}", file=sys.stderr) - sys.exit(1) + return 1 print("should-invert-with-expected-fail-ok") + return 0 -def should_not_invert_without_expected_fail() -> None: +def should_not_invert_without_expected_fail() -> int: """Verify should_invert_result returns False without @tdd_expected_fail.""" result = should_invert_result({"tdd_bug", "tdd_bug_42"}) if result is not False: print(f"FAIL: expected False, got {result}", file=sys.stderr) - sys.exit(1) + return 1 print("should-not-invert-without-expected-fail-ok") + return 0 -# --------------------------------------------------------------------------- -# Subcommands — apply_tdd_inversion -# --------------------------------------------------------------------------- - - -def inversion_expected_fail() -> None: +def inversion_expected_fail() -> int: """Verify expected failure (failed=True) is inverted to passed.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42", "tdd_expected_fail"], steps_passed=False, ) result = apply_tdd_inversion(scenario, failed=True) if result is not False: print(f"FAIL: expected False (inverted), got {result}", file=sys.stderr) - sys.exit(1) + return 1 scenario.set_status.assert_called_with(Status.passed) print("inversion-expected-fail-ok") + return 0 -def inversion_unexpected_pass() -> None: +def inversion_unexpected_pass() -> int: """Verify unexpected pass (failed=False) is inverted to failure.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42", "tdd_expected_fail"], steps_passed=True, ) result = apply_tdd_inversion(scenario, failed=False) if result is not True: print(f"FAIL: expected True (forced failure), got {result}", file=sys.stderr) - sys.exit(1) + return 1 scenario.set_status.assert_called_with(Status.failed) # Verify synthetic error is attached to last step last_step = scenario.all_steps[-1] @@ -184,22 +151,23 @@ def inversion_unexpected_pass() -> None: f"FAIL: last step status should be failed, got {last_step.status}", file=sys.stderr, ) - sys.exit(1) + return 1 if not isinstance(last_step.exception, AssertionError): print( f"FAIL: expected AssertionError, got {type(last_step.exception)}", file=sys.stderr, ) - sys.exit(1) + return 1 if str(last_step.exception) != _UNEXPECTED_PASS_MSG: print(f"FAIL: unexpected message: {last_step.exception}", file=sys.stderr) - sys.exit(1) + return 1 print("inversion-unexpected-pass-ok") + return 0 -def inversion_hook_error_guard() -> None: +def inversion_hook_error_guard() -> int: """Verify hook_failed prevents inversion.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42", "tdd_expected_fail"], steps_passed=False, hook_failed=True, @@ -207,13 +175,14 @@ def inversion_hook_error_guard() -> None: result = apply_tdd_inversion(scenario, failed=True) if result is not True: print(f"FAIL: expected True (not inverted), got {result}", file=sys.stderr) - sys.exit(1) + return 1 print("inversion-hook-error-guard-ok") + return 0 -def inversion_dry_run_guard() -> None: +def inversion_dry_run_guard() -> int: """Verify dry-run mode prevents inversion.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42", "tdd_expected_fail"], steps_passed=True, was_dry_run=True, @@ -221,13 +190,14 @@ def inversion_dry_run_guard() -> None: result = apply_tdd_inversion(scenario, failed=False) if result is not False: print(f"FAIL: expected False (not inverted), got {result}", file=sys.stderr) - sys.exit(1) + return 1 print("inversion-dry-run-guard-ok") + return 0 -def inversion_non_assertion_guard() -> None: +def inversion_non_assertion_guard() -> int: """Verify non-AssertionError exceptions prevent inversion.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42", "tdd_expected_fail"], steps_passed=False, step_exception=RuntimeError("connection lost"), @@ -235,30 +205,257 @@ def inversion_non_assertion_guard() -> None: result = apply_tdd_inversion(scenario, failed=True) if result is not True: print(f"FAIL: expected True (not inverted), got {result}", file=sys.stderr) - sys.exit(1) + return 1 print("inversion-non-assertion-guard-ok") + return 0 -def inversion_no_tag_passthrough() -> None: +def inversion_no_tag_passthrough() -> int: """Verify scenarios without @tdd_expected_fail are not modified.""" - scenario = _make_mock_scenario( + scenario = make_mock_scenario( tags=["tdd_bug", "tdd_bug_42"], steps_passed=False, ) result = apply_tdd_inversion(scenario, failed=True) if result is not True: print(f"FAIL: expected True (passthrough), got {result}", file=sys.stderr) - sys.exit(1) + return 1 # set_status should NOT have been called scenario.set_status.assert_not_called() print("inversion-no-tag-passthrough-ok") + return 0 -# --------------------------------------------------------------------------- -# Dispatch -# --------------------------------------------------------------------------- +def cmd_expected_fail_inverted() -> int: + """Verify that a tdd_expected_fail test that fails is inverted to PASS.""" + status, message = run_fixture("tdd_expected_fail_fails") + if status == "PASS" and "TDD expected failure" in message: + print("tdd-expected-fail-inverted-ok") + return 0 + print( + f"FAIL: Expected PASS with 'TDD expected failure' message " + f"but got {status}. Message: {message}", + file=sys.stderr, + ) + return 1 -_COMMANDS = { + +def cmd_unexpected_pass_inverted() -> int: + """Verify that a tdd_expected_fail test that passes is inverted to FAIL.""" + status, message = run_fixture("tdd_expected_fail_passes") + if status == "FAIL" and "Bug appears to be fixed" in message: + print("tdd-unexpected-pass-inverted-ok") + return 0 + print( + f"FAIL: Expected FAIL with guidance but got {status}. Message: {message}", + file=sys.stderr, + ) + return 1 + + +def cmd_missing_tdd_bug_validation() -> int: + """Verify that tdd_bug_N without tdd_bug causes a validation error.""" + status, message = run_fixture("tdd_missing_tdd_bug") + if status == "FAIL" and "missing the required tdd_bug tag" in message: + print("tdd-missing-tdd-bug-validation-ok") + return 0 + print( + f"FAIL: Expected FAIL with 'missing the required tdd_bug tag' " + f"but got {status}. Message: {message}", + file=sys.stderr, + ) + return 1 + + +def cmd_missing_bug_n_validation() -> int: + """Verify that tdd_expected_fail without tdd_bug_N causes validation error.""" + status, message = run_fixture("tdd_expected_fail_missing_bug_n") + if status == "FAIL" and "tdd_bug_" in message: + print("tdd-missing-bug-n-validation-ok") + return 0 + print( + f"FAIL: Expected FAIL with validation error mentioning " + f"tdd_bug_ but got {status}. Message: {message}", + file=sys.stderr, + ) + return 1 + + +def cmd_normal_test_unaffected() -> int: + """Verify that a normal test (no TDD tags) is unaffected by a loaded listener. + + Runs the normal-test fixture together with a tdd_expected_fail fixture + in a single Robot invocation. This proves the listener is loaded + (the expected-fail fixture is inverted) but does NOT modify the normal + test. + """ + results = run_multi_fixture( + "tdd_normal_test", + "tdd_expected_fail_fails", + ) + + # The normal test must be PASS. + normal_name = "Normal Test Unaffected By Listener" + if normal_name not in results: + print( + f"FAIL: Normal test '{normal_name}' not found in results. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + normal_status, normal_msg = results[normal_name] + if normal_status != "PASS": + print( + f"FAIL: Expected normal test PASS but got {normal_status}. " + f"Message: {normal_msg}", + file=sys.stderr, + ) + return 1 + + # The expected-fail fixture must have been inverted to PASS (proving + # the listener is loaded and actively processing). + inverted_name = "Bug 999 Expected Failure Is Inverted To Pass" + if inverted_name not in results: + print( + f"FAIL: Expected-fail fixture '{inverted_name}' not found. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + inv_status, _ = results[inverted_name] + if inv_status != "PASS": + print( + f"FAIL: Expected-fail fixture was NOT inverted to PASS " + f"(got {inv_status}). Listener may not be loaded.", + file=sys.stderr, + ) + return 1 + + print("tdd-normal-test-unaffected-ok") + return 0 + + +def cmd_skip_status_unchanged() -> int: + """Verify that a skipped tdd_expected_fail test stays SKIP. + + Runs alongside a tdd_expected_fail fixture to prove the listener + is loaded and selectively applies — the SKIP test must stay SKIP + while the companion is correctly inverted to PASS. + """ + results = run_multi_fixture( + "tdd_expected_fail_skip", + "tdd_expected_fail_fails", + ) + + # The SKIP test must remain SKIP. + skip_name = "Bug 997 Skipped Expected Fail Test Stays Skip" + if skip_name not in results: + print( + f"FAIL: Skip fixture '{skip_name}' not found. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + skip_status, skip_msg = results[skip_name] + if skip_status != "SKIP": + print( + f"FAIL: Expected SKIP but got {skip_status}. Message: {skip_msg}", + file=sys.stderr, + ) + return 1 + + # The expected-fail companion must be inverted to PASS (proving + # the listener is loaded and actively processing). + inverted_name = "Bug 999 Expected Failure Is Inverted To Pass" + if inverted_name not in results: + print( + f"FAIL: Companion fixture '{inverted_name}' not found. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + inv_status, _ = results[inverted_name] + if inv_status != "PASS": + print( + f"FAIL: Companion fixture was NOT inverted to PASS " + f"(got {inv_status}). Listener may not be loaded.", + file=sys.stderr, + ) + return 1 + + print("tdd-skip-status-unchanged-ok") + return 0 + + +def cmd_expected_fail_alone_validation() -> int: + """Verify tdd_expected_fail alone (no tdd_bug or tdd_bug_N) fails validation.""" + status, message = run_fixture("tdd_expected_fail_alone") + if status == "FAIL" and "tdd_bug" in message and "tdd_bug_" in message: + print("tdd-expected-fail-alone-validation-ok") + return 0 + print( + f"FAIL: Expected FAIL mentioning both tdd_bug and tdd_bug_ " + f"but got {status}. Message: {message}", + file=sys.stderr, + ) + return 1 + + +def cmd_tdd_bug_alone_valid() -> int: + """Verify tdd_bug alone (no tdd_bug_N or tdd_expected_fail) is valid. + + Runs alongside a tdd_expected_fail fixture to prove the listener + is loaded — the tdd_bug-alone test must stay PASS while the + companion is correctly inverted. + """ + results = run_multi_fixture( + "tdd_bug_alone", + "tdd_expected_fail_fails", + ) + + # The tdd_bug-alone test must be PASS (listener should not modify). + alone_name = "TDD Bug Tag Alone Is Valid" + if alone_name not in results: + print( + f"FAIL: tdd_bug-alone fixture '{alone_name}' not found. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + alone_status, alone_msg = results[alone_name] + if alone_status != "PASS": + print( + f"FAIL: Expected PASS for tdd_bug-alone test but got " + f"{alone_status}. Message: {alone_msg}", + file=sys.stderr, + ) + return 1 + + # The expected-fail companion must be inverted to PASS (proving + # the listener is loaded and actively processing). + inverted_name = "Bug 999 Expected Failure Is Inverted To Pass" + if inverted_name not in results: + print( + f"FAIL: Companion fixture '{inverted_name}' not found. " + f"Available: {list(results.keys())}", + file=sys.stderr, + ) + return 1 + inv_status, _ = results[inverted_name] + if inv_status != "PASS": + print( + f"FAIL: Companion fixture was NOT inverted to PASS " + f"(got {inv_status}). Listener may not be loaded.", + file=sys.stderr, + ) + return 1 + + print("tdd-bug-alone-valid-ok") + return 0 + + +_COMMANDS: dict[str, Callable[[], int]] = { + # Behave-side commands "validate_tags_valid_combos": validate_tags_valid_combos, "validate_tags_bug_n_without_bug": validate_tags_bug_n_without_bug, "validate_tags_expected_fail_missing_bug": validate_tags_expected_fail_missing_bug, @@ -273,11 +470,30 @@ _COMMANDS = { "inversion_dry_run_guard": inversion_dry_run_guard, "inversion_non_assertion_guard": inversion_non_assertion_guard, "inversion_no_tag_passthrough": inversion_no_tag_passthrough, + # Robot-side commands + "expected-fail-inverted": cmd_expected_fail_inverted, + "unexpected-pass-inverted": cmd_unexpected_pass_inverted, + "missing-tdd-bug-validation": cmd_missing_tdd_bug_validation, + "missing-bug-n-validation": cmd_missing_bug_n_validation, + "normal-test-unaffected": cmd_normal_test_unaffected, + "skip-status-unchanged": cmd_skip_status_unchanged, + "expected-fail-alone-validation": cmd_expected_fail_alone_validation, + "tdd-bug-alone-valid": cmd_tdd_bug_alone_valid, } -if __name__ == "__main__": + +def main() -> None: + """Dispatch to the requested sub-command.""" if len(sys.argv) < 2 or sys.argv[1] not in _COMMANDS: - print(f"Usage: {sys.argv[0]} ", file=sys.stderr) - print(f"Commands: {list(_COMMANDS)}", file=sys.stderr) + print( + f"Usage: {sys.argv[0]} <{'|'.join(_COMMANDS)}>", + file=sys.stderr, + ) sys.exit(1) - _COMMANDS[sys.argv[1]]() + + handler = _COMMANDS[sys.argv[1]] + sys.exit(handler()) + + +if __name__ == "__main__": + main() diff --git a/robot/tdd_expected_fail_listener.py b/robot/tdd_expected_fail_listener.py index d89f9ced..a66ece87 100644 --- a/robot/tdd_expected_fail_listener.py +++ b/robot/tdd_expected_fail_listener.py @@ -1,48 +1,262 @@ -"""Robot Framework listener that inverts results for ``tdd_expected_fail`` tests. +"""Robot Framework Listener v3 for TDD bug-capture tag handling. -When a test is tagged ``tdd_expected_fail``, the listener treats a FAIL as a -PASS (the bug still exists, which is expected) and a PASS as a FAIL (the bug -was fixed but the tag was not removed). +Implements the three-tag TDD bug-capture system for Robot Framework +integration tests, paralleling the Behave implementation in +``features/environment.py``. -See CONTRIBUTING.md § TDD Bug Test Tags for the full convention. +Three-Tag System (see CONTRIBUTING.md > TDD Bug Test Tags) +----------------------------------------------------------- +``tdd_bug`` + Generic filter tag. Present on ALL TDD bug tests. Used to list, + filter, and count TDD bug tests across the codebase (e.g., + ``--include tdd_bug``). -Usage:: +``tdd_bug_`` + Issue reference tag (e.g., ``tdd_bug_123``). Links the test to the + specific ``Type/Bug`` issue it captures. N is the bug issue number. + Permanent -- never removed. - pabot ... --listener robot/tdd_expected_fail_listener.py ... +``tdd_expected_fail`` + Behavioral switch. When present, the test result is inverted: -This listener uses the Robot Framework Listener API v3. + * A **failing** test (bug still exists) is reported as **passed**. + * A **passing** test (bug was fixed without removing the tag) is + reported as **failed** with a clear guidance message. + + Temporary -- removed by the bug-fix developer when the fix is + implemented. + +Tag Validation Rules +-------------------- +* ``tdd_bug_`` requires ``tdd_bug`` to also be present. +* ``tdd_expected_fail`` requires both ``tdd_bug`` and at least one + ``tdd_bug_``. + +Implementation Notes +-------------------- +The listener uses the Robot Framework Listener v3 API with module-level +``start_test`` and ``end_test`` functions. Tag validation happens in +``start_test``; result inversion happens in ``end_test``. Validation +errors are stored in ``_validation_errors`` keyed by test ``full_name`` +and applied in ``end_test`` to ensure the error message is visible in +reports. + +Tags are read from ``data.tags`` (the static test definition) rather +than ``result.tags``. This is intentional: TDD tags are declarative +metadata that must be present in the source file. Runtime tag +modification via ``Set Tags`` / ``Remove Tags`` keywords is not +supported for TDD tags — they must be statically declared. + +An idempotency guard (``_processed_tests``) prevents double-inversion +if the listener is loaded more than once (e.g., ``--listener`` specified +both in noxfile.py and via user arguments). + +Registration: The listener is registered via ``--listener`` in the nox +``integration_tests`` and ``slow_integration_tests`` sessions. """ from __future__ import annotations -from typing import Any +import logging +import re + +from robot.result.model import TestCase as ResultTestCase +from robot.running.model import TestCase as RunningTestCase ROBOT_LISTENER_API_VERSION = 3 -_TAG = "tdd_expected_fail" +_TDD_BUG_N_RE = re.compile(r"tdd_bug_\d+") + +_logger = logging.getLogger("cleveragents.testing.robot_tdd_tags") + +# Validation errors detected in start_test are stored here keyed by test +# longname so that end_test can override whatever result the test produced. +_validation_errors: dict[str, str] = {} + +# Idempotency guard: prevents double-invocation if the listener is loaded +# more than once in the same process (e.g., --listener specified twice or +# via both noxfile and user args). Each test is processed at most once. +_processed_tests: set[str] = set() -def end_test(data: Any, result: Any) -> None: - """Invert the result of tests tagged ``tdd_expected_fail``.""" - tags = getattr(result, "tags", []) - if _TAG not in tags: +def _validate_tdd_tags(tags: set[str]) -> str | None: + """Validate TDD bug-capture tag combinations. + + Returns an error message string if the tag set is inconsistent, or + ``None`` if the tags are valid. + + Design note: this function returns ``str | None`` rather than raising + ``ValueError`` (as the Behave counterpart ``validate_tdd_tags`` does) + because the Robot Framework Listener v3 API does not propagate + exceptions cleanly from ``start_test``. Returning a string allows + ``start_test`` to store the error for ``end_test`` to apply as a test + failure, giving better diagnostics in the Robot output XML. + + Rules (from CONTRIBUTING.md > TDD Bug Test Tags): + * ``tdd_bug_`` requires ``tdd_bug`` to also be present. + * ``tdd_expected_fail`` requires both ``tdd_bug`` and at least one + ``tdd_bug_``. + + Args: + tags: The tags on the test case (lowercased). + + Returns: + An error message string if validation fails, ``None`` otherwise. + """ + has_tdd_bug = "tdd_bug" in tags + has_tdd_bug_n = any(_TDD_BUG_N_RE.fullmatch(t) for t in tags) + has_expected_fail = "tdd_expected_fail" in tags + + # Note: error messages reference tag names without the ``@`` prefix used + # by Behave (e.g., ``tdd_bug`` not ``@tdd_bug``). Robot Framework tags + # do not use the ``@`` prefix — this divergence from the Behave error + # messages is intentional and matches each framework's convention. + if has_tdd_bug_n and not has_tdd_bug: + bug_n_tags = sorted(t for t in tags if _TDD_BUG_N_RE.fullmatch(t)) + return ( + f"Test has {', '.join(bug_n_tags)} but is missing the required " + f"tdd_bug tag. All TDD bug tests must include tdd_bug. " + f"See CONTRIBUTING.md > TDD Bug Test Tags." + ) + + if has_expected_fail: + missing: list[str] = [] + if not has_tdd_bug: + missing.append("tdd_bug") + if not has_tdd_bug_n: + missing.append("tdd_bug_") + if missing: + return ( + f"Test has tdd_expected_fail but is missing required " + f"tag(s): {', '.join(missing)}. tdd_expected_fail requires " + f"both tdd_bug and at least one tdd_bug_. " + f"See CONTRIBUTING.md > TDD Bug Test Tags." + ) + + return None + + +def _should_invert_result(tags: set[str]) -> bool: + """Return ``True`` if the test result should be inverted. + + A test result is inverted when ``tdd_expected_fail`` is present, + indicating the test captures a known bug that has not yet been fixed. + + Args: + tags: The tags on the test case (lowercased). + """ + return "tdd_expected_fail" in tags + + +def start_test( + data: RunningTestCase, + result: ResultTestCase, +) -> None: + """Validate TDD tags before the test runs. + + If validation fails, the error is stored for ``end_test`` to apply. + The test is allowed to execute so that ``end_test`` can properly + override the result. + """ + tags = {str(t).lower() for t in data.tags} + error = _validate_tdd_tags(tags) + if error is not None: + _validation_errors[result.full_name] = error + + +def end_test( + data: RunningTestCase, + result: ResultTestCase, +) -> None: + """Invert test results for ``tdd_expected_fail`` tests and apply + any tag validation errors. + + If there was a validation error from ``start_test``, the test is + forced to fail with the validation error message regardless of + the actual test outcome. + + For ``tdd_expected_fail`` tests with valid tags: + * If the test **failed** (bug still exists), the result is inverted + to **PASS** (expected failure). + * If the test **passed** (bug appears fixed), the result is inverted + to **FAIL** with a guidance message. + * If the test was **skipped** or has any other status, the result is + left unchanged (a skipped test is not evidence of a fix). + + An idempotency guard prevents double-processing if the listener is + loaded multiple times. + """ + full_name = result.full_name + + # Idempotency guard — skip if this test was already processed by + # a previous listener instance in the same process. + if full_name in _processed_tests: + return + _processed_tests.add(full_name) + + # --- Handle validation errors first --- + if full_name in _validation_errors: + error_msg = _validation_errors.pop(full_name) + result.status = "FAIL" + result.message = f"TDD tag validation error: {error_msg}" + _logger.error( + "TDD tag validation error in '%s': %s", + full_name, + error_msg, + ) return - status: str = getattr(result, "status", "") - original_message: str = getattr(result, "message", "") + # Tags are read from ``data.tags`` (static test definition) rather + # than ``result.tags``. TDD tags are declarative metadata and must + # be present in the source file — runtime ``Set Tags`` / ``Remove + # Tags`` modifications are not supported for TDD tags. + tags = {str(t).lower() for t in data.tags} + if not _should_invert_result(tags): + return - if status == "FAIL": - # Expected failure — bug still exists. Mark as PASS. + if result.status == "FAIL": + # Expected failure -- the bug still exists. Invert to PASS. + _logger.info( + "TDD expected failure: test '%s' failed as expected " + "(bug still exists). Inverting to PASS.", + full_name, + ) result.status = "PASS" result.message = ( - f"[tdd_expected_fail] Expected failure (bug still present). " - f"Original: {original_message}" + "TDD expected failure: test failed as expected (bug still exists)." + ) + elif result.status == "PASS": + # Unexpected pass -- the bug appears to be fixed but the + # tdd_expected_fail tag has not been removed. + guidance = ( + "Bug appears to be fixed. Remove the tdd_expected_fail tag " + "from this test and verify the fix through the bug fix " + "workflow. See CONTRIBUTING.md > Bug Fix Workflow." + ) + _logger.warning( + "%s Test: '%s'", + guidance, + full_name, ) - elif status == "PASS": - # Unexpected pass — bug appears fixed, tag should be removed. result.status = "FAIL" - result.message = ( - "[tdd_expected_fail] Test passed but still has the " - "tdd_expected_fail tag. The bug appears to be fixed — " - "remove the tag." + result.message = guidance + else: + # SKIP or other statuses — leave unchanged. A skipped + # tdd_expected_fail test is not evidence of a fix; it simply + # did not execute. + _logger.debug( + "TDD expected-fail test '%s' has status %s — leaving unchanged.", + full_name, + result.status, ) + + +def close() -> None: + """Clean up module-level state when the listener is unloaded. + + Called by Robot Framework when test execution finishes. Clears + internal state so the listener can be reused in long-lived processes + or consecutive Robot runs without stale data. + """ + _validation_errors.clear() + _processed_tests.clear() diff --git a/robot/tdd_tag_validation.robot b/robot/tdd_tag_validation.robot index dc58729a..18e0f4f6 100644 --- a/robot/tdd_tag_validation.robot +++ b/robot/tdd_tag_validation.robot @@ -1,9 +1,16 @@ *** Settings *** Documentation Integration tests for TDD bug-capture tag validation and -... @tdd_expected_fail result inversion logic defined in -... features/environment.py. Covers validate_tdd_tags(), -... should_invert_result(), and apply_tdd_inversion() including -... all guard paths (hook errors, dry-run, non-assertion exceptions). +... result-inversion logic. Covers both the Behave implementation +... (``features/environment.py``: ``validate_tdd_tags()``, +... ``should_invert_result()``, ``apply_tdd_inversion()``) and the +... Robot Framework listener (``tdd_expected_fail_listener.py``). +... +... Robot-side tests run fixture ``.robot`` files via a helper +... script that launches Robot with the listener active, then +... checks the final test status in the output XML. +... +... See CONTRIBUTING.md > TDD Bug Test Tags for the full +... specification. Resource ${CURDIR}/common.resource Suite Setup Setup Test Environment Suite Teardown Cleanup Test Environment @@ -12,6 +19,10 @@ Suite Teardown Cleanup Test Environment ${HELPER} ${CURDIR}/helper_tdd_tag_validation.py *** Test Cases *** +# =========================================================================== +# Behave-side tests (features/environment.py) +# =========================================================================== + Valid Tag Combinations Are Accepted [Documentation] Verify valid TDD tag sets raise no errors [Tags] testing tdd validation @@ -132,3 +143,90 @@ Inversion No Tag Passthrough Log ${result.stderr} Should Be Equal As Integers ${result.rc} 0 Should Contain ${result.stdout} inversion-no-tag-passthrough-ok + +# =========================================================================== +# Robot-side tests (tdd_expected_fail_listener.py) +# =========================================================================== + +TDD Expected Fail Test That Fails Is Inverted To Pass + [Documentation] A test tagged ``tdd_expected_fail`` that deliberately fails + ... should have its result inverted to PASS by the listener (bug still exists). + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} expected-fail-inverted cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-expected-fail-inverted-ok + +TDD Expected Fail Test That Passes Is Inverted To Fail + [Documentation] A test tagged ``tdd_expected_fail`` that unexpectedly passes + ... should have its result inverted to FAIL with a guidance message telling the + ... developer to remove the tag. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} unexpected-pass-inverted cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-unexpected-pass-inverted-ok + +TDD Tag Validation Catches Missing tdd_bug + [Documentation] A test with ``tdd_bug_`` but missing ``tdd_bug`` should + ... fail with a clear validation error message. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} missing-tdd-bug-validation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-missing-tdd-bug-validation-ok + +TDD Tag Validation Catches Missing tdd_bug_N + [Documentation] A test with ``tdd_expected_fail`` and ``tdd_bug`` but missing + ... ``tdd_bug_`` should fail with a clear validation error message. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} missing-bug-n-validation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-missing-bug-n-validation-ok + +Normal Test Unaffected By TDD Listener + [Documentation] A test with no TDD tags run alongside a tdd_expected_fail test + ... should be completely unaffected by the listener. Proves the listener is + ... loaded (the companion fixture is inverted) but does not modify normal tests. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} normal-test-unaffected cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-normal-test-unaffected-ok + +TDD Expected Fail Skip Status Is Unchanged + [Documentation] A ``tdd_expected_fail`` test that is skipped should stay SKIP. + ... The listener should not treat a skipped test as evidence of a fix. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} skip-status-unchanged cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-skip-status-unchanged-ok + +TDD Tag Validation Catches Expected Fail Alone + [Documentation] A test with only ``tdd_expected_fail`` (both ``tdd_bug`` and + ... ``tdd_bug_`` missing) should fail with validation error mentioning both. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} expected-fail-alone-validation cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-expected-fail-alone-validation-ok + +TDD Bug Tag Alone Is Valid + [Documentation] A test with only ``tdd_bug`` (no ``tdd_bug_`` or + ... ``tdd_expected_fail``) should pass normally — the listener should + ... not interfere. ``tdd_bug`` alone is a valid tag combination. + [Tags] tdd_infrastructure + ${result}= Run Process ${PYTHON} ${HELPER} tdd-bug-alone-valid cwd=${WORKSPACE} + Log ${result.stdout} + Log ${result.stderr} + Should Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stdout} tdd-bug-alone-valid-ok diff --git a/src/cleveragents/domain/models/core/__init__.py b/src/cleveragents/domain/models/core/__init__.py index 6a7bd552..8847edab 100644 --- a/src/cleveragents/domain/models/core/__init__.py +++ b/src/cleveragents/domain/models/core/__init__.py @@ -248,6 +248,14 @@ from cleveragents.domain.models.core.safety_profile import ( SafetyProfileRef, resolve_safety_profile, ) +from cleveragents.domain.models.core.sandbox_strategy import ( + DiffEntry as SandboxDiffEntry, +) +from cleveragents.domain.models.core.sandbox_strategy import ( + DiffView, + SandboxRef, + SandboxStrategyProtocol, +) from cleveragents.domain.models.core.session import ( MessageRole, Session, @@ -354,6 +362,7 @@ __all__ = [ "DiffBuilder", "DiffEntry", "DiffSerializer", + "DiffView", "DoDCriterion", "DoDEvaluator", "DoDResult", @@ -453,7 +462,10 @@ __all__ = [ "SafetyProfile", "SafetyProfileProvenance", "SafetyProfileRef", + "SandboxDiffEntry", + "SandboxRef", "SandboxStrategy", + "SandboxStrategyProtocol", "ScoredFragment", "ServiceRetryPolicy", "ServiceRetryPolicyRegistry", diff --git a/src/cleveragents/domain/models/core/sandbox_strategy.py b/src/cleveragents/domain/models/core/sandbox_strategy.py new file mode 100644 index 00000000..5f652a03 --- /dev/null +++ b/src/cleveragents/domain/models/core/sandbox_strategy.py @@ -0,0 +1,235 @@ +"""Custom sandbox strategy Protocol and supporting models. + +Defines the ``SandboxStrategyProtocol`` that custom sandbox strategies must +implement, along with ``SandboxRef`` and ``DiffView`` value objects used in +the Protocol method signatures. + +The Protocol specifies 9 methods covering the full sandbox lifecycle: +create, read, write, diff, commit, rollback, checkpoint, +restore_checkpoint, cleanup. + +Custom strategies are registered via configuration:: + + sandbox.custom_strategies..module = "my_module.path" + sandbox.custom_strategies..class = "MySandboxStrategy" + +See Also: + - :class:`~cleveragents.infrastructure.sandbox.protocol.Sandbox` + The existing infrastructure-level sandbox protocol. + - :class:`~cleveragents.domain.models.core.resource.SandboxStrategy` + The StrEnum defining built-in strategy names. + - Specification § Custom Sandbox Strategies (lines 46171-46186) + +Based on issue #586. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from datetime import datetime +from typing import Protocol, runtime_checkable + +from pydantic import BaseModel, ConfigDict, Field + +from cleveragents.domain.models.core.resource import Resource + +# --------------------------------------------------------------------------- +# SandboxRef — opaque reference to a sandbox instance +# --------------------------------------------------------------------------- + + +@dataclass(frozen=True) +class SandboxRef: + """Opaque reference to a sandbox instance. + + Returned by :meth:`SandboxStrategyProtocol.create` and passed to + all subsequent operations. The ``sandbox_id`` is a unique + identifier (typically a ULID); ``metadata`` allows strategy + implementations to carry extra state across calls. + + Attributes: + sandbox_id: Unique identifier for this sandbox instance. + plan_id: The plan that owns this sandbox. + resource_id: The resource being sandboxed. + created_at: When the sandbox was created. + metadata: Strategy-specific opaque data. + """ + + sandbox_id: str + plan_id: str + resource_id: str + created_at: datetime + metadata: dict[str, object] = field(default_factory=dict) + + +# --------------------------------------------------------------------------- +# DiffView — structured view of sandbox changes +# --------------------------------------------------------------------------- + + +class DiffEntry(BaseModel): + """A single file-level diff entry within a :class:`DiffView`. + + Attributes: + path: Relative path of the changed file. + operation: Type of change (``added``, ``modified``, ``deleted``). + before: Content before the change (``None`` for additions). + after: Content after the change (``None`` for deletions). + """ + + path: str = Field(..., min_length=1, description="Relative file path") + operation: str = Field( + ..., + description="Change type: 'added', 'modified', or 'deleted'", + ) + before: bytes | None = Field( + default=None, + description="Content before change (None for additions)", + ) + after: bytes | None = Field( + default=None, + description="Content after change (None for deletions)", + ) + + model_config = ConfigDict(frozen=True) + + +class DiffView(BaseModel): + """Structured view of all changes in a sandbox. + + Produced by :meth:`SandboxStrategyProtocol.diff`. + + Attributes: + sandbox_id: The sandbox these diffs belong to. + entries: Individual file-level diff entries. + summary: Human-readable summary of the changes. + """ + + sandbox_id: str = Field(..., description="Sandbox identifier") + entries: list[DiffEntry] = Field( + default_factory=list, + description="Per-file diff entries", + ) + summary: str = Field( + default="", + description="Human-readable change summary", + ) + + model_config = ConfigDict(frozen=True) + + +# --------------------------------------------------------------------------- +# SandboxStrategyProtocol — the 9-method custom strategy interface +# --------------------------------------------------------------------------- + + +@runtime_checkable +class SandboxStrategyProtocol(Protocol): + """Protocol for custom sandbox isolation strategies. + + All custom sandbox strategies must implement these 9 methods. + Strategies are registered via configuration and resolved at + runtime by the :class:`SandboxStrategyRegistry`. + + Lifecycle:: + + ref = strategy.create(plan_id, resource) # create sandbox + data = strategy.read(ref, "file.txt") # read from sandbox + change = strategy.write(ref, "f.txt", b"x") # write to sandbox + view = strategy.diff(ref) # inspect changes + strategy.checkpoint(ref, "cp-1") # save checkpoint + strategy.restore_checkpoint(ref, "cp-1") # restore checkpoint + strategy.commit(ref) # apply changes + strategy.rollback(ref) # discard changes + strategy.cleanup(ref) # release resources + """ + + def create(self, plan_id: str, resource: Resource) -> SandboxRef: + """Create a new sandbox for the given plan and resource. + + Args: + plan_id: Identifier of the plan requesting the sandbox. + resource: The resource to sandbox. + + Returns: + An opaque :class:`SandboxRef` for subsequent operations. + """ + ... + + def read(self, ref: SandboxRef, path: str) -> bytes: + """Read file content from the sandbox. + + Args: + ref: Sandbox reference from :meth:`create`. + path: Relative path within the sandbox. + + Returns: + Raw file content as bytes. + """ + ... + + def write(self, ref: SandboxRef, path: str, content: bytes) -> DiffEntry: + """Write content to a file in the sandbox. + + Args: + ref: Sandbox reference from :meth:`create`. + path: Relative path within the sandbox. + content: Raw content to write. + + Returns: + A :class:`DiffEntry` describing the change. + """ + ... + + def diff(self, ref: SandboxRef) -> DiffView: + """Compute a structured diff of all sandbox changes. + + Args: + ref: Sandbox reference from :meth:`create`. + + Returns: + A :class:`DiffView` with per-file entries. + """ + ... + + def commit(self, ref: SandboxRef) -> None: + """Apply sandbox changes to the original resource. + + Args: + ref: Sandbox reference from :meth:`create`. + """ + ... + + def rollback(self, ref: SandboxRef) -> None: + """Discard all sandbox changes. + + Args: + ref: Sandbox reference from :meth:`create`. + """ + ... + + def checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: + """Save a named checkpoint of the current sandbox state. + + Args: + ref: Sandbox reference from :meth:`create`. + checkpoint_id: Unique identifier for the checkpoint. + """ + ... + + def restore_checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: + """Restore the sandbox to a previously saved checkpoint. + + Args: + ref: Sandbox reference from :meth:`create`. + checkpoint_id: Identifier of the checkpoint to restore. + """ + ... + + def cleanup(self, ref: SandboxRef) -> None: + """Release all sandbox resources and artefacts. + + Args: + ref: Sandbox reference from :meth:`create`. + """ + ... diff --git a/src/cleveragents/infrastructure/sandbox/__init__.py b/src/cleveragents/infrastructure/sandbox/__init__.py index 5c2e5f7b..b6356d4a 100644 --- a/src/cleveragents/infrastructure/sandbox/__init__.py +++ b/src/cleveragents/infrastructure/sandbox/__init__.py @@ -6,6 +6,7 @@ and multiple strategy implementations (git worktree, filesystem copy, no-op). Stage B3 of the implementation plan. Updated in TASK-006 (B4 rework). Updated in M4 to add checkpoint/rollback hooks. Updated in M6 to add sandbox boundary algebra (#548). +Updated in M6+ to add custom sandbox strategy registration (#586). """ from cleveragents.infrastructure.sandbox.boundary import ( @@ -40,14 +41,23 @@ from cleveragents.infrastructure.sandbox.protocol import ( SandboxError, SandboxStatus, ) +from cleveragents.infrastructure.sandbox.strategy_adapter import ( + BuiltInSandboxStrategyAdapter, +) +from cleveragents.infrastructure.sandbox.strategy_registry import ( + CustomStrategyConfig, + SandboxStrategyRegistry, +) from cleveragents.infrastructure.sandbox.transaction_sandbox import TransactionSandbox __all__ = [ "BoundaryCache", + "BuiltInSandboxStrategyAdapter", "CheckpointManager", "Checkpointable", "CommitResult", "CopyOnWriteSandbox", + "CustomStrategyConfig", "GitMergeStrategy", "GitWorktreeSandbox", "JsonMergeStrategy", @@ -63,6 +73,7 @@ __all__ = [ "SandboxFactory", "SandboxManager", "SandboxStatus", + "SandboxStrategyRegistry", "SequentialMergeStrategy", "TransactionSandbox", "compute_sandbox_domains", diff --git a/src/cleveragents/infrastructure/sandbox/factory.py b/src/cleveragents/infrastructure/sandbox/factory.py index 2baef0c6..c5287243 100644 --- a/src/cleveragents/infrastructure/sandbox/factory.py +++ b/src/cleveragents/infrastructure/sandbox/factory.py @@ -6,12 +6,13 @@ available, this factory will accept ``Resource`` objects directly; until then it operates on raw parameters. Stage B3.6 of the implementation plan. Updated in TASK-006 (B4 rework). +Updated in M6+ to support custom sandbox strategy resolution (#586). """ from __future__ import annotations import logging -from typing import Literal +from typing import TYPE_CHECKING, Literal from cleveragents.infrastructure.sandbox.copy_on_write import CopyOnWriteSandbox from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox @@ -21,6 +22,11 @@ from cleveragents.infrastructure.sandbox.protocol import ( ) from cleveragents.infrastructure.sandbox.transaction_sandbox import TransactionSandbox +if TYPE_CHECKING: + from cleveragents.infrastructure.sandbox.strategy_registry import ( + SandboxStrategyRegistry, + ) + logger = logging.getLogger(__name__) # Strategy string constants aligned with spec SandboxStrategy enum (5 values) @@ -70,8 +76,25 @@ class SandboxFactory: - ``"transaction_rollback"`` -> :class:`TransactionSandbox` ``"snapshot"`` raises ``NotImplementedError``. + + Custom strategies registered in a :class:`SandboxStrategyRegistry` can + be resolved by passing the registry at construction time. """ + def __init__( + self, + *, + custom_registry: SandboxStrategyRegistry | None = None, + ) -> None: + """Initialise the factory. + + Args: + custom_registry: Optional registry of custom sandbox strategies. + When provided, the factory will fall back to this registry + for strategy names not matched by built-in strategies. + """ + self._custom_registry = custom_registry + def create_sandbox( self, resource_id: str, @@ -128,6 +151,34 @@ class SandboxFactory: raise ValueError(f"Unknown sandbox strategy: {sandbox_strategy}") + # -- custom strategy lookup ---------------------------------------------- + + def has_custom_strategy(self, name: str) -> bool: + """Check whether a custom strategy is registered. + + Args: + name: Strategy name to check. + + Returns: + ``True`` if a custom strategy with that name exists. + """ + if self._custom_registry is None: + return False + return self._custom_registry.has(name) + + def get_custom_strategy_class(self, name: str) -> type[object] | None: + """Look up a custom strategy class by name. + + Args: + name: Strategy name. + + Returns: + The strategy class, or ``None`` if not found. + """ + if self._custom_registry is None: + return None + return self._custom_registry.get(name) + # -- validation helpers -------------------------------------------------- @staticmethod diff --git a/src/cleveragents/infrastructure/sandbox/strategy_adapter.py b/src/cleveragents/infrastructure/sandbox/strategy_adapter.py new file mode 100644 index 00000000..6353fc72 --- /dev/null +++ b/src/cleveragents/infrastructure/sandbox/strategy_adapter.py @@ -0,0 +1,252 @@ +"""Adapter that wraps existing Sandbox implementations as SandboxStrategyProtocol. + +Bridges the existing :class:`Sandbox` protocol (infrastructure layer) to the +new :class:`SandboxStrategyProtocol` (domain layer). This allows built-in +sandbox implementations (NoSandbox, CopyOnWriteSandbox, GitWorktreeSandbox, +TransactionSandbox) to be used through the unified custom-strategy API. + +Based on issue #586. +""" + +from __future__ import annotations + +import logging +import os +from datetime import datetime + +from cleveragents.domain.models.core.resource import Resource +from cleveragents.domain.models.core.sandbox_strategy import ( + DiffEntry, + DiffView, + SandboxRef, +) +from cleveragents.infrastructure.sandbox.factory import ( + SandboxFactory, + SandboxStrategyStr, +) +from cleveragents.infrastructure.sandbox.protocol import ( + Sandbox, +) + +logger = logging.getLogger(__name__) + + +class BuiltInSandboxStrategyAdapter: + """Adapts a built-in :class:`Sandbox` implementation to the 9-method Protocol. + + Wraps the existing ``Sandbox`` lifecycle (create, get_path, commit, + rollback, cleanup) and adds read/write/diff/checkpoint/restore + operations on top. Checkpoint storage is in-memory per adapter + instance. + + This adapter satisfies :class:`SandboxStrategyProtocol` structurally. + + Args: + strategy_name: The built-in strategy name (e.g. ``"none"``). + factory: Optional :class:`SandboxFactory` to create sandbox instances. + """ + + def __init__( + self, + strategy_name: SandboxStrategyStr = "none", + factory: SandboxFactory | None = None, + ) -> None: + self._strategy_name: SandboxStrategyStr = strategy_name + self._factory = factory or SandboxFactory() + self._sandboxes: dict[str, Sandbox] = {} + self._checkpoints: dict[str, dict[str, bytes]] = {} + + def create(self, plan_id: str, resource: Resource) -> SandboxRef: + """Create a sandbox using the built-in factory and return a ref. + + Args: + plan_id: Identifier of the plan requesting the sandbox. + resource: The resource to sandbox. + + Returns: + A :class:`SandboxRef` wrapping the internal sandbox instance. + """ + location = resource.location or "" + sandbox = self._factory.create_sandbox( + resource_id=resource.resource_id, + original_path=location, + sandbox_strategy=self._strategy_name, + ) + ctx = sandbox.create(plan_id) + self._sandboxes[ctx.sandbox_id] = sandbox + + return SandboxRef( + sandbox_id=ctx.sandbox_id, + plan_id=plan_id, + resource_id=resource.resource_id, + created_at=datetime.now(), + metadata={"strategy": self._strategy_name}, + ) + + def read(self, ref: SandboxRef, path: str) -> bytes: + """Read file content from the sandbox. + + Args: + ref: Sandbox reference. + path: Relative file path. + + Returns: + File content as bytes. + + Raises: + FileNotFoundError: If the file does not exist. + KeyError: If the sandbox ref is unknown. + """ + sandbox = self._sandboxes[ref.sandbox_id] + full_path = sandbox.get_path(path) + with open(full_path, "rb") as fh: + return fh.read() + + def write(self, ref: SandboxRef, path: str, content: bytes) -> DiffEntry: + """Write content to a file in the sandbox. + + Args: + ref: Sandbox reference. + path: Relative file path. + content: Content to write. + + Returns: + A :class:`DiffEntry` describing the change. + """ + sandbox = self._sandboxes[ref.sandbox_id] + full_path = sandbox.get_path(path) + + existed = os.path.exists(full_path) + before: bytes | None = None + if existed: + with open(full_path, "rb") as fh: + before = fh.read() + + parent_dir = os.path.dirname(full_path) + if parent_dir: + os.makedirs(parent_dir, exist_ok=True) + + with open(full_path, "wb") as fh: + fh.write(content) + + operation = "modified" if existed else "added" + return DiffEntry( + path=path, + operation=operation, + before=before, + after=content, + ) + + def diff(self, ref: SandboxRef) -> DiffView: + """Return a diff view of sandbox changes. + + For built-in strategies, returns a minimal summary since the + underlying Sandbox protocol does not expose diff information + until commit. + + Args: + ref: Sandbox reference. + + Returns: + A :class:`DiffView` (may be empty for built-in strategies). + """ + return DiffView( + sandbox_id=ref.sandbox_id, + entries=[], + summary="Built-in strategy; diff computed at commit time.", + ) + + def commit(self, ref: SandboxRef) -> None: + """Commit sandbox changes via the underlying Sandbox. + + Args: + ref: Sandbox reference. + """ + sandbox = self._sandboxes[ref.sandbox_id] + sandbox.commit() + + def rollback(self, ref: SandboxRef) -> None: + """Rollback sandbox changes via the underlying Sandbox. + + Args: + ref: Sandbox reference. + """ + sandbox = self._sandboxes[ref.sandbox_id] + sandbox.rollback() + + def checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: + """Save a named checkpoint (in-memory snapshot for adapter). + + Args: + ref: Sandbox reference. + checkpoint_id: Unique checkpoint identifier. + """ + key = f"{ref.sandbox_id}:{checkpoint_id}" + sandbox = self._sandboxes[ref.sandbox_id] + + # Snapshot current sandbox state if sandbox is filesystem-based + ctx = sandbox.context + snapshot: dict[str, bytes] = {} + if ctx is not None and os.path.isdir(ctx.sandbox_path): + for dirpath, _dirs, files in os.walk(ctx.sandbox_path): + for fname in files: + full = os.path.join(dirpath, fname) + rel = os.path.relpath(full, ctx.sandbox_path) + with open(full, "rb") as fh: + snapshot[rel] = fh.read() + + self._checkpoints[key] = snapshot + logger.debug( + "Checkpoint saved: sandbox=%s checkpoint=%s files=%d", + ref.sandbox_id, + checkpoint_id, + len(snapshot), + ) + + def restore_checkpoint(self, ref: SandboxRef, checkpoint_id: str) -> None: + """Restore sandbox state from a named checkpoint. + + Args: + ref: Sandbox reference. + checkpoint_id: Identifier of the checkpoint to restore. + + Raises: + KeyError: If the checkpoint does not exist. + """ + key = f"{ref.sandbox_id}:{checkpoint_id}" + snapshot = self._checkpoints[key] + + sandbox = self._sandboxes[ref.sandbox_id] + ctx = sandbox.context + if ctx is not None and os.path.isdir(ctx.sandbox_path): + # Restore files from snapshot + for rel_path, content in snapshot.items(): + full = os.path.join(ctx.sandbox_path, rel_path) + parent = os.path.dirname(full) + if parent: + os.makedirs(parent, exist_ok=True) + with open(full, "wb") as fh: + fh.write(content) + + logger.debug( + "Checkpoint restored: sandbox=%s checkpoint=%s", + ref.sandbox_id, + checkpoint_id, + ) + + def cleanup(self, ref: SandboxRef) -> None: + """Cleanup sandbox resources. + + Args: + ref: Sandbox reference. + """ + sandbox = self._sandboxes.pop(ref.sandbox_id, None) + if sandbox is not None: + sandbox.cleanup() + + # Remove associated checkpoints + keys_to_remove = [ + k for k in self._checkpoints if k.startswith(f"{ref.sandbox_id}:") + ] + for k in keys_to_remove: + del self._checkpoints[k] diff --git a/src/cleveragents/infrastructure/sandbox/strategy_registry.py b/src/cleveragents/infrastructure/sandbox/strategy_registry.py new file mode 100644 index 00000000..04c8c68d --- /dev/null +++ b/src/cleveragents/infrastructure/sandbox/strategy_registry.py @@ -0,0 +1,298 @@ +"""Config-driven custom sandbox strategy registry. + +Provides :class:`SandboxStrategyRegistry` for registering, validating, +and resolving custom sandbox strategies. Strategies are registered +either programmatically or via configuration keys:: + + sandbox.custom_strategies..module = "my_package.my_module" + sandbox.custom_strategies..class = "MySandboxClass" + +All custom strategies are validated against the +:class:`SandboxStrategyProtocol` at registration time using +``@runtime_checkable`` Protocol checks. + +Thread-safe via ``threading.RLock``. + +Based on issue #586. +""" + +from __future__ import annotations + +import logging +import threading +from typing import Any + +from cleveragents.infrastructure.plugins.exceptions import ( + PluginLoadError, + ProtocolMismatchError, +) +from cleveragents.infrastructure.plugins.loader import PluginLoader + +logger = logging.getLogger(__name__) + + +# --------------------------------------------------------------------------- +# Custom strategy configuration model +# --------------------------------------------------------------------------- + + +class CustomStrategyConfig: + """Configuration for a single custom sandbox strategy. + + Attributes: + name: Strategy name (used as the key in lookups). + module: Python module path containing the strategy class. + class_name: Name of the class within the module. + options: Optional strategy-specific configuration. + """ + + __slots__ = ("class_name", "module", "name", "options") + + def __init__( + self, + name: str, + module: str, + class_name: str, + options: dict[str, Any] | None = None, + ) -> None: + if not name: + raise ValueError("Strategy name cannot be empty") + if not module: + raise ValueError("Strategy module cannot be empty") + if not class_name: + raise ValueError("Strategy class_name cannot be empty") + + self.name = name + self.module = module + self.class_name = class_name + self.options = options or {} + + +# --------------------------------------------------------------------------- +# SandboxStrategyRegistry +# --------------------------------------------------------------------------- + + +class SandboxStrategyRegistry: + """Registry for custom sandbox strategies with Protocol validation. + + Maintains a thread-safe mapping of strategy names to their + implementing classes. Strategies are validated against the + :class:`SandboxStrategyProtocol` at registration time. + + Built-in strategies (``none``, ``git_worktree``, ``copy_on_write``, + ``transaction_rollback``) are not managed by this registry; they + are handled by :class:`SandboxFactory`. + + Usage:: + + registry = SandboxStrategyRegistry() + registry.register( + "my_strategy", + "cleveragents.my_module", + "MySandbox", + ) + cls = registry.get("my_strategy") + + Args: + loader: Optional :class:`PluginLoader` for dynamic imports. + allowed_prefixes: Module prefix allowlist for the loader. + """ + + def __init__( + self, + *, + loader: PluginLoader | None = None, + allowed_prefixes: tuple[str, ...] | None = None, + ) -> None: + self._lock = threading.RLock() + self._loader = loader or PluginLoader(allowed_prefixes=allowed_prefixes) + self._strategies: dict[str, type[Any]] = {} + + # ------------------------------------------------------------------ + # Registration + # ------------------------------------------------------------------ + + def register( + self, + name: str, + module_path: str, + class_name: str, + ) -> type[Any]: + """Register a custom sandbox strategy. + + Loads the class via the plugin loader, validates it against + :class:`SandboxStrategyProtocol`, and stores it. + + Args: + name: Strategy name for lookup. + module_path: Fully-qualified module path. + class_name: Class name within the module. + + Returns: + The loaded and validated class. + + Raises: + ValueError: If *name* is empty. + PluginLoadError: If the class cannot be imported. + ProtocolMismatchError: If the class does not satisfy + the SandboxStrategyProtocol. + """ + if not name: + raise ValueError("Strategy name cannot be empty") + + cls = self._loader.load_class(module_path, class_name) + self._validate_protocol(cls) + + with self._lock: + self._strategies[name] = cls + logger.info( + "Registered custom sandbox strategy: name=%s class=%s.%s", + name, + module_path, + class_name, + ) + + return cls + + def register_from_config(self, config: CustomStrategyConfig) -> type[Any]: + """Register a strategy from a :class:`CustomStrategyConfig`. + + Args: + config: Configuration describing the strategy. + + Returns: + The loaded and validated class. + """ + return self.register(config.name, config.module, config.class_name) + + def register_all_from_config( + self, + configs: dict[str, dict[str, Any]], + ) -> list[str]: + """Register multiple strategies from a config dictionary. + + The dictionary maps strategy names to dicts with ``module`` + and ``class`` keys:: + + { + "my_strategy": { + "module": "my_package.module", + "class": "MyStrategy", + }, + } + + Args: + configs: Strategy configuration dictionary. + + Returns: + List of successfully registered strategy names. + """ + registered: list[str] = [] + for name, cfg in configs.items(): + module = cfg.get("module", "") + class_name = cfg.get("class", "") + if not module or not class_name: + logger.warning( + "Skipping custom strategy '%s': missing module or class", + name, + ) + continue + + try: + self.register(name, module, class_name) + registered.append(name) + except (PluginLoadError, ProtocolMismatchError, ValueError) as exc: + logger.error( + "Failed to register custom strategy '%s': %s", + name, + exc, + ) + + return registered + + # ------------------------------------------------------------------ + # Lookup + # ------------------------------------------------------------------ + + def get(self, name: str) -> type[Any] | None: + """Look up a registered custom strategy class. + + Args: + name: Strategy name. + + Returns: + The class, or ``None`` if not registered. + """ + with self._lock: + return self._strategies.get(name) + + def has(self, name: str) -> bool: + """Check whether a strategy is registered. + + Args: + name: Strategy name. + + Returns: + ``True`` if the strategy is registered. + """ + with self._lock: + return name in self._strategies + + def list_strategies(self) -> list[str]: + """Return names of all registered custom strategies. + + Returns: + Sorted list of strategy names. + """ + with self._lock: + return sorted(self._strategies.keys()) + + def clear(self) -> None: + """Remove all registered custom strategies.""" + with self._lock: + self._strategies.clear() + logger.debug("Custom sandbox strategy registry cleared") + + # ------------------------------------------------------------------ + # Validation + # ------------------------------------------------------------------ + + @staticmethod + def _validate_protocol(cls: type[Any]) -> None: + """Validate that a class satisfies :class:`SandboxStrategyProtocol`. + + Uses structural subtyping: checks that all 9 required methods + are present as callable attributes on the class. + + Args: + cls: The class to validate. + + Raises: + ProtocolMismatchError: If the class is missing required methods. + """ + required_methods = ( + "create", + "read", + "write", + "diff", + "commit", + "rollback", + "checkpoint", + "restore_checkpoint", + "cleanup", + ) + + missing = [ + method + for method in required_methods + if not callable(getattr(cls, method, None)) + ] + + if missing: + msg = ( + f"Class '{cls.__name__}' does not satisfy " + f"SandboxStrategyProtocol. Missing methods: " + f"{', '.join(missing)}" + ) + raise ProtocolMismatchError(msg)