diff --git a/benchmarks/decision_di_bench.py b/benchmarks/decision_di_bench.py index deaa9dd2..5cdf9574 100644 --- a/benchmarks/decision_di_bench.py +++ b/benchmarks/decision_di_bench.py @@ -11,10 +11,11 @@ from __future__ import annotations import sys from datetime import UTC, datetime from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import create_autospec try: from cleveragents.application.services.decision_service import DecisionService + from cleveragents.config.settings import Settings from cleveragents.domain.models.core.action import Action, ActionState from cleveragents.domain.models.core.decision import Decision, DecisionType from cleveragents.domain.models.core.plan import ( @@ -25,11 +26,11 @@ try: PlanTimestamps, ProcessingState, ) - from cleveragents.infrastructure.database.models import Base from cleveragents.infrastructure.database.unit_of_work import UnitOfWork except ModuleNotFoundError: sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) from cleveragents.application.services.decision_service import DecisionService + from cleveragents.config.settings import Settings from cleveragents.domain.models.core.action import Action, ActionState from cleveragents.domain.models.core.decision import Decision, DecisionType from cleveragents.domain.models.core.plan import ( @@ -40,31 +41,16 @@ except ModuleNotFoundError: PlanTimestamps, ProcessingState, ) - from cleveragents.infrastructure.database.models import Base from cleveragents.infrastructure.database.unit_of_work import UnitOfWork -from sqlalchemy import create_engine -from sqlalchemy.orm import sessionmaker _PLAN_ID = "01HV00000000000000DIBENCH1" def _make_uow() -> UnitOfWork: """Create a UoW backed by an in-memory SQLite database.""" - engine = create_engine("sqlite:///:memory:", echo=False) - Base.metadata.create_all(engine) - - uow = UnitOfWork.__new__(UnitOfWork) - uow.database_url = "sqlite:///:memory:" - uow._engine = engine - uow._session_factory = sessionmaker( - bind=engine, - expire_on_commit=False, - autoflush=False, - autocommit=False, - ) - uow._database_initialized = True - uow._prompt_for_migration = None + uow = UnitOfWork("sqlite:///:memory:") + uow.init_database() return uow @@ -140,7 +126,9 @@ class TimeDecisionRecord: def setup(self) -> None: self.uow = _make_uow() _seed_prerequisites(self.uow) - self.svc = DecisionService(settings=MagicMock(), unit_of_work=self.uow) + mock_settings = create_autospec(Settings, instance=True) + mock_settings.database_url = "sqlite:///:memory:" + self.svc = DecisionService(settings=mock_settings, unit_of_work=self.uow) self._seq = 100 def time_decision_record(self) -> None: @@ -161,7 +149,9 @@ class TimeDecisionTreeRetrieval: def setup(self) -> None: self.uow = _make_uow() _seed_prerequisites(self.uow) - self.svc = DecisionService(settings=MagicMock(), unit_of_work=self.uow) + mock_settings = create_autospec(Settings, instance=True) + mock_settings.database_url = "sqlite:///:memory:" + self.svc = DecisionService(settings=mock_settings, unit_of_work=self.uow) # Build a small tree root = self.svc.record_decision( diff --git a/features/decision_recording.feature b/features/decision_recording.feature index 5505989a..b984862b 100644 --- a/features/decision_recording.feature +++ b/features/decision_recording.feature @@ -63,6 +63,25 @@ Feature: Decision recording and snapshot store Then a dsvc validation error should be raised And the dsvc error should mention "chosen_option" + Scenario: Recording with invalid decision_type raises ValueError + When I try to record a decision with invalid type "nonexistent_type" + Then a dsvc value error should be raised + And the dsvc error should mention "nonexistent_type" + + Scenario: Recording with confidence_score at boundaries + When I record a strategy_choice decision for plan "P1" with confidence 0.0 + Then the dsvc decision confidence score should be 0.0 + When I record a strategy_choice decision for plan "P1" with confidence 1.0 + Then the dsvc decision confidence score should be 1.0 + + Scenario: Recording with negative confidence_score raises ValidationError + When I try to record a decision with confidence -0.1 + Then a dsvc validation error should be raised + + Scenario: Recording with confidence_score above 1.0 raises ValidationError + When I try to record a decision with confidence 1.1 + Then a dsvc validation error should be raised + Scenario: Auto-capture generates a snapshot hash When I record a strategy_choice decision for plan "P1" with question "Test auto-capture" Then the dsvc decision context snapshot hash should start with "sha256:" @@ -104,9 +123,11 @@ Feature: Decision recording and snapshot store When I record a prompt_definition decision for plan "P1" with question "Root" And I record a strategy_choice decision for plan "P1" with parent as child "Child 1" And I record a strategy_choice decision for plan "P1" with parent as child "Child 2" + And I record an implementation_choice decision for plan "P1" with second parent as child "Grandchild" And I get the tree for plan "P1" - Then the dsvc tree should have 3 decisions + Then the dsvc tree should have 4 decisions And the dsvc first tree decision should be a root decision + And the dsvc tree should be in BFS level order Scenario: Get tree for empty plan returns empty list When I get the tree for plan "P_EMPTY" @@ -370,3 +391,13 @@ Feature: Decision recording and snapshot store When I record a prompt_definition decision for plan "P1" with question "Original" And I try to store a duplicate of the recorded decision Then a dsvc duplicate decision error should be raised + + Scenario: Persisted sequence resumes after service restart + Given a persisted decision service + When I record a prompt_definition decision for plan "P1" with question "Before restart" + And I record a strategy_choice decision for plan "P1" with question "Also before restart" + Then the dsvc decisions should have sequence numbers 0 1 + When I recreate the decision service with the same database + And I record a strategy_choice decision for plan "P1" with question "After restart" + Then the dsvc decision sequence number should be 2 + And the dsvc next sequence for plan "P1" should be 3 diff --git a/features/decision_service_coverage.feature b/features/decision_service_coverage.feature index e56eb7c3..5b84fdeb 100644 --- a/features/decision_service_coverage.feature +++ b/features/decision_service_coverage.feature @@ -22,18 +22,15 @@ Feature: DecisionService application-layer coverage Scenario: dsvc- record_decision persists and returns the decision Given dsvc- a DecisionService with a mocked UnitOfWork - And dsvc- a sample root Decision object - When dsvc- I call record_decision with the sample decision + When dsvc- I call record_decision with plan_id and required args Then dsvc- the UoW transaction should have been entered - And dsvc- ctx.decisions.create should have been called with the decision - And dsvc- the returned decision should be the same object + And dsvc- ctx.decisions.create should have been called once + And dsvc- the returned decision should have the correct plan_id - Scenario: dsvc- record_decision logs info and debug messages + Scenario: dsvc- record_decision logs info on success Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- a sample root Decision object - When dsvc- I call record_decision with the sample decision - Then dsvc- the logger should have recorded an info call with "recording_decision" - And dsvc- the logger should have recorded a debug call with "decision_recorded" + When dsvc- I call record_decision with plan_id and required args + Then dsvc- the logger should have recorded an info call with "decision.recorded" # ------------------------------------------------------------------ # get_decision @@ -46,57 +43,38 @@ Feature: DecisionService application-layer coverage Then dsvc- ctx.decisions.get should have been called with the ID And dsvc- the returned value should be the expected Decision - Scenario: dsvc- get_decision returns None when not found + Scenario: dsvc- get_decision raises DecisionNotFoundError when not found Given dsvc- a DecisionService with a mocked UnitOfWork And dsvc- the mock repo get method returns None - When dsvc- I call get_decision with an unknown ID - Then dsvc- the returned value should be None - - Scenario: dsvc- get_decision logs a debug message - Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- the mock repo get method returns a Decision - When dsvc- I call get_decision with a known ID - Then dsvc- the logger should have recorded a debug call with "getting_decision" + When dsvc- I call get_decision with an unknown ID expecting not-found + Then dsvc- a DecisionNotFoundError should have been raised # ------------------------------------------------------------------ - # get_decisions_for_plan + # list_decisions # ------------------------------------------------------------------ - Scenario: dsvc- get_decisions_for_plan returns list of decisions + Scenario: dsvc- list_decisions returns list of decisions Given dsvc- a DecisionService with a mocked UnitOfWork And dsvc- the mock repo get_by_plan method returns 3 decisions - When dsvc- I call get_decisions_for_plan with a plan ID + When dsvc- I call list_decisions with a plan ID Then dsvc- ctx.decisions.get_by_plan should have been called with the plan ID And dsvc- the returned list should have 3 decisions - Scenario: dsvc- get_decisions_for_plan returns empty list when none exist + Scenario: dsvc- list_decisions returns empty list when none exist Given dsvc- a DecisionService with a mocked UnitOfWork And dsvc- the mock repo get_by_plan method returns 0 decisions - When dsvc- I call get_decisions_for_plan with a plan ID + When dsvc- I call list_decisions with a plan ID Then dsvc- the returned list should have 0 decisions - Scenario: dsvc- get_decisions_for_plan logs a debug message - Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- the mock repo get_by_plan method returns 3 decisions - When dsvc- I call get_decisions_for_plan with a plan ID - Then dsvc- the logger should have recorded a debug call with "getting_decisions_for_plan" - # ------------------------------------------------------------------ - # get_decision_tree + # get_tree # ------------------------------------------------------------------ - Scenario: dsvc- get_decision_tree returns BFS-ordered list + Scenario: dsvc- get_tree returns BFS-ordered list Given dsvc- a DecisionService with a mocked UnitOfWork - And dsvc- the mock repo get_tree method returns 4 decisions - When dsvc- I call get_decision_tree with a root ID - Then dsvc- ctx.decisions.get_tree should have been called with the root ID - And dsvc- the returned tree list should have 4 decisions - - Scenario: dsvc- get_decision_tree logs a debug message - Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- the mock repo get_tree method returns 4 decisions - When dsvc- I call get_decision_tree with a root ID - Then dsvc- the logger should have recorded a debug call with "getting_decision_tree" + And dsvc- the mock repo get_by_plan method returns 4 decisions as a tree + When dsvc- I call get_tree with a plan ID + Then dsvc- the returned tree list should have 4 decisions # ------------------------------------------------------------------ # get_path_to_root @@ -109,18 +87,13 @@ Feature: DecisionService application-layer coverage Then dsvc- ctx.decisions.get_path_to_root should have been called with the leaf ID And dsvc- the returned path list should have 3 decisions - Scenario: dsvc- get_path_to_root logs a debug message - Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- the mock repo get_path_to_root method returns 3 decisions - When dsvc- I call get_path_to_root with a leaf ID - Then dsvc- the logger should have recorded a debug call with "getting_path_to_root" - # ------------------------------------------------------------------ # mark_superseded # ------------------------------------------------------------------ Scenario: dsvc- mark_superseded updates and returns the decision Given dsvc- a DecisionService with a mocked UnitOfWork + And dsvc- the mock repo has the replacement decision in cache And dsvc- the mock repo update_superseded_by method returns a superseded Decision When dsvc- I call mark_superseded with old and new IDs Then dsvc- ctx.decisions.update_superseded_by should have been called with both IDs @@ -128,9 +101,10 @@ Feature: DecisionService application-layer coverage Scenario: dsvc- mark_superseded logs info with both IDs Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger + And dsvc- the mock repo has the replacement decision in cache And dsvc- the mock repo update_superseded_by method returns a superseded Decision When dsvc- I call mark_superseded with old and new IDs - Then dsvc- the logger should have recorded an info call with "marking_superseded" + Then dsvc- the logger should have recorded an info call with "decision.superseded" # ------------------------------------------------------------------ # list_by_type @@ -148,9 +122,3 @@ Feature: DecisionService application-layer coverage And dsvc- the mock repo list_by_type method returns 0 decisions When dsvc- I call list_by_type with plan ID and type "error_recovery" Then dsvc- the returned type list should have 0 decisions - - Scenario: dsvc- list_by_type logs a debug message - Given dsvc- a DecisionService with a mocked UnitOfWork and captured logger - And dsvc- the mock repo list_by_type method returns 2 decisions - When dsvc- I call list_by_type with plan ID and type "strategy_choice" - Then dsvc- the logger should have recorded a debug call with "listing_by_type" diff --git a/features/steps/decision_di_wiring_steps.py b/features/steps/decision_di_wiring_steps.py index 7c996544..7cb48c8c 100644 --- a/features/steps/decision_di_wiring_steps.py +++ b/features/steps/decision_di_wiring_steps.py @@ -10,16 +10,33 @@ existing step definitions. from __future__ import annotations import os -from unittest.mock import MagicMock +from typing import TYPE_CHECKING +from unittest.mock import create_autospec from behave import given, then, when from behave.runner import Context +if TYPE_CHECKING: + from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + # ------------------------------------------------------------------- # Helpers # ------------------------------------------------------------------- +def _make_test_uow() -> UnitOfWork: + """Create a UnitOfWork backed by an in-memory SQLite for testing. + + Uses ``UnitOfWork.__init__`` + ``init_database()`` rather than the + ``__new__()`` anti-pattern to stay in sync with UoW internals. + """ + from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + + uow = UnitOfWork("sqlite:///:memory:") + uow.init_database() + return uow + + def _ensure_test_db_env(context: Context) -> None: """Set up a test database URL if not already set.""" if not hasattr(context, "_decdi_env_saved"): @@ -81,34 +98,15 @@ def step_check_decision_service(context: Context) -> None: @given("decdi- a PlanLifecycleService with a DecisionService") def step_create_lifecycle_with_decision(context: Context) -> None: """Build an in-memory PlanLifecycleService wired with DecisionService.""" - from sqlalchemy import create_engine - from sqlalchemy.orm import sessionmaker - from cleveragents.application.services.decision_service import DecisionService from cleveragents.application.services.plan_lifecycle_service import ( PlanLifecycleService, ) - from cleveragents.infrastructure.database.models import Base - from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + from cleveragents.config.settings import Settings - # Build a fresh in-memory DB - engine = create_engine("sqlite:///:memory:", echo=False) - Base.metadata.create_all(engine) + uow = _make_test_uow() - # Create a UoW that skips migration - uow = UnitOfWork.__new__(UnitOfWork) - uow.database_url = "sqlite:///:memory:" - uow._engine = engine - uow._session_factory = sessionmaker( - bind=engine, - expire_on_commit=False, - autoflush=False, - autocommit=False, - ) - uow._database_initialized = True - uow._prompt_for_migration = None - - mock_settings = MagicMock() + mock_settings = create_autospec(Settings, instance=True) mock_settings.database_url = "sqlite:///:memory:" decision_svc = DecisionService(settings=mock_settings, unit_of_work=uow) @@ -147,8 +145,8 @@ def step_check_strategize_decision(context: Context) -> None: strategy_decisions = [ d for d in decisions if str(d.decision_type) == "strategy_choice" ] - assert len(strategy_decisions) >= 1, ( - f"Expected at least 1 strategy_choice decision, got {len(strategy_decisions)}" + assert len(strategy_decisions) == 1, ( + f"Expected exactly 1 strategy_choice decision, got {len(strategy_decisions)}" ) @@ -196,8 +194,8 @@ def step_check_execute_decision(context: Context) -> None: impl_decisions = [ d for d in decisions if str(d.decision_type) == "implementation_choice" ] - assert len(impl_decisions) >= 1, ( - f"Expected at least 1 implementation_choice decision, got {len(impl_decisions)}" + assert len(impl_decisions) == 1, ( + f"Expected exactly 1 implementation_choice decision, got {len(impl_decisions)}" ) diff --git a/features/steps/decision_recording_steps.py b/features/steps/decision_recording_steps.py index e036daa0..da55104c 100644 --- a/features/steps/decision_recording_steps.py +++ b/features/steps/decision_recording_steps.py @@ -326,6 +326,37 @@ def step_try_empty_chosen(context: Context) -> None: context.decision_error = exc +@when('I try to record a decision with invalid type "{dtype}"') +def step_try_invalid_type(context: Context, dtype: str) -> None: + try: + context.decision_service.record_decision( + plan_id="01ARZ3NDEKTSV4RRFFQ69G5FAV", + decision_type=dtype, + question="Q", + chosen_option="A", + ) + context.decision_error = None + except (ValueError, ValidationError) as exc: + context.decision_error = exc + + +@when("I try to record a decision with confidence {score:g}") +def step_try_invalid_confidence(context: Context, score: float) -> None: + import pydantic + + try: + context.decision_service.record_decision( + plan_id="01ARZ3NDEKTSV4RRFFQ69G5FAV", + decision_type=DecisionType.STRATEGY_CHOICE, + question="Q", + chosen_option="A", + confidence_score=score, + ) + context.decision_error = None + except (ValidationError, pydantic.ValidationError) as exc: + context.decision_error = exc + + # --------------------------------------------------------------------------- # When — Retrieval # --------------------------------------------------------------------------- @@ -628,8 +659,12 @@ def step_snapshot_hash_prefix(context: Context, prefix: str) -> None: @then("a dsvc validation error should be raised") def step_validation_error(context: Context) -> None: + import pydantic + assert context.decision_error is not None - assert isinstance(context.decision_error, (ValidationError, ValueError)) + assert isinstance( + context.decision_error, (ValidationError, ValueError, pydantic.ValidationError) + ) @then('the dsvc error should mention "{text}"') @@ -637,6 +672,12 @@ def step_error_mention(context: Context, text: str) -> None: assert text in str(context.decision_error) +@then("a dsvc value error should be raised") +def step_value_error(context: Context) -> None: + assert context.decision_error is not None + assert isinstance(context.decision_error, ValueError) + + @then("a dsvc decision not found error should be raised") def step_not_found_error(context: Context) -> None: assert context.decision_error is not None @@ -680,6 +721,40 @@ def step_tree_root(context: Context) -> None: assert context.tree_result[0].is_root +@then("the dsvc tree should be in BFS level order") +def step_tree_bfs_order(context: Context) -> None: + """Verify that all nodes at depth *d* appear before any node at depth *d+1*. + + We compute each node's depth by walking ``parent_decision_id`` links, + then assert the depth sequence is non-decreasing. + """ + tree: list[Decision] = context.tree_result + if len(tree) <= 1: + return # trivially ordered + + # Build a lookup: decision_id -> Decision + by_id = {d.decision_id: d for d in tree} + + def _depth(node: Decision) -> int: + depth = 0 + current = node + while current.parent_decision_id is not None: + parent = by_id.get(current.parent_decision_id) + if parent is None: + break + depth += 1 + current = parent + return depth + + depths = [_depth(d) for d in tree] + for i in range(1, len(depths)): + assert depths[i] >= depths[i - 1], ( + f"BFS order violated at index {i}: " + f"depth {depths[i]} follows depth {depths[i - 1]}. " + f"Depths: {depths}" + ) + + @then("the dsvc path should have {count:d} decisions") def step_path_count(context: Context, count: int) -> None: assert len(context.path_result) == count @@ -994,6 +1069,27 @@ def step_persisted_decision_service(context: Context) -> None: context._cleanup_handlers.append(_cleanup_db) +@when("I recreate the decision service with the same database") +def step_recreate_service_same_db(context: Context) -> None: + """Destroy the current DecisionService and create a fresh one against the same DB. + + This simulates a service restart: the in-memory sequence counters + are lost and must be rehydrated from the database. + """ + from cleveragents.infrastructure.database.unit_of_work import UnitOfWork + + db_path = context._dsvc_db_path + uow = UnitOfWork(f"sqlite:///{db_path}") + uow.init_database() + + context.decision_service = DecisionService(unit_of_work=uow) + # Preserve _plan_id_registry so symbolic IDs ("P1") still resolve + # to the same ULIDs used before the restart. + context.recorded_decisions = [] + context.decision_error = None + context.decision_result = None + + @when("I try to store a duplicate of the recorded decision") def step_try_store_duplicate(context: Context) -> None: """Attempt to store a decision with the same ID that already exists.""" diff --git a/features/steps/decision_service_coverage_steps.py b/features/steps/decision_service_coverage_steps.py index cd542275..d9eda5b1 100644 --- a/features/steps/decision_service_coverage_steps.py +++ b/features/steps/decision_service_coverage_steps.py @@ -18,7 +18,10 @@ from behave import given, then, when from behave.runner import Context from ulid import ULID -from cleveragents.application.services.decision_service import DecisionService +from cleveragents.application.services.decision_service import ( + DecisionNotFoundError, + DecisionService, +) from cleveragents.domain.models.core.decision import ( Decision, DecisionType, @@ -126,7 +129,6 @@ def step_dsvc_check_uow(context: Context) -> None: @then("dsvc- the service should have a bound structlog logger") def step_dsvc_check_logger(context: Context) -> None: - # The _logger is a BoundLogger with service="decision" assert hasattr(context.dsvc_service, "_logger"), "Service missing _logger attribute" @@ -174,37 +176,34 @@ def step_dsvc_service_with_logger(context: Context) -> None: # --------------------------------------------------------------------------- -@given("dsvc- a sample root Decision object") -def step_dsvc_sample_decision(context: Context) -> None: - context.dsvc_decision = _make_decision(plan_id=_PLAN_ID) - - -@when("dsvc- I call record_decision with the sample decision") +@when("dsvc- I call record_decision with plan_id and required args") def step_dsvc_call_record(context: Context) -> None: context.dsvc_result = context.dsvc_service.record_decision( - context.dsvc_decision, + plan_id=_PLAN_ID, + decision_type=DecisionType.PROMPT_DEFINITION, + question="Which approach?", + chosen_option="REST API", ) @then("dsvc- the UoW transaction should have been entered") def step_dsvc_txn_entered(context: Context) -> None: - # The fact that ctx.decisions.create was called proves we entered - # the transaction context manager successfully. We verify that - # the mock context's decisions repo was accessed. assert context.dsvc_decisions.create.called, ( "Transaction context was not entered — create was never called" ) -@then("dsvc- ctx.decisions.create should have been called with the decision") +@then("dsvc- ctx.decisions.create should have been called once") def step_dsvc_create_called(context: Context) -> None: - context.dsvc_decisions.create.assert_called_once_with(context.dsvc_decision) + assert context.dsvc_decisions.create.call_count == 1, ( + f"Expected create to be called once, got {context.dsvc_decisions.create.call_count}" + ) -@then("dsvc- the returned decision should be the same object") -def step_dsvc_record_returns_same(context: Context) -> None: - assert context.dsvc_result is context.dsvc_decision, ( - "record_decision should return the same Decision instance" +@then("dsvc- the returned decision should have the correct plan_id") +def step_dsvc_record_returns_correct_plan(context: Context) -> None: + assert context.dsvc_result.plan_id == _PLAN_ID, ( + f"Expected plan_id '{_PLAN_ID}', got '{context.dsvc_result.plan_id}'" ) @@ -220,13 +219,6 @@ def step_dsvc_logger_info(context: Context, event: str) -> None: assert event in events, f"Expected info event '{event}' in {events}" -@then('dsvc- the logger should have recorded a debug call with "{event}"') -def step_dsvc_logger_debug(context: Context, event: str) -> None: - debug_calls = context.dsvc_logger.debug.call_args_list - events = [c.args[0] if c.args else c.kwargs.get("event", "") for c in debug_calls] - assert event in events, f"Expected debug event '{event}' in {events}" - - # --------------------------------------------------------------------------- # get_decision # --------------------------------------------------------------------------- @@ -251,9 +243,13 @@ def step_dsvc_call_get(context: Context) -> None: context.dsvc_result = context.dsvc_service.get_decision(_DECISION_ID) -@when("dsvc- I call get_decision with an unknown ID") +@when("dsvc- I call get_decision with an unknown ID expecting not-found") def step_dsvc_call_get_unknown(context: Context) -> None: - context.dsvc_result = context.dsvc_service.get_decision("NONEXISTENT_ID_12345678") + try: + context.dsvc_service.get_decision("NONEXISTENT_ID_12345678") + context.dsvc_error = None + except DecisionNotFoundError as exc: + context.dsvc_error = exc @then("dsvc- ctx.decisions.get should have been called with the ID") @@ -268,13 +264,14 @@ def step_dsvc_get_returns_expected(context: Context) -> None: ) -@then("dsvc- the returned value should be None") -def step_dsvc_result_is_none(context: Context) -> None: - assert context.dsvc_result is None, f"Expected None, got {context.dsvc_result!r}" +@then("dsvc- a DecisionNotFoundError should have been raised") +def step_dsvc_not_found_raised(context: Context) -> None: + assert context.dsvc_error is not None, "Expected DecisionNotFoundError but got None" + assert isinstance(context.dsvc_error, DecisionNotFoundError) # --------------------------------------------------------------------------- -# get_decisions_for_plan +# list_decisions # --------------------------------------------------------------------------- @@ -297,9 +294,9 @@ def step_dsvc_mock_get_by_plan(context: Context, count: int) -> None: context.dsvc_expected_count = count -@when("dsvc- I call get_decisions_for_plan with a plan ID") -def step_dsvc_call_get_for_plan(context: Context) -> None: - context.dsvc_result = context.dsvc_service.get_decisions_for_plan(_PLAN_ID) +@when("dsvc- I call list_decisions with a plan ID") +def step_dsvc_call_list_decisions(context: Context) -> None: + context.dsvc_result = context.dsvc_service.list_decisions(_PLAN_ID) @then("dsvc- ctx.decisions.get_by_plan should have been called with the plan ID") @@ -315,34 +312,48 @@ def step_dsvc_list_count(context: Context, count: int) -> None: # --------------------------------------------------------------------------- -# get_decision_tree +# get_tree # --------------------------------------------------------------------------- - -@given("dsvc- the mock repo get_tree method returns {count:d} decisions") -def step_dsvc_mock_get_tree(context: Context, count: int) -> None: - root = _make_decision(decision_id=_ROOT_ID, plan_id=_PLAN_ID) - decisions = [root] + [ - _make_decision( - plan_id=_PLAN_ID, - sequence_number=i, - decision_type=DecisionType.STRATEGY_CHOICE, - parent_decision_id=_ROOT_ID, - ) - for i in range(1, count) - ] - context.dsvc_decisions.get_tree.return_value = decisions - context.dsvc_expected_tree_count = count +_CHILD1_ID = str(ULID()) +_CHILD2_ID = str(ULID()) +_GRANDCHILD_ID = str(ULID()) -@when("dsvc- I call get_decision_tree with a root ID") +@given("dsvc- the mock repo get_by_plan method returns 4 decisions as a tree") +def step_dsvc_mock_get_tree(context: Context) -> None: + root = _make_decision( + decision_id=_ROOT_ID, + plan_id=_PLAN_ID, + sequence_number=0, + ) + child1 = _make_decision( + decision_id=_CHILD1_ID, + plan_id=_PLAN_ID, + sequence_number=1, + decision_type=DecisionType.STRATEGY_CHOICE, + parent_decision_id=_ROOT_ID, + ) + child2 = _make_decision( + decision_id=_CHILD2_ID, + plan_id=_PLAN_ID, + sequence_number=2, + decision_type=DecisionType.STRATEGY_CHOICE, + parent_decision_id=_ROOT_ID, + ) + grandchild = _make_decision( + decision_id=_GRANDCHILD_ID, + plan_id=_PLAN_ID, + sequence_number=3, + decision_type=DecisionType.IMPLEMENTATION_CHOICE, + parent_decision_id=_CHILD1_ID, + ) + context.dsvc_decisions.get_by_plan.return_value = [root, child1, child2, grandchild] + + +@when("dsvc- I call get_tree with a plan ID") def step_dsvc_call_get_tree(context: Context) -> None: - context.dsvc_result = context.dsvc_service.get_decision_tree(_ROOT_ID) - - -@then("dsvc- ctx.decisions.get_tree should have been called with the root ID") -def step_dsvc_get_tree_called(context: Context) -> None: - context.dsvc_decisions.get_tree.assert_called_once_with(_ROOT_ID) + context.dsvc_result = context.dsvc_service.get_tree(_PLAN_ID) @then("dsvc- the returned tree list should have {count:d} decisions") @@ -398,6 +409,23 @@ def step_dsvc_path_count(context: Context, count: int) -> None: # --------------------------------------------------------------------------- +@given("dsvc- the mock repo has the replacement decision in cache") +def step_dsvc_seed_replacement_in_cache(context: Context) -> None: + """Seed the replacement decision into the service's in-memory cache. + + ``mark_superseded`` now validates that ``new_decision_id`` exists + before proceeding. We add the replacement to ``_decisions`` so + the validation lookup succeeds. + """ + replacement = _make_decision( + decision_id=_NEW_DECISION_ID, + plan_id=_PLAN_ID, + sequence_number=99, + decision_type=DecisionType.STRATEGY_CHOICE, + ) + context.dsvc_service._decisions[_NEW_DECISION_ID] = replacement + + @given("dsvc- the mock repo update_superseded_by method returns a superseded Decision") def step_dsvc_mock_superseded(context: Context) -> None: context.dsvc_superseded_decision = _make_decision( diff --git a/features/steps/plan_explain_cli_coverage_steps.py b/features/steps/plan_explain_cli_coverage_steps.py index 11107d0d..e71223ce 100644 --- a/features/steps/plan_explain_cli_coverage_steps.py +++ b/features/steps/plan_explain_cli_coverage_steps.py @@ -217,7 +217,7 @@ def step_pec_mock_tree_decisions(context: Context) -> None: context.pec_plan_id = str(ULID()) decisions = _make_tree_decisions() svc = MagicMock() - svc.get_decisions_for_plan.return_value = decisions + svc.list_decisions.return_value = decisions context.pec_container = _mock_container_with_decision_svc(svc) @@ -226,7 +226,7 @@ def step_pec_mock_deep_decisions(context: Context) -> None: context.pec_plan_id = str(ULID()) decisions = _make_deep_decisions() svc = MagicMock() - svc.get_decisions_for_plan.return_value = decisions + svc.list_decisions.return_value = decisions context.pec_container = _mock_container_with_decision_svc(svc) @@ -260,7 +260,7 @@ def step_pec_mock_superseded_decisions(context: Context) -> None: ), ] svc = MagicMock() - svc.get_decisions_for_plan.return_value = decisions + svc.list_decisions.return_value = decisions context.pec_container = _mock_container_with_decision_svc(svc) @@ -268,7 +268,7 @@ def step_pec_mock_superseded_decisions(context: Context) -> None: def step_pec_mock_empty_decisions(context: Context) -> None: context.pec_plan_id = str(ULID()) svc = MagicMock() - svc.get_decisions_for_plan.return_value = [] + svc.list_decisions.return_value = [] context.pec_container = _mock_container_with_decision_svc(svc) diff --git a/robot/helper_decision_di.py b/robot/helper_decision_di.py index de5ca06d..b711c261 100644 --- a/robot/helper_decision_di.py +++ b/robot/helper_decision_di.py @@ -13,19 +13,15 @@ from __future__ import annotations import os import sys from pathlib import Path -from unittest.mock import MagicMock +from unittest.mock import create_autospec # Ensure src is importable when run from workspace root sys.path.insert(0, str(Path(__file__).resolve().parents[1] / "src")) -from sqlalchemy import create_engine # noqa: I001 -from sqlalchemy.orm import sessionmaker - -from cleveragents.application.services.decision_service import DecisionService +from cleveragents.application.services.decision_service import DecisionService # noqa: I001 from cleveragents.application.services.plan_lifecycle_service import ( PlanLifecycleService, ) -from cleveragents.infrastructure.database.models import Base from cleveragents.infrastructure.database.unit_of_work import UnitOfWork @@ -36,20 +32,8 @@ from cleveragents.infrastructure.database.unit_of_work import UnitOfWork def _make_uow() -> UnitOfWork: """Create a UoW backed by an in-memory SQLite database.""" - engine = create_engine("sqlite:///:memory:", echo=False) - Base.metadata.create_all(engine) - - uow = UnitOfWork.__new__(UnitOfWork) - uow.database_url = "sqlite:///:memory:" - uow._engine = engine - uow._session_factory = sessionmaker( - bind=engine, - expire_on_commit=False, - autoflush=False, - autocommit=False, - ) - uow._database_initialized = True - uow._prompt_for_migration = None + uow = UnitOfWork("sqlite:///:memory:") + uow.init_database() return uow @@ -84,7 +68,9 @@ def _resolve_service() -> None: def _record_integration() -> None: """Verify decision recording during plan lifecycle transitions.""" uow = _make_uow() - mock_settings = MagicMock() + from cleveragents.config.settings import Settings + + mock_settings = create_autospec(Settings, instance=True) mock_settings.database_url = "sqlite:///:memory:" decision_svc = DecisionService(settings=mock_settings, unit_of_work=uow) diff --git a/robot/helper_decision_recording.py b/robot/helper_decision_recording.py index 9a2c70e3..10fc2cb8 100644 --- a/robot/helper_decision_recording.py +++ b/robot/helper_decision_recording.py @@ -104,7 +104,7 @@ def _tree_bfs(): question="Root", chosen_option="Root choice", ) - svc.record_decision( + child1 = svc.record_decision( plan_id=_PLAN_ID, decision_type=DecisionType.STRATEGY_CHOICE, question="Child 1", @@ -118,9 +118,38 @@ def _tree_bfs(): chosen_option="C2", parent_decision_id=root.decision_id, ) + svc.record_decision( + plan_id=_PLAN_ID, + decision_type=DecisionType.IMPLEMENTATION_CHOICE, + question="Grandchild", + chosen_option="GC", + parent_decision_id=child1.decision_id, + ) tree = svc.get_tree(_PLAN_ID) - assert len(tree) == 3, f"expected 3, got {len(tree)}" + assert len(tree) == 4, f"expected 4, got {len(tree)}" assert tree[0].is_root + + # Verify BFS level order: compute depths and check non-decreasing. + by_id = {d.decision_id: d for d in tree} + + def _depth(node): + depth = 0 + current = node + while current.parent_decision_id is not None: + parent = by_id.get(current.parent_decision_id) + if parent is None: + break + depth += 1 + current = parent + return depth + + depths = [_depth(d) for d in tree] + for i in range(1, len(depths)): + assert depths[i] >= depths[i - 1], ( + f"BFS order violated at index {i}: " + f"depth {depths[i]} follows depth {depths[i - 1]}. " + f"Depths: {depths}" + ) print("tree-bfs-ok") diff --git a/src/cleveragents/application/services/__init__.py b/src/cleveragents/application/services/__init__.py index 52c2cc8e..5ec0076f 100644 --- a/src/cleveragents/application/services/__init__.py +++ b/src/cleveragents/application/services/__init__.py @@ -19,6 +19,7 @@ from cleveragents.application.services.decision_service import ( DecisionNotFoundError, DecisionService, DuplicateDecisionError, + SequenceConflictError, SnapshotStore, ) from cleveragents.application.services.invariant_service import ( @@ -145,6 +146,7 @@ __all__ = [ "SemanticValidationRule", "SemanticValidationService", "SemanticValidationSeverity", + "SequenceConflictError", "SkeletonCompressorService", "SkillRegistryService", "SnapshotStore", diff --git a/src/cleveragents/application/services/decision_service.py b/src/cleveragents/application/services/decision_service.py index e3f3f261..d2785a95 100644 --- a/src/cleveragents/application/services/decision_service.py +++ b/src/cleveragents/application/services/decision_service.py @@ -26,7 +26,7 @@ from __future__ import annotations import hashlib import json from collections import deque -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING import structlog @@ -36,6 +36,7 @@ from cleveragents.core.exceptions import ( ValidationError, ) from cleveragents.domain.models.core.decision import ( + ArtifactRef, ContextSnapshot, Decision, DecisionType, @@ -47,6 +48,15 @@ if TYPE_CHECKING: logger = structlog.get_logger(__name__) +# --------------------------------------------------------------------------- +# Constants +# --------------------------------------------------------------------------- + +#: Maximum length for ``actor_reasoning`` (raw LLM trace). Prevents +#: unbounded storage from a misbehaving model while remaining generous +#: enough for typical chain-of-thought traces. +_MAX_ACTOR_REASONING: int = 100_000 + # --------------------------------------------------------------------------- # Custom exceptions @@ -201,6 +211,7 @@ class DecisionService: self._decisions: dict[str, Decision] = {} self._plan_decisions: dict[str, list[str]] = {} self._plan_sequence: dict[str, int] = {} + self._sequence_initialised: set[str] = set() # Snapshot store self.snapshots = SnapshotStore() @@ -227,7 +238,7 @@ class DecisionService: rationale: str = "", actor_reasoning: str | None = None, context_snapshot: ContextSnapshot | None = None, - artifacts_produced: list[Any] | None = None, + artifacts_produced: list[ArtifactRef] | None = None, is_correction: bool = False, corrects_decision_id: str | None = None, correction_reason: str | None = None, @@ -266,6 +277,10 @@ class DecisionService: raise ValidationError("question must not be empty") if not chosen_option or not chosen_option.strip(): raise ValidationError("chosen_option must not be empty") + if actor_reasoning is not None and len(actor_reasoning) > _MAX_ACTOR_REASONING: + raise ValidationError( + f"actor_reasoning exceeds maximum length of {_MAX_ACTOR_REASONING}" + ) # Coerce string to enum if needed if isinstance(decision_type, str): @@ -422,6 +437,13 @@ class DecisionService: if child.decision_id not in visited: queue.append(child) + # Include orphaned subtrees (decisions unreachable from any root). + # These can arise when a parent decision is deleted. They are + # appended in sequence-number order after the BFS-reachable nodes. + if len(visited) < len(all_decisions): + orphans = [d for d in all_decisions if d.decision_id not in visited] + result.extend(orphans) + return result def get_path_to_root(self, decision_id: str) -> list[Decision]: @@ -491,8 +513,17 @@ class DecisionService: The updated :class:`Decision` with superseded_by set. Raises: - DecisionNotFoundError: If the decision does not exist. + DecisionNotFoundError: If the decision or the replacement + decision does not exist. """ + # Validate the replacement decision exists. + replacement = self._decisions.get(new_decision_id) + if replacement is None and self._persisted and self.unit_of_work is not None: + with self.unit_of_work.transaction() as ctx: + replacement = ctx.decisions.get(new_decision_id) + if replacement is None: + raise DecisionNotFoundError(new_decision_id) + if self._persisted and self.unit_of_work is not None: with self.unit_of_work.transaction() as ctx: result = ctx.decisions.update_superseded_by( @@ -534,23 +565,22 @@ class DecisionService: Raises: DecisionNotFoundError: If the decision does not exist. """ - if self._persisted and self.unit_of_work is not None: + # Check cache first; in persisted mode also consult DB. + decision = self._decisions.get(decision_id) + if decision is None and self._persisted and self.unit_of_work is not None: with self.unit_of_work.transaction() as ctx: - ctx.decisions.delete(decision_id) - # Remove from cache - decision = self._decisions.pop(decision_id, None) - if decision: - plan_ids = self._plan_decisions.get(decision.plan_id, []) - if decision_id in plan_ids: - plan_ids.remove(decision_id) - self.snapshots.remove(decision_id) - self._logger.info("decision.deleted", decision_id=decision_id) - return True + decision = ctx.decisions.get(decision_id) - decision = self._decisions.pop(decision_id, None) if decision is None: raise DecisionNotFoundError(decision_id) + # Delete from DB if persisted + if self._persisted and self.unit_of_work is not None: + with self.unit_of_work.transaction() as ctx: + ctx.decisions.delete(decision_id) + + # Remove from cache + self._decisions.pop(decision_id, None) plan_ids = self._plan_decisions.get(decision.plan_id, []) if decision_id in plan_ids: plan_ids.remove(decision_id) @@ -593,19 +623,25 @@ class DecisionService: def count_decisions(self, plan_id: str) -> int: """Return the number of decisions recorded for a plan. + In persisted mode uses ``SELECT COUNT(*)`` for efficiency. + Args: plan_id: ULID of the plan. Returns: Decision count. """ + if self._persisted and self.unit_of_work is not None: + with self.unit_of_work.transaction() as ctx: + return ctx.decisions.count(plan_id) return len(self.list_decisions(plan_id)) def get_next_sequence(self, plan_id: str) -> int: """Return the next available sequence number for a plan. This is useful for callers that need to know the next - sequence number before recording. + sequence number before recording. Triggers DB rehydration + on the first call for a given *plan_id*. Args: plan_id: ULID of the plan. @@ -613,6 +649,9 @@ class DecisionService: Returns: The next sequence number. """ + if plan_id not in self._sequence_initialised: + self._rehydrate_sequence(plan_id) + self._sequence_initialised.add(plan_id) return self._plan_sequence.get(plan_id, 0) # ------------------------------------------------------------------ @@ -620,11 +659,57 @@ class DecisionService: # ------------------------------------------------------------------ def _next_sequence(self, plan_id: str) -> int: - """Generate the next monotonically increasing sequence number.""" + """Generate the next monotonically increasing sequence number. + + On the first call for a given *plan_id* the counter is seeded from + the database (``MAX(sequence_number) + 1``) when a + :class:`UnitOfWork` is available. Subsequent calls for the same + plan use the in-memory counter. + + Raises: + SequenceConflictError: If the generated sequence number is + already present in the in-memory cache (indicates a + concurrency or logic bug). + """ + if plan_id not in self._sequence_initialised: + self._rehydrate_sequence(plan_id) + self._sequence_initialised.add(plan_id) + seq = self._plan_sequence.get(plan_id, 0) + + # Uniqueness guard: ensure no existing decision already holds + # this sequence number for the plan. + for did in self._plan_decisions.get(plan_id, []): + existing = self._decisions.get(did) + if existing is not None and existing.sequence_number == seq: + raise SequenceConflictError(plan_id, seq) + self._plan_sequence[plan_id] = seq + 1 return seq + def _rehydrate_sequence(self, plan_id: str) -> None: + """Seed the in-memory sequence counter from the database. + + When a :class:`UnitOfWork` is wired, queries + ``MAX(sequence_number)`` for *plan_id* so that new decisions + continue the monotonic sequence after a service restart. When + no persistence layer is available the counter starts at ``0`` + (or stays at its current value if already set). + """ + if plan_id in self._plan_sequence: + # Already has a counter — nothing to rehydrate. + return + + if self._persisted and self.unit_of_work is not None: + with self.unit_of_work.transaction() as ctx: + max_seq = ctx.decisions.get_max_sequence_number(plan_id) + if max_seq is not None: + self._plan_sequence[plan_id] = max_seq + 1 + return + + # No DB or no existing rows — start from 0. + self._plan_sequence.setdefault(plan_id, 0) + def _store_decision(self, decision: Decision) -> None: """Store a decision in both persistence and cache.""" decision_id = decision.decision_id diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index f130d744..23560105 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -173,20 +173,11 @@ class PlanLifecycleService: self._actions: dict[str, Action] = {} self._plans: dict[str, Plan] = {} - # Decision sequence counter per plan (monotonic within a plan) - self._decision_seq: dict[str, int] = {} - @property def _persisted(self) -> bool: """Return True when a UnitOfWork is wired for persistence.""" return self.unit_of_work is not None - def _next_seq(self, plan_id: str) -> int: - """Return the next decision sequence number for *plan_id*.""" - seq = self._decision_seq.get(plan_id, -1) + 1 - self._decision_seq[plan_id] = seq - return seq - def _try_record_decision( self, plan_id: str, diff --git a/src/cleveragents/cli/commands/plan.py b/src/cleveragents/cli/commands/plan.py index 6a8d8935..c8098666 100644 --- a/src/cleveragents/cli/commands/plan.py +++ b/src/cleveragents/cli/commands/plan.py @@ -2870,7 +2870,7 @@ def tree_decisions_cmd( container = get_container() svc: DecisionService = container.resolve(_DS) - decisions = svc.get_decisions_for_plan(plan_id) + decisions = svc.list_decisions(plan_id) if not decisions: console.print(f"No decisions found for plan '{plan_id}'.") return diff --git a/src/cleveragents/infrastructure/database/repositories.py b/src/cleveragents/infrastructure/database/repositories.py index 2fe1e24d..35226e32 100644 --- a/src/cleveragents/infrastructure/database/repositories.py +++ b/src/cleveragents/infrastructure/database/repositories.py @@ -65,6 +65,7 @@ if TYPE_CHECKING: from cleveragents.domain.models.core.checkpoint import Checkpoint import structlog +from sqlalchemy import func as sa_func from sqlalchemy.exc import DatabaseError as SQLAlchemyDatabaseError from sqlalchemy.exc import IntegrityError, OperationalError from sqlalchemy.orm import Session @@ -5172,6 +5173,68 @@ class DecisionRepository: f"Failed to delete decision {decision_id}: {exc}", ) from exc + # --- SEQUENCE ---------------------------------------------------------- + + @database_retry + def get_max_sequence_number(self, plan_id: str) -> int | None: + """Return the highest sequence number for *plan_id*, or ``None``. + + Uses the composite index ``ix_decisions_plan_seq`` for an + efficient ``MAX()`` aggregate. + + Args: + plan_id: ULID of the plan. + + Returns: + The maximum ``sequence_number`` stored for the plan, or + ``None`` when no decisions exist for the plan. + + Raises: + DatabaseError: On transient or unexpected DB errors. + """ + session = self._session() + try: + result: int | None = ( + session.query(sa_func.max(DecisionModel.sequence_number)) + .filter(DecisionModel.plan_id == plan_id) + .scalar() + ) + return result + except (OperationalError, SQLAlchemyDatabaseError) as exc: + raise DatabaseError( + f"Failed to get max sequence number for plan {plan_id}: {exc}", + ) from exc + + # --- COUNT ------------------------------------------------------------- + + @database_retry + def count(self, plan_id: str) -> int: + """Return the number of decisions for *plan_id*. + + Uses ``COUNT(*)`` for efficiency instead of loading all rows. + + Args: + plan_id: ULID of the plan. + + Returns: + The decision count (``0`` when no decisions exist). + + Raises: + DatabaseError: On transient or unexpected DB errors. + """ + session = self._session() + try: + result: int = ( + session.query(sa_func.count(DecisionModel.decision_id)) + .filter(DecisionModel.plan_id == plan_id) + .scalar() + ) or 0 + return result + except (OperationalError, SQLAlchemyDatabaseError) as exc: + raise DatabaseError( + f"Failed to count decisions for plan {plan_id}: {exc}", + ) from exc + # --------------------------------------------------------------------------- # Checkpoint Repository (Stage M6 - checkpointing and rollback)