diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a7b3991f..5c24bd6b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,199 +2,14 @@ ## Unreleased -- Added missing `LspServerConfig` model fields per specification: - `description` (max 1000 chars), `transport` (`LspTransport` enum with - `stdio`/`tcp`, default `stdio`), `initialization` (dict for LSP - `initializationOptions`), and `workspace_settings` (dict for - `workspace/didChangeConfiguration`). Updated `agents lsp show` Rich - output to display new fields. All fields have defaults for backward - compatibility. Includes 20 Behave scenarios and 5 Robot tests. (#835) -- Added E2E test for Workflow Example 18: Container with Remote Repo Clone - (trusted profile). Introduces the new `--clone-into` CLI flag on - `resource add` for container-instance and devcontainer-instance resources - (format: `REPO_URL:CONTAINER_PATH`), with input validation and type - restriction. Exercises two-step project creation and linking, plan-level - `--execution-environment` with `--execution-env-priority fallback`, and full - plan lifecycle including container commit/push verification on apply. - (`robot/e2e/wf18_container_clone.robot`, - `src/cleveragents/cli/commands/resource.py`) (#764) -- Added E2E test for Workflow Example 12 — large-scale hierarchical feature - implementation (supervised profile). Covers 4-project setup with per-project - invariants, spec-compliant action YAML (estimation_actor, invariant_actor, - automation_profile: cautious, action-level invariants), all-project plan use, - hierarchical tree inspection, plan correct (append mode) on non-root decision, - phased lifecycle-apply, and terminal-state verification via JSON status. - Dynamic actor selection and UUID-suffixed names for CI safety. - Known limitations: `plan prompt` not yet implemented as CLI subcommand, - action `--arg` omitted due to UNIQUE constraint bug, validation registration - omitted pending independent validation. (#758) -- Added E2E test for Workflow Example 17: explicit container with directory - mount using trusted automation profile. Exercises container-instance resource - registration, project link-resource, execution environment setting via - project context set, plan-level execution-env-priority override via plan use, - and full plan lifecycle with dynamic actor selection. Includes TDD - bug-capture tests for deferred acceptance criteria: dual mount registration - (#1078), project-level execution-env-priority (#1079), and precedence - level 2 resolution (#1080). - (`robot/e2e/wf17_explicit_container.robot`) (#763) -- Added `correction_attempts` table per specification DDL with - `CorrectionAttemptModel` ORM, `CorrectionAttemptRecord` domain model, - `CorrectionAttemptRepository` CRUD layer, Alembic migration, and - `CorrectionAttemptState` enum. Repository `update_state()` accepts - typed `CorrectionAttemptState` enum and `datetime` parameters and - enforces the spec lifecycle (`pending → executing → complete|failed`) - via `InvalidCorrectionStateTransitionError`. - `CorrectionAttemptRecord.guidance` validates non-empty with - `max_length=10_000`. `created_at` column includes spec-aligned - `server_default`; `to_domain()` normalises naive timestamps to UTC. - State transition validation extracted to domain-level - `validate_correction_state_transition()` function with - `CORRECTION_ATTEMPT_VALID_TRANSITIONS` and - `CORRECTION_ATTEMPT_TERMINAL_STATES` constants. - `update_state()` rejects `completed_at` on non-terminal transitions. - Improved FK-violation error messages in `create()` and `update_state()`. - Normalised timestamp format in `from_domain()` to millisecond precision - (`SS.mmm`) matching SQLite `server_default` `strftime('%f')` output - for consistent string-based ordering. - `CORRECTION_ATTEMPT_VALID_TRANSITIONS` and - `CORRECTION_ATTEMPT_TERMINAL_STATES` now use typed - `CorrectionAttemptState` enum keys/values. - Updated repository module docstring tables. - `from_domain()` normalises timestamps to UTC via `astimezone(UTC)` - before formatting, preventing silent data loss for non-UTC datetimes. - `update_state()` auto-sets `completed_at` when transitioning to - terminal states if not explicitly provided. - `CorrectionAttemptRecord` `plan_id` and `original_decision_id` - validators now return stripped values, preventing whitespace-padded - IDs from causing FK lookup failures. - Added new domain exports to `__init__.py` `__all__`. - Aligned `update_state()` `completed_at` timestamp to millisecond - precision for consistency. - Improved FK-violation error message in `update_state()` to avoid - misleading reference when `new_decision_id` is `None`. - Removed unnecessary `session.rollback()` in read-only repository - methods (`get()`, `list_by_plan()`) for consistency with other repos. - Added `created_at` and `completed_at` Pydantic validators on - `CorrectionAttemptRecord` to normalise naive datetimes to UTC, - preventing `ValueError` in `from_domain()` `astimezone()` calls. - Added defensive enum coercion in `CorrectionAttemptModel.to_domain()` - with warning-level logging for invalid `mode`/`state` DB values, - consistent with `LifecyclePlanModel.to_domain()` pattern. - Moved `CorrectionAttemptState` from `TYPE_CHECKING`-only to runtime - import in the repository module, removing redundant in-method import. - Changed `original_decision_id` FK from `CASCADE` to `RESTRICT` - matching the spec DDL default and the codebase convention for - non-dependency FK references to decisions, preserving correction - audit trail when decisions are cleaned up. - Added `new_decision_id` strip-and-validate field validator matching - the pattern used for `plan_id` and `original_decision_id`. - Added input validation for `new_decision_id` in `update_state()` - rejecting empty and whitespace-only values per CONTRIBUTING.md - argument validation guidelines. - Fixed BDD mode-validation scenario to use dedicated `Then` step - with field-level assertion instead of reusing guidance error step. - Moved `new_decision_id` and `archived_artifacts_path` argument - validation in `update_state()` before any ORM row mutations per - CONTRIBUTING.md early-validation guidelines, preventing dirty - session state on validation failure. - Added `archived_artifacts_path` empty/whitespace-only rejection - in `update_state()` matching the `new_decision_id` validation - pattern per CONTRIBUTING.md argument validation guidelines. - 43 BDD scenarios and 5 Robot integration tests including cascade - deletion, terminal-state rejection, failed-path transition, guidance - validation, max-length boundary, min-length boundary, not-found - update, FK-violation update, completed_at guard, timezone - normalization, archived_artifacts_path round-trip, delete-in-complete- - state, cross-plan list isolation, auto-set completed_at on terminal - transition, FK violation on create, invalid mode rejection, - whitespace/empty `new_decision_id` rejection, combined field - update, self-transition rejection, and empty/whitespace - `archived_artifacts_path` rejection. - Fixed `update_state()` bug where `archived_artifacts_path` was - stored without stripping leading/trailing whitespace, unlike - `new_decision_id` which correctly used the stripped value. - Extracted `SQLITE_TIMESTAMP_MS_LEN` constant and - `format_sqlite_timestamp()` helper for millisecond-precision - timestamp formatting, used by both `from_domain()` and - `update_state()`. - Changed `InvalidCorrectionStateTransitionError` base class from - `DatabaseError` to `BusinessRuleViolation` per CONTRIBUTING.md - exception semantics (state transition is a business rule, not a - database error). - Changed `new_decision_id` FK from `SET NULL` to `RESTRICT` matching - the spec DDL default (no ON DELETE clause) and consistent with - `original_decision_id`. - Changed `update_state()` input validation for `new_decision_id` and - `archived_artifacts_path` from `DatabaseError` to `ValueError` per - CONTRIBUTING.md argument validation guidelines. - Defensive `to_domain()` coercion now defaults corrupted state to - `failed` (terminal) instead of `pending`, preventing re-execution - of completed/failed corrections with corrupted DB values. - 45 BDD scenarios (was 43) with new RESTRICT FK test for - `original_decision_id` and stronger cross-plan isolation test. - Added ORM-level `relationship(cascade="all, delete-orphan")` on - `LifecyclePlanModel` for `CorrectionAttemptModel`, consistent with - all other `v3_plans` child tables, ensuring ORM-level cascade - deletes work even when SQLite FK enforcement is disabled. - Added defensive `to_domain()` coercion for corrupted `guidance` - column (defaults to `"[corrupted]"` with warning log), consistent - with existing mode/state coercion pattern. - Added `ValueError` guard in `format_sqlite_timestamp()` rejecting - naive datetimes per CONTRIBUTING.md fail-fast argument validation. - Fixed `update_state()` to defensively handle corrupted DB state - values (coerces to `failed` terminal with warning log), consistent - with `to_domain()` defensive coercion pattern. - Strengthened RESTRICT FK BDD assertion to verify exception type - (`IntegrityError`/`DatabaseError`) instead of only checking - presence. - Split multi-When/Then cross-plan isolation BDD scenario into - idiomatic single-When/Then scenarios. - 53 BDD scenarios (was 45) with new defensive `to_domain()` coercion - tests (corrupted mode/state/guidance), `format_sqlite_timestamp()` - naive datetime rejection, domain model naive datetime normalisation, - and corrupted DB state handling in `update_state()`. - (#920) -- Hardened automation profile configuration validation after the task-flag - rename: `AutomationProfile` now rejects unknown top-level fields instead of - silently ignoring them, and raises an actionable ``ValueError`` listing the - required renames when legacy ``auto_*`` keys are supplied (instead of a - generic Pydantic "Extra inputs are not permitted" error). Updated - automation profile schema and documentation references (`specification`, - ADRs, and reference docs) to the task-type field names, added BDD coverage - for rejecting legacy threshold keys in `automation-profile add`, restored - docs-schema parity for optional `guards` profile configs, aligned the - `m5_001` migration header metadata text with its actual revision chain, - updated M6 fixture files to use the spec-defined field names, added - phase-transition semantic bridge comments in `PlanLifecycleService`. - Restored categorised CLI ``automation-profile show`` output to match the - specification (Phase Transitions / Decision Automation / Self-Repair / - Execution Controls), added missing ``access_network`` field to spec ``show`` - output examples, aligned ADR-017 and reference doc descriptions with the - specification's Automatable Tasks table (all 11 fields), and extended the - repository roundtrip test to assert all 11 threshold fields. Fixed - benchmark ``_make_profile()`` helper passing safety fields as top-level - kwargs instead of via ``SafetyProfile`` sub-model (incompatible with - ``extra="forbid"``). Aligned CLI JSON/YAML output structure for - ``automation-profile show`` with the specification's grouped format - (``phase_transitions``, ``decision_automation``, ``self_repair``, - ``execution_controls``). Moved safety boolean fields into the Execution - Controls section of Rich output per spec examples. Reverted ``auto`` - profile description to "Fully automatic except apply" per specification. - Added comprehensive field-name mapping table to ``automation_profile.py`` - module docstring documenting the old phase-transition names to new - spec task-type names correspondence and added cross-reference in - ``AutonomyController._get_threshold()`` docstring. - (#902) -- Added TDD bug-capture tests for bug #1141 — session create does not persist - into subsequent session list output. Added a Behave scenario and Robot E2E - test with required tags (`@tdd_bug`, `@tdd_bug_1141`, `@tdd_expected_fail` / - `tdd_bug`, `tdd_bug_1141`, `tdd_expected_fail`) to assert create→list should - show one session. The underlying assertion currently fails and is intentionally - inverted until bug #1141 is fixed. (#1142) -- Fixed `project context set` missing `--execution-env-priority` flag. - Setting is persisted and displayed by `project context show`. - Project-level priority propagates to `plan use` when no plan-level - override is specified. (#1079) +- Added TDD bug-capture test for bug #991 — AuditService._ensure_session() + TOCTOU race. Behave BDD scenario (`@tdd_bug @tdd_bug_991 + @tdd_expected_fail`) launches 10 concurrent threads through a + `threading.Barrier` to prove that `_ensure_session()` creates multiple + engines when called without a lock. The test asserts `create_engine` is + called exactly once — which currently fails, confirming the race. The + `@tdd_expected_fail` tag inverts this to a CI pass until the fix is + merged. (#1095) - Implemented `--mount` flag on `resource add container-instance`. Supports resource-reference mounts (`--mount local/api-repo:/workspace`) and host-path mounts (`--mount /var/config:/config:ro`). Multiple `--mount` @@ -209,24 +24,6 @@ guidance propagation -- and verify cautious-profile confidence-threshold pausing (S37262-37367) including a pause-and-resume flow. Facade stub updated to echo guidance text. (#961) -- Added TDD bug-capture tests for bug #1023: CLI commands fail without explicit - `agents init` when `CLEVERAGENTS_AUTO_APPLY_MIGRATIONS=true` is set. Two - Behave BDD scenarios and two Robot Framework integration tests verify that - `resource add` and `project create` succeed in a fresh environment without - prior init. Tests use `@tdd_expected_fail` until the bug fix is merged. - (#1033) -- Added TDD bug-capture tests for bug #1079 — `project context set` missing - `--execution-env-priority` flag. Six Behave BDD scenarios exercise the CLI - path (override, fallback, rejection without `--execution-environment`, default - value, invalid value, and round-trip via `context show`). Three Robot - Framework integration tests cover override acceptance, fallback acceptance, - and full persistence round-trip. Tests use `@tdd_expected_fail` until the - fix is merged. (#1100) -- Fixed Robot Framework test mocks for ``plan correct`` dry-run and correction - subplan helpers to use ``container.decision_service()`` instead of the - non-existent ``container.resolve()``, matching corrected production code. - Activated regression-guard BDD scenarios for ``plan tree``, ``plan explain``, - and ``plan correct``. (#647) - Added TDD bug-capture tests for bug #1076 — `use_action()` does not propagate `automation_profile` to Plan. Three Behave BDD scenarios (`@tdd_bug @tdd_bug_1076 @tdd_expected_fail`) verify the full precedence @@ -265,17 +62,6 @@ workspace-symbols query schema, and defensive schema validation. Extended `initialize()` to advertise all 11 capabilities. Fixed `workspace_symbols` runtime handler to accept query-only input. (#834) -- Added per-phase ACMS context analysis summaries for - `agents project context inspect/simulate` with human-readable metrics - (fragment/resource counts, size/tokens, budget utilization), explicit - strategize->execute->apply narrowing diagnostics, Robot acceptance - verification upgrades, and Behave edge-case coverage for empty, - single-resource, multi-resource, and budget-constrained contexts. (#849) -- Added 10,000-file ACMS indexing reliability improvements: configurable - runtime timeout bounds propagated through the repository indexing service, - utility walker, and CLI (`agents repo index --timeout-seconds`), plus - large-scale verification coverage in Behave and Robot and a dedicated - 10K-file indexing benchmark path for regression tracking. (#851) - Added TDD bug-capture E2E tests for bug #1028 — ACMS indexing pipeline not wired into CLI. Four Robot Framework E2E tests prove ContextTierService starts empty on every CLI invocation. Tests use `@tdd_expected_fail` until the bug @@ -293,15 +79,6 @@ `final_validation_results` fields on the result model, DI container registration per ADR-003, and structured logging via `structlog`. (#583) -- Implemented real revert-mode re-execution from decision point. - `CorrectionService.execute_revert` now performs checkpoint restoration via - `CheckpointService`, extracts `actor_state_ref` from the target decision's - context snapshot for reasoning rollback, generates a `user_intervention` - decision ID for guidance injection, and signals phase transition to - Strategize. Added `checkpoint_restored`, `actor_state_ref`, - `user_intervention_decision_id`, and `phase_transition_target` fields to - `CorrectionResult`. Includes 16 Behave BDD scenarios and 7 Robot Framework - integration tests. (#844) - Added LSP resource types: `executable`, `lsp-server`, `lsp-workspace`, `lsp-document` with parent/child hierarchy, auto-discovery rules, and handler references. Registered in bootstrap, with YAML configurations, @@ -347,85 +124,6 @@ violation reporting. Pipeline integration in `ACMSPipeline.assemble()` applies enforcement as a pre-filter when a `context_view` is provided. (#847) -- Added E2E test for Workflow Example 16: devcontainer-driven development - with supervised automation profile. Exercises devcontainer auto-detection - during resource registration, lazy container build during plan execution, - tool invocation routing to container workspace, and apply writing changes - back to host filesystem via bind mount. Uses dynamic actor selection - (Anthropic/OpenAI) and UUID-suffixed names for parallel CI safety. - (`robot/e2e/wf16_devcontainer.robot`) (#762) -- **Breaking (behavioral):** `SandboxManager.commit_all()` is now an - all-or-nothing atomic operation per specification line 45938. (#925) - - On partial failure, already-committed sandboxes are rolled back in - reverse (LIFO) order following the standard transaction-log undo - pattern. - - Non-`SandboxError` exceptions are wrapped in `AtomicCommitError` - (chaining the original as `__cause__`) with `rolled_back_ids` and - `failed_rollback_ids` attributes; `SandboxError` exceptions return - a `CommitResult` with `rolled_back` / `rollback_failed` metadata. - - Non-rollbackable sandboxes (`NoSandbox`, `TransactionSandbox`) are - committed last in the batch so rollbackable sandboxes can be undone - if they fail first. A warning is logged when these types are present. - - `TransactionSandbox.rollback()` from `COMMITTED` now raises - `SandboxRollbackError` (database commits are irreversible) instead - of silently reporting success. - - Extracted shared `_fs_utils` module (`backup_directory`, - `safe_restore`, `compute_diff`) with symlink, permission, and - timestamp preservation; replaces duplicated per-class methods. - - `safe_restore` uses rename-based swap (both renames are O(1) on - the same filesystem) to prevent data loss during restore. - - Pre-commit backup is created on the same filesystem as the original - (avoids cross-device copy overhead); assigned only after - `backup_directory()` succeeds; skipped when no changes detected. - - `backup_directory` defers directory permissions and timestamps to a - bottom-up post-walk pass, fixing POSIX mtime overwrite; skips - non-regular files (FIFOs, sockets, device files) with a warning. - - `CopyOnWriteSandbox` and `OverlaySandbox` commit error handler - restores original from pre-commit backup; catches `Exception` - (not just `OSError`) so unexpected errors also trigger restoration. - - Rollback from `COMMITTED` with no backup (no changes applied) is - a no-op instead of raising `SandboxRollbackError`. - - `CopyOnWriteSandbox` and `OverlaySandbox` rollback from `COMMITTED` - resets sandbox copy/merged directory from restored original, - preventing stale data from being exposed on re-activation. - - `OverlaySandbox` rollback from `COMMITTED` properly remounts - OverlayFS (or re-copies for userspace fallback); raises - `SandboxRollbackError` if unmount fails. No longer double-wraps - `SandboxRollbackError` — inner errors are re-raised directly. - - `OverlaySandbox.get_path()` now transitions `ROLLED_BACK → ACTIVE` - for consistency with `CopyOnWriteSandbox` and the protocol - transition table. - - `GitWorktreeSandbox.get_path()` now accepts `ROLLED_BACK` status - for consistency with all other sandbox types and the protocol - transition table (`ROLLED_BACK → ACTIVE`). - - `rollback_all` now also handles sandboxes in `COMMITTED` status - and catches `Exception` (not just `SandboxError`) to ensure all - rollbacks are attempted. - - `cleanup_all` now catches `Exception` (not just `SandboxError`) - to prevent a single unexpected error from aborting cleanup of - remaining sandboxes. - - `CopyOnWriteSandbox` and `OverlaySandbox` rollback from `ACTIVE` - now uses `dirs_exist_ok=True` to prevent `FileExistsError` when - `rmtree` silently fails. - - `cleanup_abandoned` now catches `Exception` (not just - `SandboxError`) so that unexpected errors do not crash the loop - and prevent remaining abandoned sandboxes from being cleaned up, - consistent with `cleanup_all`, `rollback_all`, and - `_rollback_committed`. - - `OverlaySandbox._mount_overlay()` and `_unmount_overlay()` now - catch `subprocess.TimeoutExpired` (in addition to - `CalledProcessError` and `OSError`), preventing `create()` from - leaving the sandbox in `PENDING` status and `cleanup()` from - leaving it in a zombie state when `mount`/`umount` hangs. - - `OverlaySandbox._mount_overlay()` validates that overlay paths - do not contain commas, which would corrupt the OverlayFS mount - options string. - - `GitWorktreeSandbox.commit()` now checks `git diff` return code - so that a failed diff command does not silently skip the merge. - - `safe_restore` cleanup of the temporary rollback container now - runs in a `finally` block, preventing a temp directory leak when - the rename fails and the exception is re-raised. - - `AtomicCommitError` exported from `sandbox` package `__init__.py`. - Aligned plan lifecycle model with specification: ERRORED is now terminal in `is_terminal`, per-phase state validation enforces APPLIED/CONSTRAINED to APPLY-only and COMPLETE to @@ -436,46 +134,6 @@ in `execute_plan()` for consistency with phase-state validator. Updated `PlanResumeService` docstring to reflect ERRORED terminality. (#918) -- Aligned `v3_plans` table schema with specification DDL: added - `effective_profile_snapshot` column (TEXT NOT NULL, validated as JSON), - made `root_plan_id` NOT NULL with self-referencing for root plans and - explicit `RESTRICT` FK policy, made `automation_profile` NOT NULL with - default `"balanced"`, and documented the intentional `phase` default - deviation (`"action"` vs spec `"strategize"`). Migration backfill - correctly resolves root ancestors for child plans at arbitrary hierarchy - depth via level-by-level propagation with safety bound. Hardened - `automation_profile` deserialization to catch `RecursionError`. - Hardened `effective_profile_snapshot` deserialization in `to_domain()` - to gracefully fall back to `'{}'` on corrupted JSON, preventing a - single corrupted row from crashing plan reads. Added `RecursionError` - to `effective_profile_snapshot` Pydantic validator for consistency - with `automation_profile` deserialization. Validator error message - now uses length-only to avoid potential information disclosure. - Documented intentional column naming conventions vs spec DDL. - Migration orphan-row fallback now logs affected row count. - Documented FK ondelete policy drift between ORM and migrated schemas. - Documented `automation_profile` dual-format storage semantic. - Added `TypeError` to `effective_profile_snapshot` deserialization - exception list in `to_domain()` for consistency with the Pydantic - validator. Extracted default automation profile name to a - module-level constant (`DEFAULT_AUTOMATION_PROFILE`) to reduce - sentinel duplication across `models.py` and `repositories.py`. - Migration cycle-detection now logs affected `plan_id` values - (truncated to first 50) before the orphan fallback runs. - Migration SQL statements - uniformly use `sa.text()` for consistency. Centralised - automation-profile serialisation into - `LifecyclePlanModel._serialize_automation_profile()` to - eliminate duplication between `from_domain()` and - `LifecyclePlanRepository.update()`. - Moved `root_plan_id` self-reference resolution from - `from_domain()` into a `PlanIdentity` `model_validator` so - the domain model is consistent with the DB ``NOT NULL`` - constraint before and after persistence. - Used explicit ``is not None`` check in - `_serialize_automation_profile()` for consistency with the - explicit-None-check convention used elsewhere in this commit. - (#921) - Fixed `shell=True` subprocess usage in `cli_coverage_steps.py` by replacing with `shlex.split()` and `shell=False` for defense-in-depth command injection prevention, consistent with the existing pattern in @@ -558,12 +216,6 @@ incorrectly loses to plan-level fallback (level 4). Two regression guard scenarios verify existing correct behaviour (plan override vs project override, project override vs host default). (#1101) -- Added TDD bug-capture tests for bug #1038 — `agents validation add` - missing `--required`/`--informational` flags. Four Behave BDD scenarios - (`@tdd_bug @tdd_bug_1038 @tdd_expected_fail`) verify that the `add` - command accepts `--required` and `--informational` flags and that - `--required` overrides the YAML config mode. Tests use - `@tdd_expected_fail` until the bug fix is merged. (#1102) - Added BuiltinAdapter class and MCP automatic resource slot creation. BuiltinAdapter wraps register_file_tools/register_git_tools/register_subplan_tool into a unified adapter interface. McpAdapter.infer_resource_slots() analyzes @@ -575,13 +227,6 @@ - Added overlay filesystem sandbox strategy with real OverlayFS support and userspace copy-tree fallback. Includes lifecycle management, diff computation, and sandbox factory registration. (#880) -- Added Kubernetes Helm chart in `k8s/` directory for server deployment. - Chart includes Deployment, Service, Ingress (with TLS termination), - ConfigMap, ServiceAccount, Secrets, and optional Redis subchart for - multi-instance session affinity. Added `Dockerfile.server` for ASGI - server containerization with multi-stage build, non-root user, and - pinned uv version. Includes deployment README with configuration - reference and quick-start instructions. (#928) - Added CLI polish infrastructure: shared constants.py (exit codes, format constants), centralized errors.py (cli_error, cli_not_found, cli_warning), and completion command for shell tab-completion generation. (#861) @@ -622,12 +267,7 @@ `EXECUTE_TYPES`. Code relying on `is_strategize_type` or `is_execute_type` returning `False` for `resource_selection` will see different results. Reclassification aligns with ADR-007 L72 and ADR-033 L74 which permit - resource selection during planning. (#931) -- Added E2E test for Workflow Example 4: Multi-Project Dependency Update - (supervised profile). Exercises the full supervised plan lifecycle across 4 - git repositories (common-lib + 3 services), validates child plan spawning, - dependency-ordered execution and apply, per-project validation attachment, - and automation profile enforcement via the `agents plan use` CLI. (#750) + resource selection during planning. (#931) - Added ResourceHandler CRUD and discovery methods: read, write, delete, list_children, diff, and discover_children. Frozen dataclass result types (Content, WriteResult, DeleteResult, DiffResult) added to the handler diff --git a/features/audit_session_race.feature b/features/audit_session_race.feature new file mode 100644 index 000000000..52b0efb92 --- /dev/null +++ b/features/audit_session_race.feature @@ -0,0 +1,27 @@ +# This test captures bug #991 — AuditService._ensure_session() TOCTOU race. +# +# The @tdd_expected_fail tag inverts the test result: the underlying assertion +# fails (proving the bug exists) but CI reports the scenario as passed. +# When #991 is fixed, the @tdd_expected_fail tag must be removed. +# +# See: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/991 + +@tdd_expected_fail @tdd_bug @tdd_bug_991 @tdd_issue @tdd_issue_991 +Feature: TDD Bug #991 — AuditService._ensure_session() TOCTOU race + AuditService._ensure_session() checks ``if self._session is None``, then + creates an engine, runs create_all, builds a sessionmaker, and assigns + self._session — all without a threading.Lock. If two threads call + _ensure_session() concurrently, both see self._session is None, both + create separate engines and sessions, and the second assignment wins, + leaking the first engine/session. + + This test proves the race exists by launching multiple threads that call + _ensure_session() simultaneously via a threading.Barrier and verifying + that create_engine is called exactly once (the correct, thread-safe + behavior). Without the fix, create_engine is called multiple times. + + Scenario: Concurrent _ensure_session() calls must create only one engine + Given an AuditService with no pre-injected session and a temp database + When 10 threads call _ensure_session concurrently through a barrier + Then create_engine should have been called exactly once + And all 10 threads should have completed with a session or error diff --git a/features/steps/audit_session_race_steps.py b/features/steps/audit_session_race_steps.py new file mode 100644 index 000000000..3fd2f4a25 --- /dev/null +++ b/features/steps/audit_session_race_steps.py @@ -0,0 +1,233 @@ +"""Step definitions for bug #991 — AuditService._ensure_session() TOCTOU race. + +This module provides the Behave step implementations for the TDD bug-capture +test that proves bug #991 exists. The bug is a Time-Of-Check-to-Time-Of-Use +(TOCTOU) race in ``AuditService._ensure_session()``: the method checks +``if self._session is None`` and then creates an engine, runs ``create_all``, +builds a ``sessionmaker``, and assigns ``self._session`` — all without holding +a ``threading.Lock``. Concurrent callers can each see ``_session is None`` +and create duplicate engines, leaking all but the last. + +The test uses ``@tdd_expected_fail`` so that CI passes while the bug is +unfixed. The underlying assertion ("create_engine called exactly once") +fails because the race allows multiple threads to trigger creation. The +tag inversion causes the scenario to be reported as passed. + +Race detection uses ``unittest.mock.patch`` on ``create_engine`` within the +``audit_service`` module to count actual calls. This is deterministic and +avoids the timing-dependent ``was_none`` proxy that cannot distinguish +between "thread observed None then raced" and "thread correctly entered the +creation branch". + +When bug #991 is fixed (by adding a ``threading.Lock``), the assertion will +pass and the ``@tdd_expected_fail`` tag must be removed from the feature file. + +**Accepted limitations (self-QA):** + +*Setup asserts under ``@tdd_expected_fail``* — The ``When`` step contains +infrastructure assertions (barrier synchronisation, thread liveness) that +would cause the scenario to fail if the test harness itself is broken. +Under ``@tdd_expected_fail``, any assertion failure is inverted to a pass, +so a broken barrier would appear as an unexpected-pass rather than a clear +setup error. This is an accepted trade-off: the barrier and thread-alive +checks have never failed in practice, and the alternative — splitting the +scenario into a non-``@tdd_expected_fail`` setup scenario and a separate +assertion scenario — would add complexity for negligible benefit. + +*Timing-sensitive race detection* — The ``threading.Barrier`` maximises the +probability of the race manifesting but cannot guarantee it on every run. +On a lightly loaded single-core machine the OS scheduler may serialise the +threads so that only one enters ``_ensure_session()`` at a time, producing +a false-negative (``create_engine`` called once even without a lock). In +practice, 10 threads with a barrier reliably triggers multiple calls on all +CI and developer hardware tested. If flaky false-negatives appear, increase +the thread count or add a small ``time.sleep`` after the barrier to widen +the race window. + +See: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/991 +""" + +from __future__ import annotations + +import contextlib +import os +import tempfile +import threading +from unittest.mock import MagicMock, patch + +from behave import given, then, when +from behave.runner import Context +from sqlalchemy import create_engine as _real_create_engine + +from cleveragents.application.services.audit_service import AuditService +from cleveragents.config.settings import Settings + + +def _make_settings(**overrides: str) -> Settings: + """Create a Settings instance with optional field overrides. + + Resets the Settings singleton so a fresh instance is constructed. + This follows the established project pattern used in environment.py + and dozens of other step files. + """ + Settings._instance = None + base = Settings() + if overrides: + return base.model_copy(update=overrides) + return base + + +# ── Given ───────────────────────────────────────────────────────── + + +@given("an AuditService with no pre-injected session and a temp database") +def step_audit_service_no_session(context: Context) -> None: + """Create an AuditService that will use _ensure_session() for lazy init. + + A file-based temp SQLite database is used (not :memory:) because each + ``create_engine("sqlite:///:memory:")`` call produces an independent + in-memory database, which would mask the race condition. A shared + file ensures all threads contend on the same database. + """ + fd, db_path = tempfile.mkstemp(suffix=".db", prefix="test_race_991_") + os.close(fd) + # Remove the empty file so create_engine creates it fresh + os.unlink(db_path) + context.db_path = db_path + db_url = f"sqlite:///{db_path}" + + settings = _make_settings(database_url=db_url) + # Do NOT pass session= so that _ensure_session() performs lazy init + context.service = AuditService(settings=settings, database_url=db_url) + context.thread_sessions: list[object] = [] + context.thread_errors: list[Exception] = [] + context.engine_create_count = 0 + + # Register cleanup to dispose engine/session and remove temp database + # files (M4: prevent SQLAlchemy resource leaks). + def _cleanup_db(path: str = db_path) -> None: + svc: AuditService = context.service + if svc._session is not None: + svc._session.close() + if svc._session.bind is not None: + svc._session.bind.dispose() + for suffix in ("", "-journal", "-wal", "-shm"): + with contextlib.suppress(OSError): + os.unlink(path + suffix) + + context.add_cleanup(_cleanup_db) + + +# ── When ────────────────────────────────────────────────────────── + + +@when("{n:d} threads call _ensure_session concurrently through a barrier") +def step_concurrent_ensure_session(context: Context, n: int) -> None: + """Launch *n* threads that all call ``_ensure_session()`` simultaneously. + + A ``threading.Barrier`` synchronises the threads so they all enter + ``_ensure_session()`` at (approximately) the same instant, maximising + the chance of the TOCTOU race manifesting. + + Race detection uses ``unittest.mock.patch`` on ``create_engine`` within + the ``audit_service`` module to count actual invocations. The mock + wraps the real ``create_engine`` so the database is still created, but + we can inspect ``mock.call_args_list`` afterward for a deterministic race + indicator. + """ + barrier = threading.Barrier(n) + sessions: list[object] = [] + errors: list[Exception] = [] + collect_lock = threading.Lock() + + # Wrap create_engine so real engines are created but calls are counted + engine_mock = MagicMock(side_effect=_real_create_engine) + + def worker() -> None: + """Thread worker: wait on barrier, then call _ensure_session().""" + try: + barrier.wait(timeout=10) + session = context.service._ensure_session() + with collect_lock: + sessions.append(session) + except Exception as exc: + with collect_lock: + errors.append(exc) + + with patch( + "cleveragents.application.services.audit_service.create_engine", + engine_mock, + ): + threads = [threading.Thread(target=worker, daemon=True) for _ in range(n)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=30) + + # M3: Verify no threads are still alive after join timeout + for t in threads: + assert not t.is_alive(), ( + f"Thread {t.name} is still alive after join timeout — " + f"possible deadlock in _ensure_session()" + ) + + barrier_errors = [ + err for err in errors if isinstance(err, threading.BrokenBarrierError) + ] + assert not barrier_errors, ( + "Barrier synchronization failed while coordinating concurrent " + "_ensure_session() calls. This invalidates race setup. " + f"Barrier errors: {barrier_errors}; all errors: {errors}" + ) + + context.engine_create_count = len(engine_mock.call_args_list) + + context.thread_sessions = sessions + context.thread_errors = errors + context.thread_count = n + + +# ── Then ────────────────────────────────────────────────────────── + + +@then("create_engine should have been called exactly once") +def step_create_engine_called_once(context: Context) -> None: + """Assert create_engine was called exactly once. + + With the bug present (no threading.Lock), multiple threads will enter + the ``if self._session is None`` branch and each call ``create_engine``, + so the mock's call list length will be > 1. + + This assertion therefore **fails** while the bug exists — which is + the expected behavior for a ``@tdd_expected_fail`` test. + + When the fix adds a lock, only the first thread calls create_engine + and the count is exactly 1. + """ + count = context.engine_create_count + assert count == 1, ( + f"create_engine was called {count} time(s), expected exactly 1. " + f"This confirms the TOCTOU race in _ensure_session() — multiple " + f"threads saw _session as None because there is no threading.Lock. " + f"Thread errors (if any): {context.thread_errors}" + ) + + +@then("all {n:d} threads should have completed with a session or error") +def step_all_threads_completed(context: Context, n: int) -> None: + """Verify that all *n* threads completed — either with a session or error. + + The total of successful sessions plus errors must equal the number of + threads launched (non-tautological assertion). This confirms no threads + silently hung or crashed. With the unfixed bug some threads may receive + an error (e.g. from a disposed engine); once #991 is fixed all threads + will succeed with no errors. + """ + total = len(context.thread_sessions) + len(context.thread_errors) + assert total == n, ( + f"Expected {n} threads to complete but only {total} did " + f"({len(context.thread_sessions)} succeeded, " + f"{len(context.thread_errors)} errored). " + "Each worker should report exactly one outcome (session or error). " + "A mismatch indicates a hung thread or missing outcome capture." + )