diff --git a/features/context_strategy_registry.feature b/features/context_strategy_registry.feature index 56ad9ba56..e4f22151a 100644 --- a/features/context_strategy_registry.feature +++ b/features/context_strategy_registry.feature @@ -33,7 +33,7 @@ Feature: Context Strategy Registry Scenario: Register a single strategy Given an empty strategy registry When I register the "simple-keyword" built-in strategy - Then the registry should contain "simple-keyword" + Then the strategy registry should contain "simple-keyword" And the registry size should be 1 @strategy_registry @@ -41,12 +41,12 @@ Feature: Context Strategy Registry Given an empty strategy registry When I register all 6 built-in strategies Then the registry should contain 6 strategies - And the registry should contain "simple-keyword" - And the registry should contain "semantic-embedding" - And the registry should contain "breadth-depth-navigator" - And the registry should contain "arce" - And the registry should contain "temporal-archaeology" - And the registry should contain "plan-decision-context" + And the strategy registry should contain "simple-keyword" + And the strategy registry should contain "semantic-embedding" + And the strategy registry should contain "breadth-depth-navigator" + And the strategy registry should contain "arce" + And the strategy registry should contain "temporal-archaeology" + And the strategy registry should contain "plan-decision-context" @strategy_registry @error_handling Scenario: Reject duplicate strategy registration @@ -98,6 +98,12 @@ Feature: Context Strategy Registry When I set the enabled list to "simple-keyword", "arce" Then the enabled strategies should be "simple-keyword", "arce" + @strategy_config + Scenario: Set enabled list deduplicates duplicate names + Given a registry with all built-in strategies using default enabled list + When I set the enabled list to "simple-keyword", "arce", "simple-keyword" + Then the enabled strategies should be "simple-keyword", "arce" + @strategy_config @error_handling Scenario: Set enabled list with unknown strategy raises error Given a registry with all built-in strategies using default enabled list @@ -163,8 +169,8 @@ Feature: Context Strategy Registry Then "arce" can_handle should return 0.0 @strategy_can_handle - Scenario: plan-decision-context always returns quality score - Given a BackendSet with no backends + Scenario: plan-decision-context returns quality score with temporal backend + Given a BackendSet with temporal backend only And a default ContextRequest Then "plan-decision-context" can_handle should return 0.7 @@ -230,7 +236,7 @@ Feature: Context Strategy Registry Scenario: Register a strategy from a module path Given an empty strategy registry When I register a strategy from module "cleveragents.domain.models.acms.strategy_stubs:SimpleKeywordStrategy" - Then the registry should contain "simple-keyword" + Then the strategy registry should contain "simple-keyword" And the registry size should be 1 @strategy_registry @plugin @error_handling @@ -314,7 +320,7 @@ Feature: Context Strategy Registry Given a registry with all built-in strategies When I unregister "temporal-archaeology" Then the registry should contain 5 strategies - And the registry should not contain "temporal-archaeology" + And the strategy registry should not contain "temporal-archaeology" @strategy_registry Scenario: Clear the registry @@ -382,7 +388,7 @@ Feature: Context Strategy Registry Scenario: register_from_module uses the provided name parameter Given an empty strategy registry When I register a strategy named "custom-search" from module "cleveragents.domain.models.acms.strategy_stubs:SimpleKeywordStrategy" - Then the registry should contain "custom-search" + Then the strategy registry should contain "custom-search" And the registry size should be 1 @strategy_config @regression @@ -398,6 +404,13 @@ Feature: Context Strategy Registry When I attempt to register a non-protocol object Then a StrategyRegistrationError should be raised + @strategy_registry @error_handling + Scenario: Reject non-protocol object without explicit name + Given an empty strategy registry + When I attempt to register a non-protocol object without a name + Then a StrategyRegistrationError should be raised + And the error message should contain "does not satisfy" + # --------------------------------------------------------------------------- # explain() coverage # --------------------------------------------------------------------------- @@ -424,8 +437,8 @@ Feature: Context Strategy Registry Then "breadth-depth-navigator" can_handle should return 0.0 @strategy_can_handle - Scenario: temporal-archaeology returns quality score with graph backend - Given a BackendSet with graph backend only + Scenario: temporal-archaeology returns quality score with graph and temporal backends + Given a BackendSet with graph and temporal backends And a default ContextRequest Then "temporal-archaeology" can_handle should return 0.5 @@ -452,3 +465,67 @@ Feature: Context Strategy Registry Given a registry with all built-in strategies When I attempt to update config for "simple-keyword" with max_fragments -1 Then a Pydantic ValidationError should be raised + + # --------------------------------------------------------------------------- + # update_config: resource_types and extra parameters + # --------------------------------------------------------------------------- + + @strategy_config @regression + Scenario: update_config with resource_types + Given a registry with all built-in strategies + When I update config for "simple-keyword" with resource_types "code", "doc" + Then the config for "simple-keyword" should have resource_types "code", "doc" + + @strategy_config @regression + Scenario: update_config with extra dict + Given a registry with all built-in strategies + When I update config for "simple-keyword" with extra key "boost" value "2" + Then the config for "simple-keyword" extra should contain key "boost" + And the config for "simple-keyword" extra should be immutable + + # --------------------------------------------------------------------------- + # MappingProxyType coercion validators + # --------------------------------------------------------------------------- + + @strategy_config @boundary + Scenario: StrategyConfig accepts plain dict for extra field + When I create a StrategyConfig with extra as a plain dict + Then the extra field should be a MappingProxyType + + @strategy_config @boundary + Scenario: StrategyConfig rejects non-dict non-MappingProxyType extra + When I attempt to create a StrategyConfig with extra as an integer + Then a Pydantic ValidationError should be raised + + @strategy_result @boundary + Scenario: ContextStrategyResult accepts plain dict for stats field + When I create a ContextStrategyResult with stats as a plain dict + Then the stats field should be a MappingProxyType + + # --------------------------------------------------------------------------- + # Thread safety + # --------------------------------------------------------------------------- + + @strategy_registry @thread_safety + Scenario: Concurrent register and list_enabled do not raise + Given a registry with all built-in strategies + When 10 threads concurrently register and query the registry + Then no thread should have raised an exception + And the registry should contain at least 10 strategies + And every thread-registered strategy should be present + + # --------------------------------------------------------------------------- + # temporal/cold-tier can_handle regression + # --------------------------------------------------------------------------- + + @strategy_can_handle @regression + Scenario: temporal-archaeology returns 0 without temporal backend + Given a BackendSet with graph backend only + And a default ContextRequest + Then "temporal-archaeology" can_handle should return 0.0 + + @strategy_can_handle @regression + Scenario: plan-decision-context returns 0 without temporal backend + Given a BackendSet with no backends + And a default ContextRequest + Then "plan-decision-context" can_handle should return 0.0 diff --git a/features/steps/context_strategy_registry_steps.py b/features/steps/context_strategy_registry_steps.py index b9dc0cac5..8bcffc0d7 100644 --- a/features/steps/context_strategy_registry_steps.py +++ b/features/steps/context_strategy_registry_steps.py @@ -2,6 +2,8 @@ from __future__ import annotations +from types import MappingProxyType + from behave import given, then, when from behave.runner import Context from pydantic import ValidationError @@ -186,8 +188,8 @@ def step_given_stale_enabled(context: Context) -> None: """Create a registry where the enabled list references a name not registered.""" context.registry = StrategyRegistry() _register_all_builtins(context.registry) - # Manually inject a stale name into the internal enabled list - context.registry._enabled_order.append("ghost-strategy") + # Use the public test helper to inject a stale entry + context.registry.inject_stale_enabled_entry("ghost-strategy") @given("a registry with a strategy that declares no resource types") @@ -472,14 +474,14 @@ def step_then_quality_score(context: Context, name: str, score: float) -> None: ) -@then('the registry should contain "{name}"') +@then('the strategy registry should contain "{name}"') def step_then_contains(context: Context, name: str) -> None: assert context.registry.is_registered(name), ( f"'{name}' not in registry: {context.registry.list_all()}" ) -@then('the registry should not contain "{name}"') +@then('the strategy registry should not contain "{name}"') def step_then_not_contains(context: Context, name: str) -> None: assert not context.registry.is_registered(name), ( f"'{name}' unexpectedly in registry" @@ -738,6 +740,20 @@ def step_when_register_non_protocol(context: Context) -> None: context.error = exc +@when("I attempt to register a non-protocol object without a name") +def step_when_register_non_protocol_no_name(context: Context) -> None: + try: + # No name= override — verifies guard ordering: isinstance check + # must happen before strategy.name access (bug 1.1 fix). + context.registry.register(object()) # type: ignore[arg-type] + context.error = None + except StrategyRegistrationError as exc: + context.error = exc + except AttributeError as exc: # pragma: no cover + # This would indicate the bug is still present + context.error = exc + + @then("every strategy should return a non-empty explain string") def step_then_explain_nonempty(context: Context) -> None: for name, strategy in context.strategies.items(): @@ -807,3 +823,196 @@ def step_when_update_config_bad_fragments( context.error = None except ValidationError as exc: context.error = exc + + +# --------------------------------------------------------------------------- +# Thread safety steps +# --------------------------------------------------------------------------- + + +@when("10 threads concurrently register and query the registry") +def step_when_concurrent_register_query(context: Context) -> None: + """Exercise the registry from multiple threads simultaneously.""" + import threading as _threading + + errors: list[Exception] = [] + barrier = _threading.Barrier(10) + + def _worker(idx: int) -> None: + try: + barrier.wait(timeout=5) + # Mix of reads and writes + if idx % 3 == 0: + # Write: register a unique strategy + name = f"thread-strategy-{idx}" + + class _Stub: + @property + def name(self) -> str: + return name + + @property + def capabilities(self) -> StrategyCapabilities: + return StrategyCapabilities( + uses_text=True, resource_types=("*",), quality_score=0.1 + ) + + def can_handle( + self, request: ContextRequest, backends: BackendSet + ) -> float: + return 0.1 + + def assemble( + self, + request: ContextRequest, + backends: BackendSet, + budget: int, + plan_context: PlanContext, + ) -> list[ContextFragment]: + return [] + + def explain(self) -> str: + return f"thread stub {idx}" + + context.registry.register(_Stub()) + elif idx % 3 == 1: + # Read: list enabled + context.registry.list_enabled() + else: + # Read: list all + context.registry.list_all() + except Exception as exc: + errors.append(exc) + + threads = [_threading.Thread(target=_worker, args=(i,)) for i in range(10)] + for t in threads: + t.start() + for t in threads: + t.join(timeout=10) + + context.thread_errors = errors + + +@then("no thread should have raised an exception") +def step_then_no_thread_errors(context: Context) -> None: + assert not context.thread_errors, f"Thread errors: {context.thread_errors}" + + +@then("the registry should contain at least {count:d} strategies") +def step_then_at_least_count(context: Context, count: int) -> None: + actual = len(context.registry) + assert actual >= count, f"Expected at least {count}, got {actual}" + + +@then("every thread-registered strategy should be present") +def step_then_thread_strategies_present(context: Context) -> None: + for idx in [0, 3, 6, 9]: + name = f"thread-strategy-{idx}" + assert context.registry.is_registered(name), ( + f"Thread-registered '{name}' missing from registry" + ) + + +# --------------------------------------------------------------------------- +# Temporal/cold-tier backend steps +# --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# update_config: resource_types and extra steps +# --------------------------------------------------------------------------- + + +@when('I update config for "{name}" with resource_types "{rt1}", "{rt2}"') +def step_when_update_resource_types( + context: Context, name: str, rt1: str, rt2: str +) -> None: + context.registry.update_config(name, resource_types=(rt1, rt2)) + + +@then('the config for "{name}" should have resource_types "{rt1}", "{rt2}"') +def step_then_resource_types(context: Context, name: str, rt1: str, rt2: str) -> None: + cfg = context.registry.get_config(name) + assert cfg.resource_types == (rt1, rt2), ( + f"Expected ({rt1!r}, {rt2!r}), got {cfg.resource_types}" + ) + + +@when('I update config for "{name}" with extra key "{key}" value "{val}"') +def step_when_update_extra(context: Context, name: str, key: str, val: str) -> None: + context.registry.update_config(name, extra={key: val}) + + +@then('the config for "{name}" extra should contain key "{key}"') +def step_then_extra_contains(context: Context, name: str, key: str) -> None: + cfg = context.registry.get_config(name) + assert key in cfg.extra, f"Key '{key}' not in extra: {cfg.extra}" + + +@then('the config for "{name}" extra should be immutable') +def step_then_extra_immutable(context: Context, name: str) -> None: + cfg = context.registry.get_config(name) + assert isinstance(cfg.extra, MappingProxyType), ( + f"Expected MappingProxyType, got {type(cfg.extra).__name__}" + ) + try: + cfg.extra["hack"] = "should fail" # type: ignore[index] + raise AssertionError("MappingProxyType should reject item assignment") + except TypeError: + pass # Expected — MappingProxyType is immutable + + +# --------------------------------------------------------------------------- +# MappingProxyType coercion validator steps +# --------------------------------------------------------------------------- + + +@when("I create a StrategyConfig with extra as a plain dict") +def step_when_config_plain_dict_extra(context: Context) -> None: + context.created_config = StrategyConfig(extra={"key": "val"}) # type: ignore[arg-type] + + +@then("the extra field should be a MappingProxyType") +def step_then_extra_is_mapping_proxy(context: Context) -> None: + assert isinstance(context.created_config.extra, MappingProxyType), ( + f"Expected MappingProxyType, got {type(context.created_config.extra).__name__}" + ) + + +@when("I attempt to create a StrategyConfig with extra as an integer") +def step_when_config_int_extra(context: Context) -> None: + try: + StrategyConfig(extra=42) # type: ignore[arg-type] + context.error = None + except ValidationError as exc: + context.error = exc + + +@when("I create a ContextStrategyResult with stats as a plain dict") +def step_when_result_plain_dict_stats(context: Context) -> None: + context.created_result = ContextStrategyResult( + strategy_name="test", + stats={"ops": 10}, # type: ignore[arg-type] + ) + + +@then("the stats field should be a MappingProxyType") +def step_then_stats_is_mapping_proxy(context: Context) -> None: + assert isinstance(context.created_result.stats, MappingProxyType), ( + f"Expected MappingProxyType, got {type(context.created_result.stats).__name__}" + ) + + +@given("a BackendSet with graph and temporal backends") +def step_given_backends_graph_temporal(context: Context) -> None: + context.backend_set = BackendSet( + graph=InMemoryGraphBackend(), + temporal=object(), # Presence indicates availability + ) + + +@given("a BackendSet with temporal backend only") +def step_given_backends_temporal_only(context: Context) -> None: + context.backend_set = BackendSet( + temporal=object(), # Presence indicates availability + ) diff --git a/src/cleveragents/application/services/strategy_registry.py b/src/cleveragents/application/services/strategy_registry.py index 425000e9f..3998c2cac 100644 --- a/src/cleveragents/application/services/strategy_registry.py +++ b/src/cleveragents/application/services/strategy_registry.py @@ -17,6 +17,9 @@ Based on ``docs/specification.md`` §§ 25218-25233, 28682-28708, from __future__ import annotations import importlib +import threading +from types import MappingProxyType +from typing import Any import structlog @@ -74,11 +77,39 @@ class StrategyRegistry: result = registry.get("simple-keyword") """ - def __init__(self) -> None: - """Initialize an empty registry.""" + # INVARIANTS (must hold after every public method returns): + # 1. _strategies.keys() == _entries.keys() + # 2. set(_enabled_order) is a subset of _strategies.keys() + # 3. For all n in _enabled_order: _entries[n].config.enabled is True + # 4. For all n not in _enabled_order: _entries[n].config.enabled is False + + # Default module prefix allowlist for register_from_module(). + # Only modules under these prefixes may be dynamically imported. + # Override via the ``allowed_module_prefixes`` constructor parameter. + DEFAULT_ALLOWED_MODULE_PREFIXES: tuple[str, ...] = ("cleveragents.",) + + def __init__( + self, + *, + allowed_module_prefixes: tuple[str, ...] | None = None, + ) -> None: + """Initialize an empty registry. + + Args: + allowed_module_prefixes: Module prefixes permitted for + dynamic import via :meth:`register_from_module`. + Defaults to ``("cleveragents.",)``. Pass an empty + tuple to disable the allowlist (not recommended). + """ + self._lock = threading.RLock() self._strategies: dict[str, ContextStrategy] = {} self._entries: dict[str, StrategyRegistryEntry] = {} self._enabled_order: list[str] = [] + self._allowed_module_prefixes = ( + allowed_module_prefixes + if allowed_module_prefixes is not None + else self.DEFAULT_ALLOWED_MODULE_PREFIXES + ) # ------------------------------------------------------------------ # Registration @@ -110,32 +141,38 @@ class StrategyRegistry: already registered, or the strategy doesn't satisfy the ``ContextStrategy`` protocol. """ + # Validate protocol conformance FIRST — before accessing any + # attributes (e.g., strategy.name) that a non-protocol object + # may not have. + if not isinstance(strategy, ContextStrategy): + label = name or type(strategy).__name__ + raise StrategyRegistrationError( + f"Strategy '{label}' does not satisfy the ContextStrategy protocol" + ) + name = name or strategy.name if not name: raise StrategyRegistrationError("Strategy name must not be empty") - if name in self._strategies: - raise StrategyRegistrationError(f"Strategy '{name}' is already registered") + with self._lock: + if name in self._strategies: + raise StrategyRegistrationError( + f"Strategy '{name}' is already registered" + ) - # Validate protocol conformance - if not isinstance(strategy, ContextStrategy): - raise StrategyRegistrationError( - f"Strategy '{name}' does not satisfy the ContextStrategy protocol" + resolved_config = config or StrategyConfig() + entry = StrategyRegistryEntry( + name=name, + config=resolved_config, + module_path=module_path, + is_builtin=is_builtin, ) - resolved_config = config or StrategyConfig() - entry = StrategyRegistryEntry( - name=name, - config=resolved_config, - module_path=module_path, - is_builtin=is_builtin, - ) + self._strategies[name] = strategy + self._entries[name] = entry - self._strategies[name] = strategy - self._entries[name] = entry - - if resolved_config.enabled: - self._enabled_order.append(name) + if resolved_config.enabled: + self._enabled_order.append(name) logger.debug( "strategy.registered", @@ -156,6 +193,16 @@ class StrategyRegistry: Performs plugin discovery per spec §25225-25226: ``[context.strategies.custom] "name" = "module:Class"``. + Note: + Spec §25226 uses dot notation + (``my_package.strategies.DomainStrategy``) while §25232 + uses colon notation + (``my_extensions.scorers:DomainAwareScorer``). + We use **colon notation** (``module:ClassName``) because + it unambiguously separates the importable module from the + class name, matching Python entry-point conventions. + The spec is internally inconsistent here. + Security: Module paths should come from trusted configuration only (e.g., project TOML files). Importing a module executes its @@ -170,9 +217,10 @@ class StrategyRegistry: config: Per-strategy configuration. Raises: - StrategyRegistrationError: If import fails, the class - doesn't exist, or the instance doesn't satisfy the - ``ContextStrategy`` protocol. + StrategyRegistrationError: If the module prefix is not in + the allowlist, import fails, the class doesn't exist, + or the instance doesn't satisfy the ``ContextStrategy`` + protocol. """ if ":" not in module_path: raise StrategyRegistrationError( @@ -182,6 +230,17 @@ class StrategyRegistry: module_name, class_name = module_path.rsplit(":", 1) + # CWE-706: Restrict dynamic imports to an allowlist of trusted + # module prefixes to prevent arbitrary code execution. + if self._allowed_module_prefixes and not any( + module_name.startswith(prefix) for prefix in self._allowed_module_prefixes + ): + raise StrategyRegistrationError( + f"Module '{module_name}' is not in the allowed prefix list: " + f"{self._allowed_module_prefixes}. Only modules under these " + f"prefixes may be dynamically imported." + ) + try: module = importlib.import_module(module_name) except ImportError as exc: @@ -226,12 +285,13 @@ class StrategyRegistry: Raises: StrategyNotFoundError: If the name is not registered. """ - if name not in self._strategies: - raise StrategyNotFoundError( - f"Strategy '{name}' not found in registry. " - f"Available: {sorted(self._strategies)}" - ) - return self._strategies[name] + with self._lock: + if name not in self._strategies: + raise StrategyNotFoundError( + f"Strategy '{name}' not found in registry. " + f"Available: {sorted(self._strategies)}" + ) + return self._strategies[name] def get_entry(self, name: str) -> StrategyRegistryEntry: """Return the registry entry (metadata + config) for a strategy. @@ -239,9 +299,10 @@ class StrategyRegistry: Raises: StrategyNotFoundError: If the name is not registered. """ - if name not in self._entries: - raise StrategyNotFoundError(f"Strategy '{name}' not found in registry") - return self._entries[name] + with self._lock: + if name not in self._entries: + raise StrategyNotFoundError(f"Strategy '{name}' not found in registry") + return self._entries[name] def get_config(self, name: str) -> StrategyConfig: """Return the configuration for a registered strategy. @@ -253,7 +314,8 @@ class StrategyRegistry: def list_all(self) -> list[str]: """Return names of all registered strategies.""" - return sorted(self._strategies) + with self._lock: + return sorted(self._strategies) def list_enabled(self) -> list[str]: """Return names of enabled strategies in registration order. @@ -261,27 +323,32 @@ class StrategyRegistry: The order matches the ``[context.strategies] enabled`` list, which controls priority during selection (spec §28682). """ - return [ - n - for n in self._enabled_order - if n in self._entries and self._entries[n].config.enabled - ] + with self._lock: + return [ + n + for n in self._enabled_order + if n in self._entries and self._entries[n].config.enabled + ] def list_builtin(self) -> list[str]: """Return names of built-in strategies.""" - return sorted(n for n, e in self._entries.items() if e.is_builtin) + with self._lock: + return sorted(n for n, e in self._entries.items() if e.is_builtin) def is_registered(self, name: str) -> bool: """Check whether a strategy is registered.""" - return name in self._strategies + with self._lock: + return name in self._strategies def __len__(self) -> int: """Return the number of registered strategies.""" - return len(self._strategies) + with self._lock: + return len(self._strategies) - def __contains__(self, name: str) -> bool: + def __contains__(self, name: object) -> bool: """Check whether a strategy name is registered.""" - return name in self._strategies + with self._lock: + return name in self._strategies # ------------------------------------------------------------------ # Configuration @@ -303,23 +370,26 @@ class StrategyRegistry: Raises: StrategyNotFoundError: If any name is not registered. """ - for name in names: - if name not in self._strategies: - raise StrategyNotFoundError(f"Cannot enable unknown strategy '{name}'") + with self._lock: + for name in names: + if name not in self._strategies: + raise StrategyNotFoundError( + f"Cannot enable unknown strategy '{name}'" + ) - # Update config.enabled for all strategies - enabled_set = set(names) - for sname, entry in self._entries.items(): - should_enable = sname in enabled_set - if entry.config.enabled != should_enable: - new_config = entry.config.model_copy( - update={"enabled": should_enable}, - ) - self._entries[sname] = entry.model_copy( - update={"config": new_config}, - ) + # Update config.enabled for all strategies + enabled_set = set(names) + for sname, entry in self._entries.items(): + should_enable = sname in enabled_set + if entry.config.enabled != should_enable: + new_config = entry.config.model_copy( + update={"enabled": should_enable}, + ) + self._entries[sname] = entry.model_copy( + update={"config": new_config}, + ) - self._enabled_order = list(names) + self._enabled_order = list(dict.fromkeys(names)) def update_config( self, @@ -330,6 +400,8 @@ class StrategyRegistry: max_fragments: int | None = None, max_workers: int | None = None, circuit_breaker_threshold: int | None = None, + resource_types: tuple[str, ...] | None = None, + extra: dict[str, Any] | None = None, ) -> None: """Update configuration for a registered strategy. @@ -343,35 +415,42 @@ class StrategyRegistry: max_fragments: Max fragments per call (>=1). max_workers: Max parallel workers (>=1). circuit_breaker_threshold: Failures before circuit opens (>=1). + resource_types: Resource types this strategy is limited to. + extra: Strategy-specific extra configuration. Raises: StrategyNotFoundError: If the name is not registered. pydantic.ValidationError: If the updated values violate ``StrategyConfig`` constraints (e.g., ``timeout_seconds < 1``). """ - entry = self.get_entry(name) - updates: dict[str, bool | int] = {} - if enabled is not None: - updates["enabled"] = enabled - if timeout_seconds is not None: - updates["timeout_seconds"] = timeout_seconds - if max_fragments is not None: - updates["max_fragments"] = max_fragments - if max_workers is not None: - updates["max_workers"] = max_workers - if circuit_breaker_threshold is not None: - updates["circuit_breaker_threshold"] = circuit_breaker_threshold - merged = entry.config.model_dump() - merged.update(updates) - new_config = StrategyConfig.model_validate(merged) - new_entry = entry.model_copy(update={"config": new_config}) - self._entries[name] = new_entry + with self._lock: + entry = self.get_entry(name) + updates: dict[str, Any] = {} + if enabled is not None: + updates["enabled"] = enabled + if timeout_seconds is not None: + updates["timeout_seconds"] = timeout_seconds + if max_fragments is not None: + updates["max_fragments"] = max_fragments + if max_workers is not None: + updates["max_workers"] = max_workers + if circuit_breaker_threshold is not None: + updates["circuit_breaker_threshold"] = circuit_breaker_threshold + if resource_types is not None: + updates["resource_types"] = resource_types + if extra is not None: + updates["extra"] = MappingProxyType(extra) + merged = entry.config.model_dump() + merged.update(updates) + new_config = StrategyConfig.model_validate(merged) + new_entry = entry.model_copy(update={"config": new_config}) + self._entries[name] = new_entry - # Keep _enabled_order in sync with the enabled flag - if enabled is True and name not in self._enabled_order: - self._enabled_order.append(name) - elif enabled is False and name in self._enabled_order: - self._enabled_order = [n for n in self._enabled_order if n != name] + # Keep _enabled_order in sync with the enabled flag + if enabled is True and name not in self._enabled_order: + self._enabled_order.append(name) + elif enabled is False and name in self._enabled_order: + self._enabled_order = [n for n in self._enabled_order if n != name] # ------------------------------------------------------------------ # Validation @@ -389,31 +468,34 @@ class StrategyRegistry: Returns: List of warning messages (empty = valid). """ - warnings: list[str] = [] + with self._lock: + warnings: list[str] = [] - for name in self._enabled_order: - if name not in self._strategies: - warnings.append(f"Enabled strategy '{name}' is not registered") + for name in self._enabled_order: + if name not in self._strategies: + warnings.append(f"Enabled strategy '{name}' is not registered") - for name, strategy in self._strategies.items(): - caps = strategy.capabilities + for name, strategy in self._strategies.items(): + caps = strategy.capabilities - has_any_backend = ( - caps.uses_text - or caps.uses_vector - or caps.uses_graph - or caps.uses_temporal - ) - if not has_any_backend: - warnings.append(f"Strategy '{name}' declares no backend capabilities") - - if not caps.resource_types: - warnings.append( - f"Strategy '{name}' does not declare supported " - f"resource types (capabilities.resource_types is empty)" + has_any_backend = ( + caps.uses_text + or caps.uses_vector + or caps.uses_graph + or caps.uses_temporal ) + if not has_any_backend: + warnings.append( + f"Strategy '{name}' declares no backend capabilities" + ) - return warnings + if not caps.resource_types: + warnings.append( + f"Strategy '{name}' does not declare supported " + f"resource types (capabilities.resource_types is empty)" + ) + + return warnings # ------------------------------------------------------------------ # Removal (for testing / reconfiguration) @@ -428,15 +510,33 @@ class StrategyRegistry: Raises: StrategyNotFoundError: If the name is not registered. """ - if name not in self._strategies: - raise StrategyNotFoundError(f"Cannot unregister unknown strategy '{name}'") + with self._lock: + if name not in self._strategies: + raise StrategyNotFoundError( + f"Cannot unregister unknown strategy '{name}'" + ) - del self._strategies[name] - del self._entries[name] - self._enabled_order = [n for n in self._enabled_order if n != name] + del self._strategies[name] + del self._entries[name] + self._enabled_order = [n for n in self._enabled_order if n != name] def clear(self) -> None: """Remove all registered strategies.""" - self._strategies.clear() - self._entries.clear() - self._enabled_order.clear() + with self._lock: + self._strategies.clear() + self._entries.clear() + self._enabled_order.clear() + + def inject_stale_enabled_entry(self, name: str) -> None: + """Inject a stale name into the enabled list for testing. + + This is a test helper that creates a deliberately inconsistent + state where the enabled list references a name that is not + registered. Used to verify that :meth:`validate_registry` and + :meth:`list_enabled` handle stale entries correctly. + + Args: + name: A strategy name that is NOT currently registered. + """ + with self._lock: + self._enabled_order.append(name) diff --git a/src/cleveragents/domain/models/acms/strategy.py b/src/cleveragents/domain/models/acms/strategy.py index f8e132cc8..832db0675 100644 --- a/src/cleveragents/domain/models/acms/strategy.py +++ b/src/cleveragents/domain/models/acms/strategy.py @@ -13,6 +13,7 @@ All frozen Pydantic models follow ADR-004 immutability rules. from __future__ import annotations +from types import MappingProxyType from typing import Any, Protocol, runtime_checkable from pydantic import BaseModel, ConfigDict, Field, field_validator @@ -27,6 +28,16 @@ from cleveragents.domain.models.acms.crp import ( ContextRequest, ) +__all__ = [ + "BackendSet", + "ContextStrategy", + "ContextStrategyResult", + "PlanContext", + "StrategyCapabilities", + "StrategyConfig", + "StrategyRegistryEntry", +] + # --------------------------------------------------------------------------- # BackendSet — container for available data backends # --------------------------------------------------------------------------- @@ -54,6 +65,14 @@ class BackendSet(BaseModel, frozen=True): default=None, description="Knowledge graph backend (e.g., Blazegraph, Neo4j)", ) + temporal: object | None = Field( + default=None, + description=( + "Temporal/cold-tier backend for historical pattern discovery. " + "Typed as ``object`` until a formal TemporalBackend protocol " + "is defined; presence (not ``None``) indicates availability." + ), + ) model_config = ConfigDict(arbitrary_types_allowed=True) @@ -241,11 +260,27 @@ class StrategyConfig(BaseModel, frozen=True): default=(), description="Resource types this strategy is limited to (empty = all)", ) - extra: dict[str, Any] = Field( - default_factory=dict, - description="Strategy-specific extra configuration", + extra: MappingProxyType[str, Any] = Field( + default_factory=lambda: MappingProxyType({}), + description="Strategy-specific extra configuration (immutable)", ) + model_config = ConfigDict(arbitrary_types_allowed=True) + + @field_validator("extra", mode="before") + @classmethod + def _coerce_extra( + cls: type[StrategyConfig], + v: Any, + ) -> MappingProxyType[str, Any]: + """Wrap plain dicts in MappingProxyType for immutability (ADR-004).""" + if isinstance(v, MappingProxyType): + return v + if isinstance(v, dict): + return MappingProxyType(v) + msg = f"expected dict or MappingProxyType, got {type(v).__name__}" + raise ValueError(msg) + # --------------------------------------------------------------------------- # ContextStrategyResult — output of a strategy execution @@ -290,11 +325,13 @@ class ContextStrategyResult(BaseModel, frozen=True): default=(), description="Error messages encountered during execution", ) - stats: dict[str, Any] = Field( - default_factory=dict, - description="Strategy-specific statistics", + stats: MappingProxyType[str, Any] = Field( + default_factory=lambda: MappingProxyType({}), + description="Strategy-specific statistics (immutable)", ) + model_config = ConfigDict(arbitrary_types_allowed=True) + @field_validator("fragments") @classmethod def _sort_fragments( @@ -306,6 +343,20 @@ class ContextStrategyResult(BaseModel, frozen=True): sorted(v, key=lambda f: (-f.relevance_score, f.uko_node)), ) + @field_validator("stats", mode="before") + @classmethod + def _coerce_stats( + cls: type[ContextStrategyResult], + v: Any, + ) -> MappingProxyType[str, Any]: + """Wrap plain dicts in MappingProxyType for immutability (ADR-004).""" + if isinstance(v, MappingProxyType): + return v + if isinstance(v, dict): + return MappingProxyType(v) + msg = f"expected dict or MappingProxyType, got {type(v).__name__}" + raise ValueError(msg) + # --------------------------------------------------------------------------- # StrategyRegistryEntry — metadata wrapper for registered strategies diff --git a/src/cleveragents/domain/models/acms/strategy_stubs.py b/src/cleveragents/domain/models/acms/strategy_stubs.py index b8f4a2e76..0b9aee7f6 100644 --- a/src/cleveragents/domain/models/acms/strategy_stubs.py +++ b/src/cleveragents/domain/models/acms/strategy_stubs.py @@ -23,6 +23,8 @@ Six built-in strategies per spec: from __future__ import annotations +import functools + from cleveragents.domain.models.acms.crp import ( ContextFragment, ContextRequest, @@ -33,6 +35,17 @@ from cleveragents.domain.models.acms.strategy import ( StrategyCapabilities, ) +__all__ = [ + "BUILTIN_STRATEGY_CLASSES", + "DEFAULT_ENABLED_STRATEGIES", + "ARCEStrategy", + "BreadthDepthNavigatorStrategy", + "PlanDecisionContextStrategy", + "SemanticEmbeddingStrategy", + "SimpleKeywordStrategy", + "TemporalArchaeologyStrategy", +] + # --------------------------------------------------------------------------- # simple-keyword (spec §25211, §43171-43173) # --------------------------------------------------------------------------- @@ -51,7 +64,7 @@ class SimpleKeywordStrategy: def name(self) -> str: return "simple-keyword" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=True, @@ -106,7 +119,7 @@ class SemanticEmbeddingStrategy: def name(self) -> str: return "semantic-embedding" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=False, @@ -161,7 +174,7 @@ class BreadthDepthNavigatorStrategy: def name(self) -> str: return "breadth-depth-navigator" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=False, @@ -217,7 +230,7 @@ class ARCEStrategy: def name(self) -> str: return "arce" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=True, @@ -276,7 +289,7 @@ class TemporalArchaeologyStrategy: def name(self) -> str: return "temporal-archaeology" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=False, @@ -293,7 +306,8 @@ class TemporalArchaeologyStrategy: request: ContextRequest, backends: BackendSet, ) -> float: - if backends.graph is not None: + # Requires both graph AND temporal/cold-tier (spec §43193-43195). + if backends.graph is not None and backends.temporal is not None: return self.capabilities.quality_score return 0.0 @@ -332,7 +346,7 @@ class PlanDecisionContextStrategy: def name(self) -> str: return "plan-decision-context" - @property + @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities( uses_text=False, @@ -349,8 +363,10 @@ class PlanDecisionContextStrategy: request: ContextRequest, backends: BackendSet, ) -> float: - # Works even without graph; reads from warm/cold tiers. - return self.capabilities.quality_score + # Requires temporal/cold-tier backend (spec §43197-43199). + if backends.temporal is not None: + return self.capabilities.quality_score + return 0.0 def assemble( self, @@ -376,6 +392,12 @@ class PlanDecisionContextStrategy: # Canonical ordered list per spec §28682 default: # ["simple-keyword", "semantic-embedding", "breadth-depth-navigator"] # Plus the remaining 3 registered but not enabled by default. +# +# NOTE: Spec §25223 lists 4 strategies (including "arce") in the default +# enabled config, but §28682 lists only 3 (without "arce"). This is a +# spec internal contradiction. We follow §28682 (the config key table) +# because it is the more specific/operational reference. This decision +# should be confirmed with the spec author. BUILTIN_STRATEGY_CLASSES: tuple[type, ...] = ( SimpleKeywordStrategy, diff --git a/vulture_whitelist.py b/vulture_whitelist.py index 0bb07a73a..12182182b 100644 --- a/vulture_whitelist.py +++ b/vulture_whitelist.py @@ -622,6 +622,7 @@ dependency_type # noqa: B018, F821 # Context Strategy Registry — protocol parameter required by ContextStrategy.assemble() plan_context # noqa: B018, F821 +inject_stale_enabled_entry # noqa: B018, F821 # ACMS Backend Abstraction Layer — public API (issue #498) TextBackend # noqa: B018, F821