From 3e13411fcf63b1bd3f76c3268797588b9e48e747 Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Thu, 21 May 2026 19:33:49 +0000 Subject: [PATCH 1/5] fix(actors): distinguish namespace/name from provider/model in actor name parsing When action YAML references actors using namespace/name format (e.g. `strategy_actor: local/my-strategist`), `_parse_actor_name()` incorrectly treated the namespace prefix as a provider name, causing `ValueError: Unknown provider type: local`. Changes: - `_is_known_provider()`: New utility function (strategy_resolution.py) that checks whether a slash-separated first segment matches a known `ProviderType` value (openai, anthropic, etc.). Consolidated into a single implementation; llm_actors.py now imports from strategy_resolution.py. - `_parse_actor_name()`: When the first segment IS a known provider, preserves existing behaviour (provider/model). When it is NOT a known provider, treats the input as namespace/name and returns the full name as the model identifier with the default provider. Callers SHOULD pre-resolve namespace/name references via the actor registry before calling this function. Both implementations (strategy_resolution.py and llm_actors.py) updated; ProviderType import moved to top level. - `PlanLifecycleService.resolve_actor_provider_model()`: New method that resolves a namespaced actor name (e.g. local/my-strategist) to provider/model format by looking up the actor record and extracting its provider and model fields. - `LifecycleService` Protocol (strategy_resolution.py): Added `resolve_actor_provider_model` to the protocol, eliminating the need for `# type: ignore[union-attr]` at call sites. - `PlanLifecycleProtocol` (llm_actors.py): Added `resolve_actor_provider_model` to the protocol. - Caller pre-resolution: StrategyActor, LLMStrategizeActor, LLMExecuteActor, SessionWorkflow._resolve_llm(), and CLI session commands now pre-resolve actor names through the lifecycle service or via injected actor_resolver callables before calling `_parse_actor_name()`, ensuring namespace/name references are correctly mapped to their underlying LLM providers. - `_build_actor_resolver()` (cli/commands/session.py) and `_build_actor_resolver_for_session_workflow()` (a2a/facade.py): Moved `_is_known_provider` imports to top level; removed redundant `except (NotFoundError, Exception)`; added warning logging for outer exception handlers. - `make_mock_lifecycle()` (mock_strategy_llm.py) and `_make_mock_lifecycle()` (llm_actors_coverage_steps.py): Added `resolve_actor_provider_model` to mock lifecycle services for test compatibility. - Added `@tdd_issue @tdd_issue_11254` tags to all new BDD scenarios related to namespace/name disambiguation in llm_actors_coverage, strategy_actor_llm, and plan_lifecycle_service_coverage_boost_r4 feature files. - Updated docs/CHANGELOG.md with the fix entry. ISSUES CLOSED: #11254 --- CHANGELOG.md | 12 ++ docs/CHANGELOG.md | 12 ++ features/llm_actors_coverage.feature | 18 +++ features/mocks/mock_strategy_llm.py | 7 +- ...ifecycle_service_coverage_boost_r4.feature | 43 +++++- features/steps/llm_actors_coverage_steps.py | 1 + ...fecycle_service_coverage_boost_r4_steps.py | 144 ++++++++++++++++++ features/steps/strategy_actor_llm_steps.py | 25 +++ features/strategy_actor_llm.feature | 36 ++++- robot/helper_m5_e2e_context.py | 1 + src/cleveragents/a2a/facade.py | 28 ++++ .../application/services/llm_actors.py | 38 ++++- .../services/plan_lifecycle_service.py | 32 ++++ .../application/services/session_workflow.py | 24 ++- .../application/services/strategy_actor.py | 5 + .../services/strategy_resolution.py | 81 +++++++++- src/cleveragents/cli/commands/session.py | 39 +++++ 17 files changed, 524 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61543b7c5..a77559e73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,18 @@ Changed `wf10_batch.robot` to be less likely to create files, and validation to `_execute_child_plan` to prevent re-entrant or orphaned child plan execution. +- **Actor namespace/name disambiguation** (#11254): When action YAML references + actors using `namespace/name` format (e.g. `strategy_actor: local/my-strategist`), + the `_parse_actor_name()` functions no longer mistake the namespace prefix for an + LLM provider name. A new `_is_known_provider()` utility checks whether the first + slash-separated segment matches a known `ProviderType` (e.g. `openai`, `anthropic`); + if not, the input is treated as namespace/name. `PlanLifecycleService` now provides + `resolve_actor_provider_model()` to resolve namespaced references to their underlying + `provider/model` via the actor registry. All affected call sites — `StrategyActor`, + `LLMStrategizeActor`, `LLMExecuteActor`, and `SessionWorkflow._resolve_llm()` — + pre-resolve actor names before passing them to the LLM provider, fixing the + `ValueError: Unknown provider type` crash when using namespaced actor references. + Data integrity fix: ValidationAttachmentRepository argument swap (#7492): Fixed a critical data integrity issue in `ValidationAttachmentRepository.attach` where `validation_name` and `resource_id` arguments were being silently swapped based on a diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 88cd53d5e..06c71c580 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -59,6 +59,18 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **Actor namespace/name disambiguation** (#11254): When action YAML references + actors using `namespace/name` format (e.g. `strategy_actor: local/my-strategist`), + the `_parse_actor_name()` functions no longer mistake the namespace prefix for an + LLM provider name. A new `_is_known_provider()` utility checks whether the first + slash-separated segment matches a known `ProviderType` (e.g. `openai`, `anthropic`); + if not, the input is treated as namespace/name. `PlanLifecycleService` now provides + `resolve_actor_provider_model()` to resolve namespaced references to their underlying + `provider/model` via the actor registry. All affected call sites — `StrategyActor`, + `LLMStrategizeActor`, `LLMExecuteActor`, and `SessionWorkflow._resolve_llm()` — + pre-resolve actor names before passing them to the LLM provider, fixing the + `ValueError: Unknown provider type` crash when using namespaced actor references. + - **Built-in actors v3 YAML format** (#10883): Fixed `agents actor run` failing for built-in actors (e.g., `openai/gpt-4`, `anthropic/claude-3-opus`) due to missing v3 `type` field in stored configuration. `ActorRegistry.ensure_built_in_actors()` diff --git a/features/llm_actors_coverage.feature b/features/llm_actors_coverage.feature index 03bc9e8e5..673b120d6 100644 --- a/features/llm_actors_coverage.feature +++ b/features/llm_actors_coverage.feature @@ -26,6 +26,24 @@ Feature: LLM Actors Coverage Then the parsed provider should be "anthropic" And the parsed model should be "claude-3" + @tdd_issue @tdd_issue_11254 + Scenario: Parse actor name with namespace/name format + When I parse actor name "local/my-strategist" + Then the parsed provider should be "openai" + And the parsed model should be "local/my-strategist" + + @tdd_issue @tdd_issue_11254 + Scenario: Parse actor name with org namespace/name format + When I parse actor name "cleverthis/my-executor" + Then the parsed provider should be "openai" + And the parsed model should be "cleverthis/my-executor" + + @tdd_issue @tdd_issue_11254 + Scenario: Parse actor name with unknown-provider-like segment + When I parse actor name "unknown/gpt-4" + Then the parsed provider should be "openai" + And the parsed model should be "unknown/gpt-4" + # --------------------------------------------------------------- # LLMStrategizeActor.__init__ validation # --------------------------------------------------------------- diff --git a/features/mocks/mock_strategy_llm.py b/features/mocks/mock_strategy_llm.py index 18657035f..e3a91b1fc 100644 --- a/features/mocks/mock_strategy_llm.py +++ b/features/mocks/mock_strategy_llm.py @@ -309,13 +309,18 @@ def make_mock_registry(response_content: str) -> SimpleNamespace: def make_mock_lifecycle( strategy_actor: str | None = "openai/gpt-4", + execution_actor: str | None = None, ) -> SimpleNamespace: """Create a mock lifecycle service.""" plan = SimpleNamespace(action_name="test-action") - action = SimpleNamespace(strategy_actor=strategy_actor) + action = SimpleNamespace( + strategy_actor=strategy_actor, + execution_actor=execution_actor or strategy_actor, + ) return SimpleNamespace( get_plan=MagicMock(return_value=plan), get_action=MagicMock(return_value=action), + resolve_actor_provider_model=MagicMock(return_value=None), ) diff --git a/features/plan_lifecycle_service_coverage_boost_r4.feature b/features/plan_lifecycle_service_coverage_boost_r4.feature index b58edf637..9bd7e0eab 100644 --- a/features/plan_lifecycle_service_coverage_boost_r4.feature +++ b/features/plan_lifecycle_service_coverage_boost_r4.feature @@ -160,4 +160,45 @@ Scenario: _handle_correction_applied skips event with no plan_id Given the plan lifecycle service is configured for r4 And get_plan will fail for the plan for r4 When I handle a CORRECTION_APPLIED event when get_plan fails for r4 - Then no exception should be raised for r4 \ No newline at end of file + Then no exception should be raised for r4 + + # --------------------------------------------------------------- + # resolve_actor_provider_model — namespace/name → provider/model + # Lines 731-764 (factor resolution for #11254) + # --------------------------------------------------------------- + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model returns provider/model for known provider + Given the plan lifecycle service has a unit of work for actor resolution r4 + When I resolve actor provider model for "openai/gpt-4" + Then the resolved actor provider model should be "openai/gpt-4" + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model resolves namespace/name via actor lookup + Given the plan lifecycle service has a unit of work with a known actor "local/strategist" using "openai/gpt-4" for r4 + When I resolve actor provider model for "local/strategist" + Then the resolved actor provider model should be "openai/gpt-4" + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model returns None when actor is not found + Given the plan lifecycle service has a unit of work without actor "local/missing-actor" for r4 + When I resolve actor provider model for "local/missing-actor" + Then the resolved actor provider model should be None + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model catches exception from actor lookup + Given the plan lifecycle service has a unit of work that raises on actor lookup for r4 + When I resolve actor provider model for "local/crash-actor" + Then the resolved actor provider model should be None + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model returns None when unit_of_work is absent + Given the plan lifecycle service has no unit of work for r4 + When I resolve actor provider model for "local/strategist" + Then the resolved actor provider model should be None + + @tdd_issue @tdd_issue_11254 + Scenario: resolve_actor_provider_model returns None for empty actor name + Given the plan lifecycle service has a unit of work for actor resolution r4 + When I resolve actor provider model for "" + Then the resolved actor provider model should be None \ No newline at end of file diff --git a/features/steps/llm_actors_coverage_steps.py b/features/steps/llm_actors_coverage_steps.py index dfef3938e..8142fdd45 100644 --- a/features/steps/llm_actors_coverage_steps.py +++ b/features/steps/llm_actors_coverage_steps.py @@ -92,6 +92,7 @@ def _make_mock_lifecycle(strategy_actor=None, execution_actor=None): lifecycle = SimpleNamespace( get_plan=MagicMock(return_value=plan), get_action=MagicMock(return_value=action), + resolve_actor_provider_model=MagicMock(return_value=None), ) return lifecycle diff --git a/features/steps/plan_lifecycle_service_coverage_boost_r4_steps.py b/features/steps/plan_lifecycle_service_coverage_boost_r4_steps.py index 2f487b296..61547a9f9 100644 --- a/features/steps/plan_lifecycle_service_coverage_boost_r4_steps.py +++ b/features/steps/plan_lifecycle_service_coverage_boost_r4_steps.py @@ -884,3 +884,147 @@ def step_handle_correction_get_plan_fails_r4(context: Context) -> None: context.service._handle_correction_applied(event) except Exception as e: context.error = e + + +# ================================================================= +# resolve_actor_provider_model — namespace/name → provider/model +# Lines 731-764 +# ================================================================= + + +@given("the plan lifecycle service has a unit of work for actor resolution r4") +def step_service_with_uow_for_actor_r4(context: Context) -> None: + """Create a service with a basic unit of work for actor resolution.""" + from unittest.mock import MagicMock + from contextlib import contextmanager + + Settings._instance = None + settings = Settings() + mock_uow = MagicMock() + mock_ctx = MagicMock() + mock_ctx.actors.get_by_name.return_value = None + + @contextmanager + def _fake_transaction(): + yield mock_ctx + + mock_uow.transaction = _fake_transaction + context.service = PlanLifecycleService(settings=settings, unit_of_work=mock_uow) + context.error = None + + +@given( + "the plan lifecycle service has a unit of work with a known actor " + '"{actor_name}" using "{provider_model}" for r4' +) +def step_service_with_known_actor_r4( + context: Context, actor_name: str, provider_model: str +) -> None: + """Create a service where actor lookup succeeds for the given name.""" + from unittest.mock import MagicMock + from contextlib import contextmanager + + provider, model = provider_model.split("/", 1) + mock_actor = MagicMock() + mock_actor.provider = provider + mock_actor.model = model + + Settings._instance = None + settings = Settings() + mock_uow = MagicMock() + mock_ctx = MagicMock() + mock_ctx.actors.get_by_name.return_value = mock_actor + + @contextmanager + def _fake_transaction(): + yield mock_ctx + + mock_uow.transaction = _fake_transaction + context.service = PlanLifecycleService(settings=settings, unit_of_work=mock_uow) + context.error = None + + +@given( + 'the plan lifecycle service has a unit of work without actor "{actor_name}" for r4' +) +def step_service_without_actor_r4(context: Context, actor_name: str) -> None: + """Create a service where actor lookup returns None.""" + from unittest.mock import MagicMock + from contextlib import contextmanager + + Settings._instance = None + settings = Settings() + mock_uow = MagicMock() + mock_ctx = MagicMock() + mock_ctx.actors.get_by_name.return_value = None + + @contextmanager + def _fake_transaction(): + yield mock_ctx + + mock_uow.transaction = _fake_transaction + context.service = PlanLifecycleService(settings=settings, unit_of_work=mock_uow) + context.error = None + + +@given( + "the plan lifecycle service has a unit of work that raises on actor lookup for r4" +) +def step_service_with_raising_actor_lookup_r4(context: Context) -> None: + """Create a service where actor lookup raises an exception.""" + from unittest.mock import MagicMock + from contextlib import contextmanager + + Settings._instance = None + settings = Settings() + mock_uow = MagicMock() + mock_ctx = MagicMock() + mock_ctx.actors.get_by_name.side_effect = RuntimeError("DB failure") + + @contextmanager + def _fake_transaction(): + yield mock_ctx + + mock_uow.transaction = _fake_transaction + context.service = PlanLifecycleService(settings=settings, unit_of_work=mock_uow) + context.error = None + + +@given("the plan lifecycle service has no unit of work for r4") +def step_service_no_uow_r4(context: Context) -> None: + """Create a service with no unit_of_work set.""" + Settings._instance = None + settings = Settings() + context.service = PlanLifecycleService( + settings=settings, + unit_of_work=None, + ) + context.error = None + + +@when('I resolve actor provider model for "{actor_name}"') +def step_resolve_actor_provider_model(context: Context, actor_name: str) -> None: + """Call resolve_actor_provider_model directly.""" + context.resolved_model = context.service.resolve_actor_provider_model(actor_name) + + +@when('I resolve actor provider model for ""') +def step_resolve_actor_provider_model_empty(context: Context) -> None: + """Call resolve_actor_provider_model with empty string.""" + context.resolved_model = context.service.resolve_actor_provider_model("") + + +@then('the resolved actor provider model should be "{expected}"') +def step_verify_resolved_provider_model(context: Context, expected: str) -> None: + """Verify the resolved provider/model string matches.""" + assert context.resolved_model == expected, ( + f"Expected '{expected}', got '{context.resolved_model}'" + ) + + +@then("the resolved actor provider model should be None") +def step_verify_resolved_provider_model_none(context: Context) -> None: + """Verify the resolved provider/model is None.""" + assert context.resolved_model is None, ( + f"Expected None, got '{context.resolved_model}'" + ) diff --git a/features/steps/strategy_actor_llm_steps.py b/features/steps/strategy_actor_llm_steps.py index 4b70fe350..ea127ea68 100644 --- a/features/steps/strategy_actor_llm_steps.py +++ b/features/steps/strategy_actor_llm_steps.py @@ -41,6 +41,7 @@ from cleveragents.application.services.strategy_actor import ( resolve_strategy_actor, validate_no_cycles, ) +from cleveragents.application.services.strategy_resolution import _is_known_provider from cleveragents.core.exceptions import PlanError, ValidationError from cleveragents.domain.models.core.decision import DecisionType from cleveragents.domain.models.core.plan import InvariantSource, PlanInvariant @@ -583,6 +584,30 @@ def step_verify_sa_parsed_model(context, expected): ) +# --------------------------------------------------------------------------- +# _is_known_provider helper +# --------------------------------------------------------------------------- + + +@when('I check if "{provider}" is a known strategy provider') +def step_check_known_strategy_provider(context, provider): + context.sa_is_known = _is_known_provider(provider) + + +@then("the strategy provider check should be true") +def step_verify_provider_check_true(context): + assert context.sa_is_known is True, ( + f"Expected provider to be known, got {context.sa_is_known}" + ) + + +@then("the strategy provider check should be false") +def step_verify_provider_check_false(context): + assert context.sa_is_known is False, ( + f"Expected provider to NOT be known, got {context.sa_is_known}" + ) + + # --------------------------------------------------------------------------- # Cyclic dependency detection through LLM execute path (H8) # --------------------------------------------------------------------------- diff --git a/features/strategy_actor_llm.feature b/features/strategy_actor_llm.feature index a36c2b9f6..ab45fb2b9 100644 --- a/features/strategy_actor_llm.feature +++ b/features/strategy_actor_llm.feature @@ -218,6 +218,34 @@ Feature: LLM-powered Strategy Actor Then the strategy parsed provider should be "openai" And the strategy parsed model should be "gpt-4" + @tdd_issue @tdd_issue_11254 + Scenario: Parse strategy actor name with namespace/name format + When I parse strategy actor name "local/my-strategist" + Then the strategy parsed provider should be "openai" + And the strategy parsed model should be "local/my-strategist" + + @tdd_issue @tdd_issue_11254 + Scenario: Parse strategy actor name with org namespace/name format + When I parse strategy actor name "cleverthis/my-executor" + Then the strategy parsed provider should be "openai" + And the strategy parsed model should be "cleverthis/my-executor" + + @tdd_issue @tdd_issue_11254 + Scenario: Parse strategy actor name with unknown-provider-like segment + When I parse strategy actor name "unknown/gpt-4" + Then the strategy parsed provider should be "openai" + And the strategy parsed model should be "unknown/gpt-4" + + @tdd_issue @tdd_issue_11254 + Scenario: _is_known_provider recognises valid providers + When I check if "openai" is a known strategy provider + Then the strategy provider check should be true + + @tdd_issue @tdd_issue_11254 + Scenario: _is_known_provider rejects namespace prefix + When I check if "local" is a known strategy provider + Then the strategy provider check should be false + # --------------------------------------------------------------- # Cyclic dependency detection through LLM execute path (H8) # --------------------------------------------------------------- @@ -326,8 +354,8 @@ Feature: LLM-powered Strategy Actor Scenario: Parse strategy actor name with multiple slashes When I parse strategy actor name "provider/model/version" - Then the strategy parsed provider should be "provider" - And the strategy parsed model should be "model/version" + Then the strategy parsed provider should be "openai" + And the strategy parsed model should be "provider/model/version" # --------------------------------------------------------------- # Actor name with empty segments (M2 fix) @@ -345,8 +373,8 @@ Feature: LLM-powered Strategy Actor Scenario: Parse strategy actor name with empty model uses default model When I parse strategy actor name "provider/" - Then the strategy parsed provider should be "provider" - And the strategy parsed model should be "gpt-4" + Then the strategy parsed provider should be "openai" + And the strategy parsed model should be "provider/" # --------------------------------------------------------------- # Whitespace-only definition_of_done (L5 fix) diff --git a/robot/helper_m5_e2e_context.py b/robot/helper_m5_e2e_context.py index 8be565464..2e29b2079 100644 --- a/robot/helper_m5_e2e_context.py +++ b/robot/helper_m5_e2e_context.py @@ -475,6 +475,7 @@ def llm_execute_acms_context() -> None: lifecycle = SimpleNamespace( get_plan=MagicMock(return_value=plan), get_action=MagicMock(return_value=action), + resolve_actor_provider_model=MagicMock(return_value=None), ) actor = LLMExecuteActor( diff --git a/src/cleveragents/a2a/facade.py b/src/cleveragents/a2a/facade.py index 43be9f4c2..268a543b9 100644 --- a/src/cleveragents/a2a/facade.py +++ b/src/cleveragents/a2a/facade.py @@ -43,6 +43,9 @@ from cleveragents.application.services.resource_registry_service import ( ResourceRegistryService, ) from cleveragents.application.services.session_workflow import SessionWorkflow +from cleveragents.application.services.strategy_resolution import ( + build_actor_resolver, +) from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core.session import ( SessionActorNotConfiguredError, @@ -380,11 +383,36 @@ class A2aLocalFacade: if svc is None: raise RuntimeError("Session service not available — create a session first") registry = self._provider_registry + actor_resolver = self._build_actor_resolver_for_session_workflow() return SessionWorkflow( session_service=svc, provider_registry=registry, + actor_resolver=actor_resolver, ) + @staticmethod + def _build_actor_resolver_for_session_workflow(): + """Build a namespace/name -> provider/model resolver for SessionWorkflow. + + Returns a callable ``(actor_name: str) -> str | None``, or + ``None`` when the DI container or actor service is unavailable. + """ + try: + from cleveragents.application.container import get_container + + container = get_container() + actor_service = container.actor_service() + if actor_service is None: + return None + + return build_actor_resolver(actor_service) + except Exception: + logger.warning( + "actor_resolver_unavailable", + exc_info=True, + ) + return None + def _handle_message_send(self, params: dict[str, Any]) -> dict[str, Any]: """Handle A2A ``message/send`` — invoke orchestrator actor (non-streaming). diff --git a/src/cleveragents/application/services/llm_actors.py b/src/cleveragents/application/services/llm_actors.py index 86bf89841..07f114e87 100644 --- a/src/cleveragents/application/services/llm_actors.py +++ b/src/cleveragents/application/services/llm_actors.py @@ -26,6 +26,7 @@ from cleveragents.application.services.plan_executor import ( StrategyDecision, StreamCallback, ) +from cleveragents.application.services.strategy_resolution import _is_known_provider from cleveragents.config.settings import Settings, get_settings from cleveragents.core.exceptions import ValidationError from cleveragents.domain.models.acms.crp import AssembledContext @@ -65,6 +66,10 @@ class PlanLifecycleProtocol(Protocol): """Commit the plan by persisting its state and artifacts.""" ... + def resolve_actor_provider_model(self, actor_name: str) -> str | None: + """Resolve a namespaced actor name to ``provider/model`` format.""" + ... + # --------------------------------------------------------------------------- # Internal data structures @@ -91,18 +96,31 @@ _LOG_RESPONSE_CHARS = 500 def _parse_actor_name(actor_name: str) -> tuple[str, str]: - """Split ``provider/model`` into *(provider_type, model_id)*. + """Split *actor_name* into *(provider_type, model_id)*. + + When the first slash-separated segment is a known + :class:`ProviderType` value the input is treated as + ``provider/model``. Otherwise it is treated as a namespace/name + reference and the entire string is returned as the model with the + default provider. Callers SHOULD pre-resolve namespace/name + references via the actor registry before calling this function. Examples: - ``openai/gpt-4`` → ``("openai", "gpt-4")`` + ``openai/gpt-4`` → ``("openai", "gpt-4")`` ``anthropic/claude-3`` → ``("anthropic", "claude-3")`` - ``gpt-4`` → ``("openai", "gpt-4")`` (default provider) + ``gpt-4`` → ``("openai", "gpt-4")`` + ``local/my-actor`` → ``("openai", "local/my-actor")`` """ if not actor_name: return ("openai", "gpt-4") parts = actor_name.split("/", 1) if len(parts) == 2: - return (parts[0], parts[1]) + provider, model = parts[0], parts[1] + if _is_known_provider(provider): + return (provider, model) + if not provider: + return ("openai", model) + return ("openai", actor_name) return ("openai", actor_name) @@ -148,6 +166,12 @@ class LLMStrategizeActor: plan = self._lifecycle.get_plan(plan_id) action = self._lifecycle.get_action(plan.action_name) actor_name = action.strategy_actor or "openai/gpt-4" + + # Pre-resolve namespace/name to provider/model via actor registry + resolved = self._lifecycle.resolve_actor_provider_model(actor_name) + if resolved: + actor_name = resolved + provider_type, model_id = _parse_actor_name(actor_name) self._logger.info( @@ -377,6 +401,12 @@ class LLMExecuteActor: plan = self._lifecycle.get_plan(plan_id) action = self._lifecycle.get_action(plan.action_name) actor_name = action.execution_actor or "openai/gpt-4" + + # Pre-resolve namespace/name to provider/model via actor registry + resolved = self._lifecycle.resolve_actor_provider_model(actor_name) + if resolved: + actor_name = resolved + provider_type, model_id = _parse_actor_name(actor_name) self._logger.info( diff --git a/src/cleveragents/application/services/plan_lifecycle_service.py b/src/cleveragents/application/services/plan_lifecycle_service.py index 6445a398c..420ae4fb3 100644 --- a/src/cleveragents/application/services/plan_lifecycle_service.py +++ b/src/cleveragents/application/services/plan_lifecycle_service.py @@ -65,6 +65,7 @@ from cleveragents.application.services.config_service import ConfigLevel, Config from cleveragents.application.services.plan_preflight_guardrail import ( PlanPreflightGuardrail, ) +from cleveragents.application.services.strategy_resolution import _is_known_provider from cleveragents.core.exceptions import ( BusinessRuleViolation, DatabaseError, @@ -728,6 +729,37 @@ class PlanLifecycleService: """Generate a new ULID string.""" return str(ULID()) + def resolve_actor_provider_model(self, actor_name: str) -> str | None: + """Resolve a namespaced actor name to ``provider/model`` format. + + When *actor_name* is a known ``provider/model`` pair + (e.g. ``openai/gpt-4``) it is returned unchanged. When it is a + namespace/name (e.g. ``local/my-strategist``) the function looks + up the actor record and extracts its provider and model fields, + returning ``"/"`` or ``None`` if the actor + cannot be found. + """ + if not actor_name or actor_name.startswith("__"): + return None + parts = actor_name.split("/", 1) + if len(parts) == 2 and _is_known_provider(parts[0].strip()): + return actor_name + if self.unit_of_work is None: + return None + try: + with self.unit_of_work.transaction() as ctx: + actor: Actor | None = ctx.actors.get_by_name(actor_name) + except Exception: + self._logger.warning( + "actor_provider_resolution_failed", + actor_name=actor_name, + exc_info=True, + ) + return None + if actor is None: + return None + return f"{actor.provider}/{actor.model}" + def _resolve_actor_registry_entry(self, actor_name: str) -> object | None: """Resolve a namespaced actor name to its stored configuration payload.""" if not actor_name or actor_name.startswith("__"): diff --git a/src/cleveragents/application/services/session_workflow.py b/src/cleveragents/application/services/session_workflow.py index 409122809..6791bfc15 100644 --- a/src/cleveragents/application/services/session_workflow.py +++ b/src/cleveragents/application/services/session_workflow.py @@ -38,6 +38,7 @@ from cleveragents.application.services.session_caller import ( extract_content, history_to_langchain_messages, ) +from cleveragents.application.services.strategy_resolution import _parse_actor_name from cleveragents.domain.models.core.session import ( MessageRole, SessionActorNotConfiguredError, @@ -138,12 +139,14 @@ class SessionWorkflow: provider_registry: ProviderRegistry | None = None, tool_registry: ToolRegistry | None = None, llm_factory: Callable[[str], Any] | None = None, + actor_resolver: Callable[[str], str | None] | None = None, max_iterations: int = 25, ) -> None: self._session_service = session_service self._provider_registry = provider_registry self._tool_registry = tool_registry or ToolRegistry() self._llm_factory = llm_factory + self._actor_resolver = actor_resolver self._max_iterations = max_iterations self._logger = logger.bind(component="session_workflow") # Populated by tell_stream() so callers can read real usage metrics @@ -367,6 +370,10 @@ class SessionWorkflow: Delegates to ``llm_factory`` (test injection), ``ProviderRegistry``, or falls back to a stub when neither is available. + + Pre-resolves namespace/name actor references (e.g. ``local/my-actor``) + via ``actor_resolver`` before passing the name to the provider + registry (addresses #11254). """ # Test injection point — skips provider registry when llm_factory is set. # Must be checked before _provider_registry fallback so tests can inject @@ -380,11 +387,20 @@ class SessionWorkflow: if self._provider_registry is None: return self._make_stub_llm() - from cleveragents.application.services.strategy_resolution import ( - _parse_actor_name, - ) + # Pre-resolve namespace/name references (e.g. local/my-strategist) + # to provider/model format via the injected actor_resolver. + # When no resolver is available or resolution fails, fall through + # to _parse_actor_name which treats the input as namespace/name + # with the default provider (avoiding a ValueError crash). + resolved: str | None = None + if self._actor_resolver is not None: + try: + resolved = self._actor_resolver(actor_name) + except Exception: + resolved = None + effective_name = resolved if resolved is not None else actor_name - provider_type, model_id = _parse_actor_name(actor_name) + provider_type, model_id = _parse_actor_name(effective_name) return self._provider_registry.create_llm( provider_type=provider_type, model_id=model_id, diff --git a/src/cleveragents/application/services/strategy_actor.py b/src/cleveragents/application/services/strategy_actor.py index 8aa10059f..71284a6af 100644 --- a/src/cleveragents/application/services/strategy_actor.py +++ b/src/cleveragents/application/services/strategy_actor.py @@ -457,6 +457,11 @@ class StrategyActor: plan = self._lifecycle.get_plan(plan_id) action = self._lifecycle.get_action(plan.action_name) actor_name = action.strategy_actor or _DEFAULT_ACTOR_NAME + + # Pre-resolve namespace/name to provider/model via actor registry + resolved = self._lifecycle.resolve_actor_provider_model(actor_name) + if resolved: + actor_name = resolved except (KeyError, ValueError, RuntimeError): self._logger.debug( "Could not resolve actor from plan, using default", diff --git a/src/cleveragents/application/services/strategy_resolution.py b/src/cleveragents/application/services/strategy_resolution.py index dd7eabfdd..4790d6631 100644 --- a/src/cleveragents/application/services/strategy_resolution.py +++ b/src/cleveragents/application/services/strategy_resolution.py @@ -7,6 +7,7 @@ graphs, and actor name parsing. from __future__ import annotations from collections import deque +from collections.abc import Callable from typing import TYPE_CHECKING, Any, Protocol, runtime_checkable import structlog @@ -15,6 +16,7 @@ from cleveragents.application.services.context_tiers import ( ContextTierService, ) from cleveragents.core.exceptions import PlanError +from cleveragents.providers.registry import ProviderType if TYPE_CHECKING: from cleveragents.providers.registry import ProviderRegistry @@ -26,7 +28,7 @@ _DEFAULT_ACTOR_NAME = "openai/gpt-4" @runtime_checkable class LifecycleService(Protocol): - """Minimal interface for plan/action resolution.""" + """Minimal interface for plan/action/actor resolution.""" def get_plan(self, plan_id: str) -> Any: """Get a plan by ID.""" @@ -36,6 +38,10 @@ class LifecycleService(Protocol): """Get an action by name.""" ... + def resolve_actor_provider_model(self, actor_name: str) -> str | None: + """Resolve a namespaced actor name to ``provider/model`` format.""" + ... + @runtime_checkable class AcmsPipeline(Protocol): @@ -99,15 +105,37 @@ def validate_no_cycles(edges: list[tuple[str, str]]) -> bool: return True +def _is_known_provider(provider: str) -> bool: + """Check whether *provider* is a known ``ProviderType`` value. + + Returns ``True`` when the string matches a registered provider + (e.g. ``"openai"``, ``"anthropic"``). Returns ``False`` when the + string is something else -- typically a namespace prefix like + ``"local"`` or ``"cleverthis"``. + """ + return provider in ProviderType.__members__.values() + + def _parse_actor_name(actor_name: str) -> tuple[str, str]: - """Split ``provider/model`` into *(provider_type, model_id)*. + """Split *actor_name* into *(provider_type, model_id)*. + + When the first slash-separated segment is a known + :class:`~cleveragents.providers.registry.ProviderType` value the + input is treated as ``provider/model``. Otherwise it is treated as + a namespace/name reference (e.g. ``"local/my-strategist"``) and the + entire string is returned as the model with the default provider. + + Callers SHOULD pre-resolve namespace/name references via the actor + registry before calling this function. Falls back to the default derived from :data:`_DEFAULT_ACTOR_NAME` when the input is empty or contains no ``/`` separator. Examples: - ``openai/gpt-4`` -> ``("openai", "gpt-4")`` - ``gpt-4`` -> ``("openai", "gpt-4")`` (default provider) + ``"openai/gpt-4"`` → ``("openai", "gpt-4")`` + ``"anthropic/claude-3"`` → ``("anthropic", "claude-3")`` + ``"gpt-4"`` → ``("openai", "gpt-4")`` + ``"local/my-strategist"`` → ``("openai", "local/my-strategist")`` """ _default_provider, _default_model = _DEFAULT_ACTOR_NAME.split("/", 1) if not actor_name or not actor_name.strip(): @@ -120,19 +148,56 @@ def _parse_actor_name(actor_name: str) -> tuple[str, str]: if len(parts) == 2: provider, model = parts[0].strip(), parts[1].strip() if not provider and not model: - # Both segments empty (e.g. "/") — full default. logger.warning( "Actor name '%s' has empty provider and model, defaulting to %s", actor_name, _DEFAULT_ACTOR_NAME, ) return (_default_provider, _default_model) - # Preserve whichever segment the caller provided, filling - # in the default for the missing half. - return (provider or _default_provider, model or _default_model) + if _is_known_provider(provider): + return (provider or _default_provider, model or _default_model) + if not provider: + return (_default_provider, model or _default_model) + logger.debug( + "Actor name '%s' first segment is not a known provider; " + "treating as namespace/name.", + actor_name, + ) + return (_default_provider, actor_name) return (_default_provider, actor_name) +def build_actor_resolver( + actor_service: Any, +) -> Callable[[str], str | None]: + """Build a namespace/name → provider/model resolver closure. + + Returns a callable ``(actor_name: str) -> str | None`` that: + - returns ``None`` when the name is already in ``provider/model`` format + (checked via :func:`_is_known_provider`) + - looks up the actor via *actor_service* and returns + ``"/"`` on success + - returns ``None`` when the actor is unknown or lookup fails + + Used by both CLI session commands and the A2A facade to provide + ``SessionWorkflow`` with namespace/name resolution capability. + """ + + def resolve(actor_name: str) -> str | None: + if not actor_name: + return None + parts = actor_name.split("/", 1) + if len(parts) == 2 and _is_known_provider(parts[0].strip()): + return None # already in provider/model format + try: + actor = actor_service.get_actor(actor_name) + except Exception: + return None + return f"{actor.provider}/{actor.model}" + + return resolve + + def resolve_strategy_actor( provider_registry: ProviderRegistry | None = None, lifecycle_service: LifecycleService | None = None, diff --git a/src/cleveragents/cli/commands/session.py b/src/cleveragents/cli/commands/session.py index 77cd4f9f8..c4f80c198 100644 --- a/src/cleveragents/cli/commands/session.py +++ b/src/cleveragents/cli/commands/session.py @@ -29,6 +29,9 @@ from rich.table import Table from cleveragents.a2a.models import A2aRequest from cleveragents.application.services.session_workflow import SessionWorkflow +from cleveragents.application.services.strategy_resolution import ( + build_actor_resolver, +) from cleveragents.cli.formatting import OutputFormat, format_output from cleveragents.core.exceptions import DatabaseError from cleveragents.domain.models.core.session import ( @@ -100,9 +103,11 @@ def _build_session_workflow() -> SessionWorkflow: """ service = _get_session_service() provider_registry = _get_provider_registry() + actor_resolver = _build_actor_resolver() return SessionWorkflow( session_service=service, provider_registry=provider_registry, + actor_resolver=actor_resolver, ) @@ -121,6 +126,40 @@ def _get_provider_registry() -> ProviderRegistry | None: return None +def _build_actor_resolver(): + """Build a resolver callable for namespace/name -> provider/model resolution. + + Returns a callable ``(actor_name: str) -> str | None`` that looks up + a namespace/name actor reference (e.g. ``"local/my-strategist"``) in + the actor registry and returns the ``"provider/model"`` string, or + ``None`` when the name is already in provider/model format, the actor + is unknown, or the registry is unavailable. + + When no DI container is configured the function returns a resolver + that always returns ``None`` (graceful degradation). + """ + try: + from cleveragents.application.container import get_container + + container = get_container() + actor_service = container.actor_service() + if actor_service is None: + return _null_actor_resolver + + return build_actor_resolver(actor_service) + except Exception: + _log.warning( + "actor_resolver_unavailable", + exc_info=True, + ) + return _null_actor_resolver + + +def _null_actor_resolver(_actor_name: str) -> None: + """Null-object resolver — always returns ``None``.""" + return None + + def _facade_dispatch(operation: str, params: dict[str, Any]) -> dict[str, Any]: """Route an operation through the A2A local facade. -- 2.52.0 From 73d3bed2da330f87e208bf7a293e0f4ef2449f8f Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Sat, 23 May 2026 00:01:44 +0000 Subject: [PATCH 2/5] test(persistence): remove stale @tdd_issue tags from EntityStore persistence tests The _load_from_persistence() and _persist_if_needed() stubs have been replaced with real SQLite persistence implementations, fixing bug #10455. All five scenarios now pass as regular regression tests. Refs: #10455 --- ..._memory_service_entity_persistence.feature | 38 +++++-------------- 1 file changed, 9 insertions(+), 29 deletions(-) diff --git a/features/tdd_memory_service_entity_persistence.feature b/features/tdd_memory_service_entity_persistence.feature index 968bb1837..60506ae4d 100644 --- a/features/tdd_memory_service_entity_persistence.feature +++ b/features/tdd_memory_service_entity_persistence.feature @@ -1,62 +1,42 @@ -# TDD issue-capture test for bug #10455 — EntityStore persistence stubs. +# Regression tests for EntityStore and MemoryService SQL persistence. # -# EntityStore in MemoryService exposes a connection_string parameter that -# implies SQL-backed entity persistence. However, both persistence methods -# are unimplemented stubs: -# -# _load_from_persistence() — contains only `pass`, entities never loaded. -# _persist_if_needed() — marks dirty=False without writing any data. -# -# This creates a silent data-loss bug: callers that supply a connection_string -# expect entities to survive process restarts, but they do not. -# -# These scenarios prove the bug exists by simulating separate process -# invocations (fresh EntityStore / MemoryService instances backed by the -# same SQLite database) and asserting that entities added in one invocation -# are visible in the next. They FAIL until the bug is fixed. -# The @tdd_expected_fail tag inverts the result so CI passes. +# EntityStore in MemoryService now implements real SQL-backed entity +# persistence via _load_from_persistence() and _persist_if_needed(). +# These scenarios verify that entities tracked in one instance survive +# in a fresh instance backed by the same SQLite database. # +# Originally TDD issue-capture tests for bug #10455 (EntityStore stubs). # See: https://git.cleverthis.com/cleveragents/cleveragents-core/issues/10455 -@tdd_issue @tdd_issue_10455 @mock_only -Feature: TDD Issue #10455 — EntityStore entity data lost across process restarts +@mock_only +Feature: EntityStore and MemoryService SQL persistence As a developer using MemoryService with a connection_string I want entities tracked via track_entity() to survive process restarts So that cross-session entity recall works as documented - EntityStore._load_from_persistence() is a stub (pass) and - _persist_if_needed() marks dirty=False without writing data. - A fresh EntityStore instance backed by the same database should - contain entities added by a previous instance. - - @tdd_issue @tdd_issue_10455 Scenario: Entity tracked in one EntityStore instance is visible in a fresh instance Given I create an EntityStore with a SQLite connection string and session "entity-persist-test" When I track a project entity "my-project" in the first EntityStore instance And I create a fresh EntityStore instance with the same connection string and session Then the fresh EntityStore instance should contain the entity "my-project" - @tdd_issue @tdd_issue_10455 Scenario: Entity tracked via MemoryService survives simulated process restart Given I create a MemoryService with a SQLite connection string and session "memory-persist-test" When I track a plan entity "my-plan" via the MemoryService And I create a fresh MemoryService with the same connection string and session Then the fresh MemoryService should return the entity "my-plan" when queried - @tdd_issue @tdd_issue_10455 Scenario: Persistence failure raises an exception rather than silently succeeding Given I create an EntityStore with an invalid connection string When I attempt to track an entity in the EntityStore with invalid connection Then an exception should be raised rather than silently failing - @tdd_issue @tdd_issue_10455 Scenario: Multiple entities survive a simulated process restart Given I create an EntityStore with a SQLite connection string and session "multi-entity-persist" When I track multiple entities in the first EntityStore instance And I create a fresh EntityStore instance with the same connection string and session Then all tracked entities should be present in the fresh EntityStore instance - @tdd_issue @tdd_issue_10455 Scenario: Entity metadata and mention count survive a simulated process restart Given I create an EntityStore with a SQLite connection string and session "entity-metadata-persist" When I track a project entity "project-delta" with metadata @@ -70,4 +50,4 @@ Feature: TDD Issue #10455 — EntityStore entity data lost across process restar | key | value | | owner | alice | | status | active | - And the fresh EntityStore entity "project-delta" should have mention count 2 + And the fresh EntityStore entity "project-delta" should have mention count 2 \ No newline at end of file -- 2.52.0 From cdbe504b2c1b93a5e4bc6376f8ea3a11c6f0654b Mon Sep 17 00:00:00 2001 From: CoreRasurae Date: Sat, 23 May 2026 10:26:05 +0000 Subject: [PATCH 3/5] fix(test): use shared in-memory SQLite URI in _ensure_memory_database_url Replace sqlite:///:memory: with sqlite:///file::memory:?cache=shared to ensure all SQLAlchemy connections within the same process share a single in-memory database. This prevents isolation issues where SQLChatMessageHistory and EntityStore would each create separate in-memory databases, avoiding unexpected errors in CI parallel execution. --- features/steps/plan_service_steps.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/features/steps/plan_service_steps.py b/features/steps/plan_service_steps.py index e1947551a..a418e406f 100644 --- a/features/steps/plan_service_steps.py +++ b/features/steps/plan_service_steps.py @@ -2713,10 +2713,17 @@ def step_check_no_applied_in_pending(context: Context) -> None: def _ensure_memory_database_url(context: Context) -> None: """Ensure plan service uses an in-memory database for memory tests.""" try: - if context.plan_service.settings.database_url != "sqlite:///:memory:": - context.plan_service.settings.database_url = "sqlite:///:memory:" + if ( + context.plan_service.settings.database_url + != "sqlite:///file::memory:?cache=shared" + ): + context.plan_service.settings.database_url = ( + "sqlite:///file::memory:?cache=shared" + ) except AttributeError: - context.plan_service.settings = Settings(database_url="sqlite:///:memory:") + context.plan_service.settings = Settings( + database_url="sqlite:///file::memory:?cache=shared" + ) def _request_memory_service_for_session( -- 2.52.0 From 190606d7e637e5c80d0902a60a6a021ab74f50bd Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Sat, 23 May 2026 12:22:51 +0000 Subject: [PATCH 4/5] fix(memory): handle langchain-community SQLChatMessageHistory API rename langchain-community >= 0.3 renamed connection_string to connection in SQLChatMessageHistory.__init__(). Use the new 'connection' kwarg first and fall back to 'connection_string' for older versions. Also updates the test assertion in memory_service_coverage_steps.py to accept either parameter name. Fixes CI errors in: - plan_service_coverage.feature:128,141 - consolidated_misc.feature:1531 --- .../steps/memory_service_coverage_steps.py | 5 +++- .../application/services/memory_service.py | 29 +++++++++++++------ 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/features/steps/memory_service_coverage_steps.py b/features/steps/memory_service_coverage_steps.py index 0ffcf079f..5e8143aff 100644 --- a/features/steps/memory_service_coverage_steps.py +++ b/features/steps/memory_service_coverage_steps.py @@ -342,7 +342,10 @@ def step_validate_sql_constructor(context: Any) -> None: assert context.sql_history_calls _, kwargs = context.sql_history_calls[0] assert kwargs["session_id"] == "sql-session" - assert kwargs["connection_string"] == context.connection_string + actual_connection = kwargs.get("connection") or kwargs.get("connection_string") + assert actual_connection == context.connection_string, ( + f"Expected {context.connection_string}, got {actual_connection}" + ) assert kwargs.get("table_name") == "message_history" diff --git a/src/cleveragents/application/services/memory_service.py b/src/cleveragents/application/services/memory_service.py index dd85c8e1f..460fdd96e 100644 --- a/src/cleveragents/application/services/memory_service.py +++ b/src/cleveragents/application/services/memory_service.py @@ -485,15 +485,26 @@ class MemoryService: # Initialize message history history: BaseChatMessageHistory if connection_string: - # Use SQL persistence if connection string provided - history = cast( - BaseChatMessageHistory, - SQLChatMessageHistory( - session_id=session_id, - connection_string=connection_string, - table_name="message_history", - ), - ) + # Use SQL persistence if connection string provided. + # langchain-community >= 0.3 renamed connection_string to connection. + try: + history = cast( + BaseChatMessageHistory, + SQLChatMessageHistory( + session_id=session_id, + connection=connection_string, + table_name="message_history", + ), + ) + except TypeError: + history = cast( + BaseChatMessageHistory, + SQLChatMessageHistory( + session_id=session_id, + connection_string=connection_string, + table_name="message_history", + ), + ) else: # Use in-memory storage as fallback history = InMemoryChatMessageHistory() -- 2.52.0 From f1a4858a2969394a97a47b66c492c82a3902d53e Mon Sep 17 00:00:00 2001 From: HAL9000 Date: Sat, 23 May 2026 13:14:21 +0000 Subject: [PATCH 5/5] fix(memory): use **kwargs to avoid Pyright param name mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit langchain-community renamed connection_string → connection, but the locally installed stubs only know connection_string while CI has the newer API. Build kwargs dict dynamically to satisfy both environments without # type: ignore annotations. --- .../application/services/memory_service.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/cleveragents/application/services/memory_service.py b/src/cleveragents/application/services/memory_service.py index 460fdd96e..52a3d8ca8 100644 --- a/src/cleveragents/application/services/memory_service.py +++ b/src/cleveragents/application/services/memory_service.py @@ -487,23 +487,22 @@ class MemoryService: if connection_string: # Use SQL persistence if connection string provided. # langchain-community >= 0.3 renamed connection_string to connection. + kwargs: dict[str, Any] = { + "session_id": session_id, + "table_name": "message_history", + } try: + kwargs["connection"] = connection_string history = cast( BaseChatMessageHistory, - SQLChatMessageHistory( - session_id=session_id, - connection=connection_string, - table_name="message_history", - ), + SQLChatMessageHistory(**kwargs), ) except TypeError: + del kwargs["connection"] + kwargs["connection_string"] = connection_string history = cast( BaseChatMessageHistory, - SQLChatMessageHistory( - session_id=session_id, - connection_string=connection_string, - table_name="message_history", - ), + SQLChatMessageHistory(**kwargs), ) else: # Use in-memory storage as fallback -- 2.52.0