fix(acms): address PR #565 review findings from CoreRasurae
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 14s
CI / build (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 18s
CI / typecheck (pull_request) Successful in 43s
CI / security (pull_request) Successful in 1m1s
CI / unit_tests (pull_request) Successful in 2m18s
CI / docker (pull_request) Successful in 47s
CI / integration_tests (pull_request) Successful in 4m48s
CI / coverage (pull_request) Successful in 4m29s
CI / benchmark-regression (pull_request) Successful in 29m8s

Resolve 14 review findings across bugs, spec deviations, design gaps,
security, performance, thread safety, and test quality:

- Fix register() guard ordering: isinstance check before attribute access
- Add BackendSet.temporal field for cold-tier backend availability
- Fix temporal-archaeology/plan-decision-context can_handle to require
  temporal backend (spec §43193-43199)
- Add threading.RLock to StrategyRegistry for thread safety
- Add allowed_module_prefixes (CWE-706) to register_from_module
- Change StrategyConfig.extra and ContextStrategyResult.stats to
  MappingProxyType with field_validator coercion (ADR-004 immutability)
- Switch 6 stub capabilities from @property to @functools.cached_property
- Add resource_types/extra params to update_config()
- Add inject_stale_enabled_entry() public test helper
- Rename colliding step patterns to avoid AmbiguousStep with master
- Add scenarios: non-protocol without name, concurrent threads,
  temporal backend regression tests
- Document spec §25223 vs §28682 conflict and colon vs dot notation
This commit is contained in:
2026-03-05 19:56:49 +00:00
parent 1521c4ae8c
commit b7effcafc1
6 changed files with 601 additions and 141 deletions
+91 -14
View File
@@ -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
@@ -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
)
@@ -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)
@@ -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
@@ -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,
+1
View File
@@ -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