fix(service): address review findings for decision service
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / build (pull_request) Successful in 14s
CI / quality (pull_request) Successful in 16s
CI / security (pull_request) Successful in 31s
CI / typecheck (pull_request) Successful in 43s
CI / unit_tests (pull_request) Successful in 2m18s
CI / integration_tests (pull_request) Successful in 2m53s
CI / docker (pull_request) Successful in 39s
CI / coverage (pull_request) Successful in 3m58s
CI / benchmark-regression (pull_request) Successful in 24m44s

- Rehydrate sequence counter from DB on restart (BUG-1)
- Add uniqueness guard raising SequenceConflictError (BUG-2)
- Fix delete_decision consistency between persisted/in-memory (BUG-3)
- Validate new_decision_id exists in mark_superseded (BUG-4)
- Include orphaned subtrees in get_tree output (BUG-5)
- Remove dead _decision_seq/_next_seq from plan_lifecycle (BUG-7)
- Export SequenceConflictError from services __init__ (SPEC-2)
- Replace list[Any] with list[ArtifactRef] typing (SPEC-4)
- Add actor_reasoning max_length validation (SEC-1)
- Use SELECT COUNT(*) in count_decisions (PERF-1)
- Replace MagicMock with create_autospec(Settings) (TEST-3)
- Eliminate UnitOfWork.__new__() anti-pattern (TEST-4)
- Use exact assertion counts in DI tests (TEST-5)
- Add restart rehydration, BFS order, invalid type, and
  confidence boundary test scenarios (TEST-1/2/6/7)
- Fix decision_service_coverage.feature to match actual API

ISSUES CLOSED: #172
This commit is contained in:
khyari hamza
2026-03-03 03:17:19 +00:00
parent 67c63f4c58
commit 0e36755db9
14 changed files with 482 additions and 215 deletions
+11 -21
View File
@@ -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(
+32 -1
View File
@@ -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
+22 -54
View File
@@ -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"
+25 -27
View File
@@ -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)}"
)
+97 -1
View File
@@ -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."""
@@ -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(
@@ -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)
+7 -21
View File
@@ -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)
+31 -2
View File
@@ -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")
@@ -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",
@@ -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
@@ -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,
+1 -1
View File
@@ -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
@@ -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)