feat(acms): add context strategy registry with plugin discovery #565

Merged
hamza.khyari merged 2 commits from feature/m6-acms-strategy-registry into master 2026-03-05 23:40:18 +00:00
Member

Summary

Implements the ACMS context strategy registry with plugin discovery, enabling extensible context assembly strategies within the ACMS pipeline. Per spec §25162–25233, §28682–28708, §42628–42653, and §43167–43199.

Changes

Domain Models (src/cleveragents/domain/models/acms/strategy.py)

  • ContextStrategy protocol defining the strategy interface (name, capabilities, can_handle, execute, explain)
  • StrategyCapabilities model with supported resource types and backend requirements
  • BackendSet, PlanContext, StrategyConfig configuration models
  • ContextStrategyResult with deterministic fragment ordering (-relevance_score, uko_node)
  • MappingProxyType for StrategyConfig.extra and ContextStrategyResult.stats (ADR-004 immutability)

Built-in Stub Strategies (src/cleveragents/domain/models/acms/strategy_stubs.py)

  • 6 strategies: simple-keyword, semantic-embedding, breadth-depth-navigator, arce, temporal-archaeology, plan-decision-context
  • Each declares spec-aligned quality scores and feature flags
  • @functools.cached_property for capabilities (avoids per-access Pydantic allocation)
  • No-op defaults for strategies requiring backends not yet available

Strategy Registry (src/cleveragents/application/services/strategy_registry.py)

  • StrategyRegistry with register(), register_from_module() (plugin discovery), enable()/disable()
  • Per-strategy timeout, max-fragment limits, and circuit-breaker configuration
  • Registry validation ensuring strategies declare supported resource types
  • Configuration-driven enabled list with per-project overrides
  • threading.RLock for thread safety on all public methods
  • allowed_module_prefixes allowlist for dynamic import security (CWE-706)

Documentation (docs/reference/context_strategies.md)

  • Strategy contracts, configuration keys, and expected outputs

Tests

  • 72 BDD scenarios (features/context_strategy_registry.feature)
  • 4 Robot integration tests (robot/context_strategy_registry.robot)
  • ASV benchmarks for registry lookup (benchmarks/context_strategy_bench.py)
  • Thread safety test with post-condition invariant verification
  • MappingProxyType coercion validator tests

Closes #191

## Summary Implements the ACMS context strategy registry with plugin discovery, enabling extensible context assembly strategies within the ACMS pipeline. Per spec §25162–25233, §28682–28708, §42628–42653, and §43167–43199. ## Changes ### Domain Models (`src/cleveragents/domain/models/acms/strategy.py`) - `ContextStrategy` protocol defining the strategy interface (`name`, `capabilities`, `can_handle`, `execute`, `explain`) - `StrategyCapabilities` model with supported resource types and backend requirements - `BackendSet`, `PlanContext`, `StrategyConfig` configuration models - `ContextStrategyResult` with deterministic fragment ordering (`-relevance_score`, `uko_node`) - `MappingProxyType` for `StrategyConfig.extra` and `ContextStrategyResult.stats` (ADR-004 immutability) ### Built-in Stub Strategies (`src/cleveragents/domain/models/acms/strategy_stubs.py`) - 6 strategies: `simple-keyword`, `semantic-embedding`, `breadth-depth-navigator`, `arce`, `temporal-archaeology`, `plan-decision-context` - Each declares spec-aligned quality scores and feature flags - `@functools.cached_property` for capabilities (avoids per-access Pydantic allocation) - No-op defaults for strategies requiring backends not yet available ### Strategy Registry (`src/cleveragents/application/services/strategy_registry.py`) - `StrategyRegistry` with `register()`, `register_from_module()` (plugin discovery), `enable()`/`disable()` - Per-strategy timeout, max-fragment limits, and circuit-breaker configuration - Registry validation ensuring strategies declare supported resource types - Configuration-driven enabled list with per-project overrides - `threading.RLock` for thread safety on all public methods - `allowed_module_prefixes` allowlist for dynamic import security (CWE-706) ### Documentation (`docs/reference/context_strategies.md`) - Strategy contracts, configuration keys, and expected outputs ### Tests - 72 BDD scenarios (`features/context_strategy_registry.feature`) - 4 Robot integration tests (`robot/context_strategy_registry.robot`) - ASV benchmarks for registry lookup (`benchmarks/context_strategy_bench.py`) - Thread safety test with post-condition invariant verification - MappingProxyType coercion validator tests Closes #191
hamza.khyari changed title from feat(acms): add context strategy registry with plugin discovery (#191) to feat(acms): add context strategy registry with plugin discovery 2026-03-04 19:35:59 +00:00
freemo added this to the v3.5.0 milestone 2026-03-05 03:28:27 +00:00
Owner

@hamza.khyari — This PR has no milestone assigned, no labels, and a merge conflict with master. I've added State/In Review, Priority/Low, Type/Feature labels and assigned it to milestone v3.5.0.

Action required: Please rebase this branch (feature/m6-acms-strategy-registry) onto the latest master to resolve the merge conflict. The ACMS v1 pipeline (#188) has since merged, which likely caused the conflict. If this PR has been superseded by other work, please close it with an explanation.

This is blocking the strategy registry work item (#191) which is on the M5 critical path.

@hamza.khyari — This PR has no milestone assigned, no labels, and a merge conflict with master. I've added `State/In Review`, `Priority/Low`, `Type/Feature` labels and assigned it to milestone v3.5.0. **Action required**: Please rebase this branch (`feature/m6-acms-strategy-registry`) onto the latest master to resolve the merge conflict. The ACMS v1 pipeline (#188) has since merged, which likely caused the conflict. If this PR has been superseded by other work, please close it with an explanation. This is blocking the strategy registry work item (#191) which is on the M5 critical path.
hamza.khyari force-pushed feature/m6-acms-strategy-registry from 2ebce4da56
Some checks failed
CI / lint (pull_request) Successful in 15s
CI / typecheck (pull_request) Successful in 31s
CI / quality (pull_request) Successful in 15s
CI / security (pull_request) Successful in 34s
CI / benchmark-publish (pull_request) Has been skipped
CI / build (pull_request) Successful in 17s
CI / unit_tests (pull_request) Successful in 1m59s
CI / integration_tests (pull_request) Successful in 3m56s
CI / docker (pull_request) Successful in 2m8s
CI / coverage (pull_request) Failing after 5m8s
CI / benchmark-regression (pull_request) Successful in 35m58s
to 1942b9ef6a
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 17s
CI / build (pull_request) Successful in 20s
CI / security (pull_request) Successful in 32s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 1m54s
CI / docker (pull_request) Successful in 52s
CI / integration_tests (pull_request) Successful in 2m56s
CI / coverage (pull_request) Failing after 4m10s
CI / benchmark-regression (pull_request) Successful in 25m50s
2026-03-05 15:36:42 +00:00
Compare
hamza.khyari force-pushed feature/m6-acms-strategy-registry from 1942b9ef6a
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 17s
CI / build (pull_request) Successful in 20s
CI / security (pull_request) Successful in 32s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 1m54s
CI / docker (pull_request) Successful in 52s
CI / integration_tests (pull_request) Successful in 2m56s
CI / coverage (pull_request) Failing after 4m10s
CI / benchmark-regression (pull_request) Successful in 25m50s
to 98c1fa3125
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 13s
CI / build (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 18s
CI / security (pull_request) Successful in 31s
CI / typecheck (pull_request) Successful in 33s
CI / unit_tests (pull_request) Successful in 1m56s
CI / docker (pull_request) Successful in 40s
CI / integration_tests (pull_request) Successful in 3m6s
CI / coverage (pull_request) Successful in 4m49s
CI / benchmark-regression (pull_request) Successful in 25m44s
2026-03-05 16:08:38 +00:00
Compare
CoreRasurae left a comment

Code Review — Commit 98c1fa31 (feat(acms): add context strategy registry)

Reviewer scope: Bug detection, security, performance, thread safety, test quality, and spec conformance against issue #191 and docs/specification.md.

Overall the implementation is well-structured: the domain models, protocol design, and 55 BDD scenarios are thorough. However, I found several issues worth addressing before merge.


1. BUGS

1.1 [HIGH] register() can raise AttributeError instead of StrategyRegistrationError

File: src/cleveragents/application/services/strategy_registry.py:113

When name=None (the default), the code accesses strategy.name before the protocol check at line 121:

name = name or strategy.name          # line 113 — AttributeError if not a protocol object
# ...
if not isinstance(strategy, ContextStrategy):  # line 121 — never reached

If a caller passes a non-protocol object without an explicit name, this raises a raw AttributeError rather than the intended StrategyRegistrationError. The existing test sidesteps this by always passing name="fake". Fix: move the isinstance check before the name resolution, or wrap strategy.name access in a try/except.

1.2 [MEDIUM] temporal-archaeology can_handle only checks graph, not cold-tier

File: src/cleveragents/domain/models/acms/strategy_stubs.py:291-298

Spec §25215 / §43193-43195 says this strategy requires Graph + Cold tier. Capabilities correctly declare uses_graph=True, uses_temporal=True, but can_handle only checks backends.graph is not None. There is no cold-tier check because BackendSet has no temporal/cold field (see finding 3.1). The strategy reports confidence 0.5 when only a graph backend exists, even if no cold-tier data is available.

1.3 [MEDIUM] plan-decision-context can_handle unconditionally returns 0.7

File: src/cleveragents/domain/models/acms/strategy_stubs.py:347-353

def can_handle(self, request, backends) -> float:
    return self.capabilities.quality_score   # always 0.7

Spec §43197-43199 says this strategy "operates on warm/cold tiers", yet can_handle never inspects backend availability at all. Every other strategy properly degrades to 0.0 when its required backends are missing; this one does not. The comment says "reads from warm/cold tiers" but there is no tier-availability check.


2. SPEC DEVIATIONS

2.1 [MEDIUM] Default enabled list — spec internal conflict

Spec §25223 lists 4 strategies in the default enabled config:

["simple-keyword", "semantic-embedding", "arce", "breadth-depth-navigator"]

Spec §28682 (config key table) lists 3:

["simple-keyword", "semantic-embedding", "breadth-depth-navigator"]

The implementation follows §28682 (3 strategies, arce excluded). The spec has an internal inconsistency — this should be clarified with the spec author and the decision documented.

2.2 [MEDIUM] Custom strategy module path format diverges from spec

Spec §25226 shows custom strategy registration using dot notation:

"my-domain-strategy" = "my_package.strategies.DomainStrategy"

The implementation's register_from_module requires colon notation:

"my_package.strategies:DomainStrategy"

These formats are incompatible. If a user follows the spec's TOML example, register_from_module will reject it. Note the spec's own pipeline component format at §25232 uses colons ("my_extensions.scorers:DomainAwareScorer"), so the spec itself is inconsistent — but the strategy registration example uses dots.


3. DESIGN GAPS

3.1 [MEDIUM] BackendSet lacks temporal/cold-tier field

File: src/cleveragents/domain/models/acms/strategy.py:35-58

BackendSet models text, vector, and graph but has no field for temporal/cold-tier backends. Two of six strategies (temporal-archaeology, plan-decision-context) declare uses_temporal=True in capabilities, but there is no way to verify temporal backend availability. This is the root cause of bugs 1.2 and 1.3.

3.2 [LOW] update_config() doesn't expose resource_types or extra

File: src/cleveragents/application/services/strategy_registry.py:324-374

StrategyConfig has resource_types and extra fields, but update_config() only accepts enabled, timeout_seconds, max_fragments, max_workers, and circuit_breaker_threshold. There is no way to update those fields through the public API.


4. SECURITY

4.1 [MEDIUM] register_from_module — arbitrary code execution via dynamic import (CWE-706)

File: src/cleveragents/application/services/strategy_registry.py:186

Semgrep flagged importlib.import_module(module_name) as CWE-706. Module-level code runs on import, __init__ runs on instantiation — both are arbitrary code execution vectors. The docstring has a security note, which is good, but there is no enforcement: no allowlist of permitted module prefixes, no namespace restriction. Consider adding a configurable allowlist or restricting to known package namespaces.

4.2 [LOW] Mutable dicts on frozen Pydantic models violate ADR-004

Files: strategy.py:244 (StrategyConfig.extra), strategy.py:293 (ContextStrategyResult.stats)

Both are dict[str, Any] on frozen=True models. Pydantic's frozen prevents attribute reassignment but not dict mutation (config.extra["key"] = "value" succeeds). If ADR-004 immutability is a hard requirement, these should use types.MappingProxyType or a frozen representation.


5. PERFORMANCE

5.1 [MEDIUM] capabilities property creates a new Pydantic model on every access

File: src/cleveragents/domain/models/acms/strategy_stubs.py (all 6 classes)

Every stub constructs a fresh StrategyCapabilities Pydantic model on each .capabilities access:

@property
def capabilities(self) -> StrategyCapabilities:
    return StrategyCapabilities(uses_text=True, ...)  # new allocation every call

This property is also called inside can_handle() (self.capabilities.quality_score), so every can_handle invocation triggers Pydantic model construction + validation. Spec target is < 20ms for all strategy can_handle checks combined (§43253). With 6 strategies, that is 12+ Pydantic allocations per polling cycle.

Fix: Use functools.cached_property or store as a class-level constant.


6. THREAD SAFETY

6.1 [MEDIUM] StrategyRegistry has no synchronization for concurrent access

File: src/cleveragents/application/services/strategy_registry.py:77-81

The registry uses plain dict and list for internal state with no locking. Other services in the same package use threading.Lock (semantic_validation_service.py:210) or threading.RLock (autonomy_guardrail_service.py:59). The spec describes strategies being executed in parallel (§42636). If the registry is a shared singleton, concurrent reads during parallel strategy execution could race with configuration updates.


7. TEST QUALITY

7.1 [LOW] Tests access private _enabled_order attribute

File: features/steps/context_strategy_registry_steps.py (step_given_stale_enabled)

context.registry._enabled_order.append("ghost-strategy")

This couples the test to the internal data structure. If the enabled-order tracking changes, this test breaks silently. A public test-helper method or factory would be more robust.

7.2 [LOW] No test for register(non_protocol_obj) without explicit name

Bug 1.1 has no test coverage. step_when_register_non_protocol always passes name="fake", so the strategy.name access on line 113 is never exercised with a non-protocol object that lacks .name.

7.3 [LOW] No concurrent access tests

Given finding 6.1, there are no tests exercising the registry under concurrent reads/writes.


Summary Table

# Severity Category Finding
1.1 HIGH Bug register() raises AttributeError instead of StrategyRegistrationError
1.2 MEDIUM Bug temporal-archaeology doesn't verify cold-tier availability
1.3 MEDIUM Bug plan-decision-context never degrades — returns 0.7 unconditionally
2.1 MEDIUM Spec Default enabled list deviates from spec §25223 (missing arce)
2.2 MEDIUM Spec Custom strategy path format (colon) differs from spec §25226 (dot)
3.1 MEDIUM Design BackendSet missing temporal/cold field
3.2 LOW Design update_config() doesn't expose resource_types/extra
4.1 MEDIUM Security register_from_module dynamic import with no allowlist (CWE-706)
4.2 LOW Security Mutable dicts on frozen models violate ADR-004
5.1 MEDIUM Perf capabilities creates new Pydantic model on every access
6.1 MEDIUM Thread No synchronization on shared registry state
7.1 LOW Tests Test accesses private _enabled_order
7.2 LOW Tests Missing test for bug 1.1
7.3 LOW Tests No concurrent access tests

The highest-priority items are bug 1.1 (wrong exception type — easy fix), perf 5.1 (easy fix with cached_property), and the spec deviations 2.1/2.2 (need spec-author clarification).

# Code Review — Commit `98c1fa31` (feat(acms): add context strategy registry) **Reviewer scope:** Bug detection, security, performance, thread safety, test quality, and spec conformance against issue #191 and `docs/specification.md`. Overall the implementation is well-structured: the domain models, protocol design, and 55 BDD scenarios are thorough. However, I found several issues worth addressing before merge. --- ## 1. BUGS ### 1.1 [HIGH] `register()` can raise `AttributeError` instead of `StrategyRegistrationError` **File:** `src/cleveragents/application/services/strategy_registry.py:113` When `name=None` (the default), the code accesses `strategy.name` **before** the protocol check at line 121: ```python name = name or strategy.name # line 113 — AttributeError if not a protocol object # ... if not isinstance(strategy, ContextStrategy): # line 121 — never reached ``` If a caller passes a non-protocol object without an explicit `name`, this raises a raw `AttributeError` rather than the intended `StrategyRegistrationError`. The existing test sidesteps this by always passing `name="fake"`. **Fix:** move the `isinstance` check before the name resolution, or wrap `strategy.name` access in a try/except. ### 1.2 [MEDIUM] `temporal-archaeology` `can_handle` only checks graph, not cold-tier **File:** `src/cleveragents/domain/models/acms/strategy_stubs.py:291-298` Spec §25215 / §43193-43195 says this strategy requires **Graph + Cold tier**. Capabilities correctly declare `uses_graph=True, uses_temporal=True`, but `can_handle` only checks `backends.graph is not None`. There is no cold-tier check because `BackendSet` has no temporal/cold field (see finding 3.1). The strategy reports confidence 0.5 when only a graph backend exists, even if no cold-tier data is available. ### 1.3 [MEDIUM] `plan-decision-context` `can_handle` unconditionally returns 0.7 **File:** `src/cleveragents/domain/models/acms/strategy_stubs.py:347-353` ```python def can_handle(self, request, backends) -> float: return self.capabilities.quality_score # always 0.7 ``` Spec §43197-43199 says this strategy "operates on warm/cold tiers", yet `can_handle` never inspects backend availability at all. Every other strategy properly degrades to 0.0 when its required backends are missing; this one does not. The comment says "reads from warm/cold tiers" but there is no tier-availability check. --- ## 2. SPEC DEVIATIONS ### 2.1 [MEDIUM] Default enabled list — spec internal conflict **Spec §25223** lists **4** strategies in the default enabled config: ``` ["simple-keyword", "semantic-embedding", "arce", "breadth-depth-navigator"] ``` **Spec §28682** (config key table) lists **3**: ``` ["simple-keyword", "semantic-embedding", "breadth-depth-navigator"] ``` The implementation follows §28682 (3 strategies, `arce` excluded). The spec has an internal inconsistency — this should be clarified with the spec author and the decision documented. ### 2.2 [MEDIUM] Custom strategy module path format diverges from spec **Spec §25226** shows custom strategy registration using **dot notation**: ```toml "my-domain-strategy" = "my_package.strategies.DomainStrategy" ``` The implementation's `register_from_module` requires **colon notation**: ``` "my_package.strategies:DomainStrategy" ``` These formats are incompatible. If a user follows the spec's TOML example, `register_from_module` will reject it. Note the spec's own pipeline component format at §25232 uses colons (`"my_extensions.scorers:DomainAwareScorer"`), so the spec itself is inconsistent — but the strategy registration example uses dots. --- ## 3. DESIGN GAPS ### 3.1 [MEDIUM] `BackendSet` lacks temporal/cold-tier field **File:** `src/cleveragents/domain/models/acms/strategy.py:35-58` `BackendSet` models `text`, `vector`, and `graph` but has no field for temporal/cold-tier backends. Two of six strategies (`temporal-archaeology`, `plan-decision-context`) declare `uses_temporal=True` in capabilities, but there is no way to verify temporal backend availability. This is the root cause of bugs 1.2 and 1.3. ### 3.2 [LOW] `update_config()` doesn't expose `resource_types` or `extra` **File:** `src/cleveragents/application/services/strategy_registry.py:324-374` `StrategyConfig` has `resource_types` and `extra` fields, but `update_config()` only accepts `enabled`, `timeout_seconds`, `max_fragments`, `max_workers`, and `circuit_breaker_threshold`. There is no way to update those fields through the public API. --- ## 4. SECURITY ### 4.1 [MEDIUM] `register_from_module` — arbitrary code execution via dynamic import (CWE-706) **File:** `src/cleveragents/application/services/strategy_registry.py:186` Semgrep flagged `importlib.import_module(module_name)` as CWE-706. Module-level code runs on import, `__init__` runs on instantiation — both are arbitrary code execution vectors. The docstring has a security note, which is good, but there is **no enforcement**: no allowlist of permitted module prefixes, no namespace restriction. Consider adding a configurable allowlist or restricting to known package namespaces. ### 4.2 [LOW] Mutable dicts on frozen Pydantic models violate ADR-004 **Files:** `strategy.py:244` (`StrategyConfig.extra`), `strategy.py:293` (`ContextStrategyResult.stats`) Both are `dict[str, Any]` on `frozen=True` models. Pydantic's `frozen` prevents attribute reassignment but **not** dict mutation (`config.extra["key"] = "value"` succeeds). If ADR-004 immutability is a hard requirement, these should use `types.MappingProxyType` or a frozen representation. --- ## 5. PERFORMANCE ### 5.1 [MEDIUM] `capabilities` property creates a new Pydantic model on every access **File:** `src/cleveragents/domain/models/acms/strategy_stubs.py` (all 6 classes) Every stub constructs a fresh `StrategyCapabilities` Pydantic model on each `.capabilities` access: ```python @property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities(uses_text=True, ...) # new allocation every call ``` This property is also called inside `can_handle()` (`self.capabilities.quality_score`), so every `can_handle` invocation triggers Pydantic model construction + validation. Spec target is < 20ms for **all** strategy `can_handle` checks combined (§43253). With 6 strategies, that is 12+ Pydantic allocations per polling cycle. **Fix:** Use `functools.cached_property` or store as a class-level constant. --- ## 6. THREAD SAFETY ### 6.1 [MEDIUM] `StrategyRegistry` has no synchronization for concurrent access **File:** `src/cleveragents/application/services/strategy_registry.py:77-81` The registry uses plain `dict` and `list` for internal state with no locking. Other services in the same package use `threading.Lock` (`semantic_validation_service.py:210`) or `threading.RLock` (`autonomy_guardrail_service.py:59`). The spec describes strategies being executed in parallel (§42636). If the registry is a shared singleton, concurrent reads during parallel strategy execution could race with configuration updates. --- ## 7. TEST QUALITY ### 7.1 [LOW] Tests access private `_enabled_order` attribute **File:** `features/steps/context_strategy_registry_steps.py` (`step_given_stale_enabled`) ```python context.registry._enabled_order.append("ghost-strategy") ``` This couples the test to the internal data structure. If the enabled-order tracking changes, this test breaks silently. A public test-helper method or factory would be more robust. ### 7.2 [LOW] No test for `register(non_protocol_obj)` without explicit name Bug 1.1 has no test coverage. `step_when_register_non_protocol` always passes `name="fake"`, so the `strategy.name` access on line 113 is never exercised with a non-protocol object that lacks `.name`. ### 7.3 [LOW] No concurrent access tests Given finding 6.1, there are no tests exercising the registry under concurrent reads/writes. --- ## Summary Table | # | Severity | Category | Finding | |---|----------|----------|---------| | 1.1 | **HIGH** | Bug | `register()` raises `AttributeError` instead of `StrategyRegistrationError` | | 1.2 | MEDIUM | Bug | `temporal-archaeology` doesn't verify cold-tier availability | | 1.3 | MEDIUM | Bug | `plan-decision-context` never degrades — returns 0.7 unconditionally | | 2.1 | MEDIUM | Spec | Default enabled list deviates from spec §25223 (missing `arce`) | | 2.2 | MEDIUM | Spec | Custom strategy path format (colon) differs from spec §25226 (dot) | | 3.1 | MEDIUM | Design | `BackendSet` missing temporal/cold field | | 3.2 | LOW | Design | `update_config()` doesn't expose `resource_types`/`extra` | | 4.1 | MEDIUM | Security | `register_from_module` dynamic import with no allowlist (CWE-706) | | 4.2 | LOW | Security | Mutable dicts on frozen models violate ADR-004 | | 5.1 | MEDIUM | Perf | `capabilities` creates new Pydantic model on every access | | 6.1 | MEDIUM | Thread | No synchronization on shared registry state | | 7.1 | LOW | Tests | Test accesses private `_enabled_order` | | 7.2 | LOW | Tests | Missing test for bug 1.1 | | 7.3 | LOW | Tests | No concurrent access tests | The highest-priority items are **bug 1.1** (wrong exception type — easy fix), **perf 5.1** (easy fix with `cached_property`), and the **spec deviations 2.1/2.2** (need spec-author clarification).
@ -0,0 +212,4 @@
def assemble(
self,
request: ContextRequest,
Member

[TEST - LOW] Directly accessing the private _enabled_order attribute couples this test to the internal implementation. If the enabled-order tracking mechanism changes, this test breaks silently. Consider adding a public test-helper method on StrategyRegistry (e.g., _inject_stale_entry_for_test()) or a factory that produces a registry in this state.

**[TEST - LOW]** Directly accessing the private `_enabled_order` attribute couples this test to the internal implementation. If the enabled-order tracking mechanism changes, this test breaks silently. Consider adding a public test-helper method on `StrategyRegistry` (e.g., `_inject_stale_entry_for_test()`) or a factory that produces a registry in this state.
@ -0,0 +76,4 @@
def __init__(self) -> None:
"""Initialize an empty registry."""
self._strategies: dict[str, ContextStrategy] = {}
Member

[THREAD SAFETY - MEDIUM] These three plain dict/list fields have no synchronization. Other services in the same package (e.g., autonomy_guardrail_service.py, semantic_validation_service.py) use threading.Lock or threading.RLock. Since the spec describes parallel strategy execution (§42636), this registry will likely be a shared singleton accessed from multiple threads. Consider adding a threading.RLock to protect mutation methods (register, unregister, set_enabled, update_config, clear).

**[THREAD SAFETY - MEDIUM]** These three plain `dict`/`list` fields have no synchronization. Other services in the same package (e.g., `autonomy_guardrail_service.py`, `semantic_validation_service.py`) use `threading.Lock` or `threading.RLock`. Since the spec describes parallel strategy execution (§42636), this registry will likely be a shared singleton accessed from multiple threads. Consider adding a `threading.RLock` to protect mutation methods (`register`, `unregister`, `set_enabled`, `update_config`, `clear`).
@ -0,0 +110,4 @@
already registered, or the strategy doesn't satisfy
the ``ContextStrategy`` protocol.
"""
name = name or strategy.name
Member

[BUG - HIGH] When name=None (default) and the passed object is not a ContextStrategy, this line accesses strategy.name before the protocol check at line 121. If the object lacks a .name attribute, this raises AttributeError instead of StrategyRegistrationError.

Fix: Move the isinstance(strategy, ContextStrategy) check before this line, or wrap the strategy.name access in a try/except that converts to StrategyRegistrationError.

**[BUG - HIGH]** When `name=None` (default) and the passed object is not a `ContextStrategy`, this line accesses `strategy.name` before the protocol check at line 121. If the object lacks a `.name` attribute, this raises `AttributeError` instead of `StrategyRegistrationError`. **Fix:** Move the `isinstance(strategy, ContextStrategy)` check before this line, or wrap the `strategy.name` access in a try/except that converts to `StrategyRegistrationError`.
@ -0,0 +183,4 @@
module_name, class_name = module_path.rsplit(":", 1)
try:
module = importlib.import_module(module_name)
Member

[SECURITY - MEDIUM] CWE-706: importlib.import_module() with a runtime-determined string executes arbitrary module-level code on import. The docstring warning is good, but there is no enforcement — no allowlist of permitted module prefixes or namespace restriction.

Consider adding a configurable allowlist (e.g., only cleveragents.* modules or explicitly configured trusted namespaces) to prevent loading untrusted code from user-supplied TOML configs.

**[SECURITY - MEDIUM] CWE-706:** `importlib.import_module()` with a runtime-determined string executes arbitrary module-level code on import. The docstring warning is good, but there is no enforcement — no allowlist of permitted module prefixes or namespace restriction. Consider adding a configurable allowlist (e.g., only `cleveragents.*` modules or explicitly configured trusted namespaces) to prevent loading untrusted code from user-supplied TOML configs.
@ -0,0 +32,4 @@
# ---------------------------------------------------------------------------
class BackendSet(BaseModel, frozen=True):
Member

[DESIGN - MEDIUM] BackendSet only models text, vector, and graph. There is no field for temporal/cold-tier backends. Two of six strategies (temporal-archaeology, plan-decision-context) declare uses_temporal=True in capabilities but have no way to verify temporal backend availability via this model. This is the root cause of the can_handle bugs in those two strategies. Consider adding a temporal or cold backend field.

**[DESIGN - MEDIUM]** `BackendSet` only models `text`, `vector`, and `graph`. There is no field for temporal/cold-tier backends. Two of six strategies (`temporal-archaeology`, `plan-decision-context`) declare `uses_temporal=True` in capabilities but have no way to verify temporal backend availability via this model. This is the root cause of the `can_handle` bugs in those two strategies. Consider adding a `temporal` or `cold` backend field.
@ -0,0 +241,4 @@
default=(),
description="Resource types this strategy is limited to (empty = all)",
)
extra: dict[str, Any] = Field(
Member

[SECURITY - LOW / ADR-004] extra: dict[str, Any] on a frozen=True model can still be mutated via dict operations (config.extra["key"] = "value" succeeds). Pydantic's frozen only prevents attribute reassignment, not deep mutation. Same issue exists for ContextStrategyResult.stats at line 293. If ADR-004 immutability is a hard requirement, consider types.MappingProxyType or a frozen representation.

**[SECURITY - LOW / ADR-004]** `extra: dict[str, Any]` on a `frozen=True` model can still be mutated via dict operations (`config.extra["key"] = "value"` succeeds). Pydantic's `frozen` only prevents attribute reassignment, not deep mutation. Same issue exists for `ContextStrategyResult.stats` at line 293. If ADR-004 immutability is a hard requirement, consider `types.MappingProxyType` or a frozen representation.
@ -0,0 +52,4 @@
return "simple-keyword"
@property
def capabilities(self) -> StrategyCapabilities:
Member

[PERF - MEDIUM] This constructs a new StrategyCapabilities Pydantic model on every property access. Since can_handle() reads self.capabilities.quality_score, each can_handle call allocates and validates a Pydantic instance. With 6 strategies and a spec target of < 20ms total for all can_handle checks (§43253), this is wasteful.

Fix: Use functools.cached_property to compute once and cache:

@functools.cached_property
def capabilities(self) -> StrategyCapabilities:
    return StrategyCapabilities(...)

This applies to all 6 stub strategy classes.

**[PERF - MEDIUM]** This constructs a new `StrategyCapabilities` Pydantic model on every property access. Since `can_handle()` reads `self.capabilities.quality_score`, each `can_handle` call allocates and validates a Pydantic instance. With 6 strategies and a spec target of < 20ms total for all `can_handle` checks (§43253), this is wasteful. **Fix:** Use `functools.cached_property` to compute once and cache: ```python @functools.cached_property def capabilities(self) -> StrategyCapabilities: return StrategyCapabilities(...) ``` This applies to all 6 stub strategy classes.
@ -0,0 +293,4 @@
request: ContextRequest,
backends: BackendSet,
) -> float:
if backends.graph is not None:
Member

[BUG - MEDIUM] Spec §25215 / §43193-43195 says temporal-archaeology requires Graph + Cold tier. Capabilities correctly declare uses_temporal=True, but can_handle only checks backends.graph. The root cause is that BackendSet has no temporal/cold field — but this means the strategy reports 0.5 confidence even when cold-tier data is unavailable, which could lead to it being selected and then failing at assembly time.

**[BUG - MEDIUM]** Spec §25215 / §43193-43195 says `temporal-archaeology` requires **Graph + Cold tier**. Capabilities correctly declare `uses_temporal=True`, but `can_handle` only checks `backends.graph`. The root cause is that `BackendSet` has no temporal/cold field — but this means the strategy reports 0.5 confidence even when cold-tier data is unavailable, which could lead to it being selected and then failing at assembly time.
@ -0,0 +350,4 @@
backends: BackendSet,
) -> float:
# Works even without graph; reads from warm/cold tiers.
return self.capabilities.quality_score
Member

[BUG - MEDIUM] This unconditionally returns 0.7 regardless of backend/tier availability. Spec §43197-43199 says this strategy "operates on warm/cold tiers", yet it never checks whether those tiers are available. Every other strategy degrades to 0.0 when its required backends are missing; this one does not. This could cause the strategy to be selected when it has no data source to read from.

**[BUG - MEDIUM]** This unconditionally returns 0.7 regardless of backend/tier availability. Spec §43197-43199 says this strategy "operates on warm/cold tiers", yet it never checks whether those tiers are available. Every other strategy degrades to 0.0 when its required backends are missing; this one does not. This could cause the strategy to be selected when it has no data source to read from.
hamza.khyari force-pushed feature/m6-acms-strategy-registry from 98c1fa3125
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 13s
CI / build (pull_request) Successful in 15s
CI / quality (pull_request) Successful in 18s
CI / security (pull_request) Successful in 31s
CI / typecheck (pull_request) Successful in 33s
CI / unit_tests (pull_request) Successful in 1m56s
CI / docker (pull_request) Successful in 40s
CI / integration_tests (pull_request) Successful in 3m6s
CI / coverage (pull_request) Successful in 4m49s
CI / benchmark-regression (pull_request) Successful in 25m44s
to 09e3c18aba
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 18s
CI / security (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 2m6s
CI / docker (pull_request) Successful in 44s
CI / integration_tests (pull_request) Successful in 3m3s
CI / coverage (pull_request) Successful in 4m37s
CI / benchmark-regression (pull_request) Successful in 28m35s
2026-03-05 19:57:02 +00:00
Compare
hamza.khyari force-pushed feature/m6-acms-strategy-registry from 09e3c18aba
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 18s
CI / security (pull_request) Successful in 33s
CI / typecheck (pull_request) Successful in 36s
CI / unit_tests (pull_request) Successful in 2m6s
CI / docker (pull_request) Successful in 44s
CI / integration_tests (pull_request) Successful in 3m3s
CI / coverage (pull_request) Successful in 4m37s
CI / benchmark-regression (pull_request) Successful in 28m35s
to cf622c8cd6
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 21s
CI / typecheck (pull_request) Successful in 35s
CI / security (pull_request) Successful in 38s
CI / unit_tests (pull_request) Successful in 2m27s
CI / docker (pull_request) Successful in 43s
CI / integration_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 4m33s
CI / benchmark-regression (pull_request) Successful in 29m8s
2026-03-05 20:35:22 +00:00
Compare
Author
Member

Review Response — All 14 Findings Addressed

Thanks for the thorough review, @CoreRasurae. All findings have been addressed in commit cf622c8c. Here's the breakdown:


1. BUGS

1.1 [HIGH] register() guard ordering — Fixed. Moved the isinstance(strategy, ContextStrategy) check before strategy.name access. Uses type(strategy).__name__ as fallback label in the error message when the object has no .name attribute.

1.2 [MEDIUM] temporal-archaeology can_handle missing cold-tier check — Fixed. can_handle now checks backends.graph is not None and backends.temporal is not None, returning 0.0 if either is absent. See also 3.1 below for the BackendSet.temporal field.

1.3 [MEDIUM] plan-decision-context unconditional 0.7 — Fixed. can_handle now checks backends.temporal is not None, returning 0.0 otherwise. Added regression tests for both the degraded (0.0) and happy (0.7) paths.


2. SPEC DEVIATIONS

2.1 [MEDIUM] Default enabled list conflict (§25223 vs §28682) — Documented. Added a comment in strategy_stubs.py explaining the contradiction and the rationale for following §28682 (3 strategies). ARCE requires all 3 backends, so excluding it from defaults is defensible for minimal deployments.

2.2 [MEDIUM] Colon vs dot notation — Documented. Added a Note: section in the register_from_module docstring explaining that §25226 (dot) and §25232 (colon) are inconsistent within the spec, and we chose colon notation because it unambiguously separates module from class name, matching Python entry-point conventions.


3. DESIGN GAPS

3.1 [MEDIUM] BackendSet missing temporal field — Fixed. Added temporal: object | None = Field(default=None) to BackendSet. This unblocks the can_handle checks in 1.2 and 1.3. Typed as object | None as a placeholder until a TemporalBackend protocol is defined.

3.2 [LOW] update_config() missing resource_types/extra — Fixed. Added both parameters to update_config(). New BDD scenarios verify that resource_types is persisted correctly and that extra is wrapped in MappingProxyType for immutability.


4. SECURITY

4.1 [MEDIUM] No import allowlist (CWE-706) — Fixed. Added allowed_module_prefixes constructor parameter (default ("cleveragents.",)) with a check before importlib.import_module(). Module paths not matching any allowed prefix raise StrategyRegistrationError with a clear message.

4.2 [LOW] Mutable dicts on frozen models — Fixed. StrategyConfig.extra and ContextStrategyResult.stats changed from dict[str, Any] to MappingProxyType[str, Any], with field_validator(mode="before") to coerce incoming plain dicts. model_config = ConfigDict(arbitrary_types_allowed=True) added to both models.


5. PERFORMANCE

5.1 [MEDIUM] capabilities property allocates on every access — Fixed. All 6 stubs changed from @property to @functools.cached_property. The Pydantic model is constructed once per instance and cached in __dict__ thereafter.


6. THREAD SAFETY

6.1 [MEDIUM] No synchronization — Fixed. Added threading.RLock to StrategyRegistry.__init__. All mutation methods (register, set_enabled, update_config, unregister, clear) and all query methods (get, get_entry, list_all, list_enabled, list_builtin, validate_registry, is_registered, __contains__, __len__) are wrapped with with self._lock:. RLock chosen over Lock to support nested calls (e.g., update_configget_entry).


7. TEST QUALITY

7.1 [LOW] Private _enabled_order access — Fixed. Added inject_stale_enabled_entry() as a public test helper on StrategyRegistry. The step now uses the public helper instead of reaching into internals. Added to vulture_whitelist.py.

7.2 [LOW] Missing test for bug 1.1 — Fixed. New scenario: "Reject non-protocol object without explicit name" exercises register(object()) without a name parameter, asserting StrategyRegistrationError.

7.3 [LOW] No concurrent tests — Fixed. New scenario: "Concurrent register and list_enabled do not raise" uses 10 threads with a Barrier for synchronized start. Post-condition assertions verify the registry contains all expected strategies (6 builtins + 4 thread-registered) and that every thread-registered name is present.


Additional Changes (self-review)

During self-review, found and fixed 3 additional issues:

  • is_registered(), __contains__(), and __len__() were reading self._strategies without the lock — now all three acquire self._lock
  • Added MappingProxyType field validator tests (plain dict coercion, non-dict rejection)
  • Added update_config tests for the new resource_types and extra parameters

Test count: 72 scenarios, 0 failed. Lint, typecheck, and vulture all pass.

## Review Response — All 14 Findings Addressed Thanks for the thorough review, @CoreRasurae. All findings have been addressed in commit `cf622c8c`. Here's the breakdown: --- ### 1. BUGS **1.1 [HIGH] `register()` guard ordering** — Fixed. Moved the `isinstance(strategy, ContextStrategy)` check before `strategy.name` access. Uses `type(strategy).__name__` as fallback label in the error message when the object has no `.name` attribute. **1.2 [MEDIUM] `temporal-archaeology` `can_handle` missing cold-tier check** — Fixed. `can_handle` now checks `backends.graph is not None and backends.temporal is not None`, returning 0.0 if either is absent. See also 3.1 below for the `BackendSet.temporal` field. **1.3 [MEDIUM] `plan-decision-context` unconditional 0.7** — Fixed. `can_handle` now checks `backends.temporal is not None`, returning 0.0 otherwise. Added regression tests for both the degraded (0.0) and happy (0.7) paths. --- ### 2. SPEC DEVIATIONS **2.1 [MEDIUM] Default enabled list conflict (§25223 vs §28682)** — Documented. Added a comment in `strategy_stubs.py` explaining the contradiction and the rationale for following §28682 (3 strategies). ARCE requires all 3 backends, so excluding it from defaults is defensible for minimal deployments. **2.2 [MEDIUM] Colon vs dot notation** — Documented. Added a `Note:` section in the `register_from_module` docstring explaining that §25226 (dot) and §25232 (colon) are inconsistent within the spec, and we chose colon notation because it unambiguously separates module from class name, matching Python entry-point conventions. --- ### 3. DESIGN GAPS **3.1 [MEDIUM] `BackendSet` missing temporal field** — Fixed. Added `temporal: object | None = Field(default=None)` to `BackendSet`. This unblocks the `can_handle` checks in 1.2 and 1.3. Typed as `object | None` as a placeholder until a `TemporalBackend` protocol is defined. **3.2 [LOW] `update_config()` missing `resource_types`/`extra`** — Fixed. Added both parameters to `update_config()`. New BDD scenarios verify that `resource_types` is persisted correctly and that `extra` is wrapped in `MappingProxyType` for immutability. --- ### 4. SECURITY **4.1 [MEDIUM] No import allowlist (CWE-706)** — Fixed. Added `allowed_module_prefixes` constructor parameter (default `("cleveragents.",)`) with a check before `importlib.import_module()`. Module paths not matching any allowed prefix raise `StrategyRegistrationError` with a clear message. **4.2 [LOW] Mutable dicts on frozen models** — Fixed. `StrategyConfig.extra` and `ContextStrategyResult.stats` changed from `dict[str, Any]` to `MappingProxyType[str, Any]`, with `field_validator(mode="before")` to coerce incoming plain dicts. `model_config = ConfigDict(arbitrary_types_allowed=True)` added to both models. --- ### 5. PERFORMANCE **5.1 [MEDIUM] `capabilities` property allocates on every access** — Fixed. All 6 stubs changed from `@property` to `@functools.cached_property`. The Pydantic model is constructed once per instance and cached in `__dict__` thereafter. --- ### 6. THREAD SAFETY **6.1 [MEDIUM] No synchronization** — Fixed. Added `threading.RLock` to `StrategyRegistry.__init__`. All mutation methods (`register`, `set_enabled`, `update_config`, `unregister`, `clear`) and all query methods (`get`, `get_entry`, `list_all`, `list_enabled`, `list_builtin`, `validate_registry`, `is_registered`, `__contains__`, `__len__`) are wrapped with `with self._lock:`. RLock chosen over Lock to support nested calls (e.g., `update_config` → `get_entry`). --- ### 7. TEST QUALITY **7.1 [LOW] Private `_enabled_order` access** — Fixed. Added `inject_stale_enabled_entry()` as a public test helper on `StrategyRegistry`. The step now uses the public helper instead of reaching into internals. Added to `vulture_whitelist.py`. **7.2 [LOW] Missing test for bug 1.1** — Fixed. New scenario: "Reject non-protocol object without explicit name" exercises `register(object())` without a name parameter, asserting `StrategyRegistrationError`. **7.3 [LOW] No concurrent tests** — Fixed. New scenario: "Concurrent register and list_enabled do not raise" uses 10 threads with a `Barrier` for synchronized start. Post-condition assertions verify the registry contains all expected strategies (6 builtins + 4 thread-registered) and that every thread-registered name is present. --- ### Additional Changes (self-review) During self-review, found and fixed 3 additional issues: - `is_registered()`, `__contains__()`, and `__len__()` were reading `self._strategies` without the lock — now all three acquire `self._lock` - Added `MappingProxyType` field validator tests (plain dict coercion, non-dict rejection) - Added `update_config` tests for the new `resource_types` and `extra` parameters **Test count:** 72 scenarios, 0 failed. Lint, typecheck, and vulture all pass.
brent.edwards left a comment

Review: feat(acms): add context strategy registry with plugin discovery

Reviewed per docs/development/review_playbook.md. Routing: domain models & services → primary Luis, secondary Jeff.

Luis's prior REQUEST_CHANGES review appears fully addressed — all 14 findings have been resolved. Nice work on the CWE-706 allowlist for dynamic imports and the clean separation between ContextStrategy protocol and StrategyRegistryEntry metadata.


P1:must-fix — set_enabled() does not reject duplicate names

set_enabled() accepts names: list[str] and assigns self._enabled_order = list(names) directly. If the caller passes ["a", "b", "a"], the enabled_set dedup for config updates works correctly, but _enabled_order preserves duplicates. This violates Invariant 2 documented in the class body (set(_enabled_order) is a subset of _strategies.keys() — the invariant implies it should behave like a set). Downstream code in list_enabled() would return ["a", "b", "a"], causing duplicate strategy execution.

Fix: Either deduplicate while preserving order (list(dict.fromkeys(names))) or raise on duplicates. Dedup-with-warning is probably safest for config-driven callers.

File: src/cleveragents/application/services/strategy_registry.py, set_enabled() method, near the self._enabled_order = list(names) assignment.


P2:should-fix — PR body claims "40 BDD scenarios" but feature file has 55+

The PR description says "40 BDD scenarios" but features/context_strategy_registry.feature contains 55+ scenarios. The count is stale from an earlier draft. Not a code issue, but misleading for reviewers.

P2:should-fix — Missing __all__ exports in strategy.py and strategy_stubs.py

The domain model modules strategy.py and strategy_stubs.py don't declare __all__. While strategy_registry.py re-exports the key types, explicit __all__ in each module is the project convention (per CONTRIBUTING.md § public API).


Process notes

  • Milestone mismatch: Issue #191 targets v3.4.0 but this PR targets v3.5.0. Please reconcile.
  • Priority label: Issue #191 is Priority/High but PR has Priority/Low. Should match.
  • Merge conflict: PR is currently not mergeable (mergeable: false). Rebase needed.
  • Stale review: Luis's prior REQUEST_CHANGES should be dismissed since all items are addressed.

Overall this is solid work — the registry design is clean, thread safety is well-handled, and the BDD coverage is thorough. The P1 duplicate-name issue is the only blocker.

## Review: feat(acms): add context strategy registry with plugin discovery Reviewed per `docs/development/review_playbook.md`. Routing: domain models & services → primary Luis, secondary Jeff. Luis's prior `REQUEST_CHANGES` review appears fully addressed — all 14 findings have been resolved. Nice work on the CWE-706 allowlist for dynamic imports and the clean separation between `ContextStrategy` protocol and `StrategyRegistryEntry` metadata. --- ### P1:must-fix — `set_enabled()` does not reject duplicate names `set_enabled()` accepts `names: list[str]` and assigns `self._enabled_order = list(names)` directly. If the caller passes `["a", "b", "a"]`, the `enabled_set` dedup for config updates works correctly, but `_enabled_order` preserves duplicates. This violates **Invariant 2** documented in the class body (`set(_enabled_order) is a subset of _strategies.keys()` — the invariant implies it should behave like a set). Downstream code in `list_enabled()` would return `["a", "b", "a"]`, causing duplicate strategy execution. **Fix**: Either deduplicate while preserving order (`list(dict.fromkeys(names))`) or raise on duplicates. Dedup-with-warning is probably safest for config-driven callers. **File**: `src/cleveragents/application/services/strategy_registry.py`, `set_enabled()` method, near the `self._enabled_order = list(names)` assignment. --- ### P2:should-fix — PR body claims "40 BDD scenarios" but feature file has 55+ The PR description says "40 BDD scenarios" but `features/context_strategy_registry.feature` contains 55+ scenarios. The count is stale from an earlier draft. Not a code issue, but misleading for reviewers. ### P2:should-fix — Missing `__all__` exports in `strategy.py` and `strategy_stubs.py` The domain model modules `strategy.py` and `strategy_stubs.py` don't declare `__all__`. While `strategy_registry.py` re-exports the key types, explicit `__all__` in each module is the project convention (per CONTRIBUTING.md § public API). --- ### Process notes - **Milestone mismatch**: Issue #191 targets v3.4.0 but this PR targets v3.5.0. Please reconcile. - **Priority label**: Issue #191 is `Priority/High` but PR has `Priority/Low`. Should match. - **Merge conflict**: PR is currently not mergeable (`mergeable: false`). Rebase needed. - **Stale review**: Luis's prior `REQUEST_CHANGES` should be dismissed since all items are addressed. --- Overall this is solid work — the registry design is clean, thread safety is well-handled, and the BDD coverage is thorough. The P1 duplicate-name issue is the only blocker.
@ -0,0 +237,4 @@
):
raise StrategyRegistrationError(
f"Module '{module_name}' is not in the allowed prefix list: "
f"{self._allowed_module_prefixes}. Only modules under these "
Member

P1:must-fixset_enabled() does not validate for duplicate names in the input list.

enabled_set = set(names) correctly deduplicates for the config-update loop, but self._enabled_order = list(names) preserves any duplicates from the caller. This violates Invariant 2 (set(_enabled_order) should behave like a proper set) and would cause list_enabled() to return duplicate entries, leading to duplicate strategy execution.

Fix: Either self._enabled_order = list(dict.fromkeys(names)) to deduplicate while preserving order, or raise ValueError on duplicate input. Deduplicate-with-warning is probably safest for config-driven callers.

**P1:must-fix** — `set_enabled()` does not validate for duplicate names in the input list. `enabled_set = set(names)` correctly deduplicates for the config-update loop, but `self._enabled_order = list(names)` preserves any duplicates from the caller. This violates Invariant 2 (`set(_enabled_order)` should behave like a proper set) and would cause `list_enabled()` to return duplicate entries, leading to duplicate strategy execution. **Fix**: Either `self._enabled_order = list(dict.fromkeys(names))` to deduplicate while preserving order, or raise `ValueError` on duplicate input. Deduplicate-with-warning is probably safest for config-driven callers.
hamza.khyari modified the milestone from v3.5.0 to v3.4.0 2026-03-05 21:53:18 +00:00
hamza.khyari force-pushed feature/m6-acms-strategy-registry from cf622c8cd6
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
CI / lint (pull_request) Successful in 16s
CI / build (pull_request) Successful in 16s
CI / quality (pull_request) Successful in 21s
CI / typecheck (pull_request) Successful in 35s
CI / security (pull_request) Successful in 38s
CI / unit_tests (pull_request) Successful in 2m27s
CI / docker (pull_request) Successful in 43s
CI / integration_tests (pull_request) Successful in 3m28s
CI / coverage (pull_request) Successful in 4m33s
CI / benchmark-regression (pull_request) Successful in 29m8s
to b7effcafc1
All checks were successful
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
2026-03-05 22:01:10 +00:00
Compare
Author
Member

Response to @brent.edwards Review

Thanks for the review, Brent. All items addressed:

P1: set_enabled() duplicate names — Fixed

Changed self._enabled_order = list(names) to self._enabled_order = list(dict.fromkeys(names)), which deduplicates while preserving insertion order. Added BDD scenario "Set enabled list deduplicates duplicate names" that passes ["simple-keyword", "arce", "simple-keyword"] and asserts the result is ["simple-keyword", "arce"].

P2: Stale scenario count in PR body — Fixed

Updated PR description from "40 BDD scenarios" to "73 BDD scenarios" and added mentions of thread safety tests, MappingProxyType validators, and other additions since the original draft.

P2: Missing __all__ — Fixed

Added __all__ to both strategy.py (7 public symbols) and strategy_stubs.py (8 public symbols, isort-sorted per RUF022).

Process notes

  • Merge conflict: Rebased onto latest master (fae438a7). Conflict in acms/__init__.py was from the analyzer plugin PR — resolved by keeping both sides. PR is now mergeable: true.
  • Milestone/priority mismatch: Already addressed — PR is on v3.4.0 milestone.
  • Luis's stale REQUEST_CHANGES: Dismissal requires repo admin. @CoreRasurae — could you re-review or dismiss your previous review since all 14 findings are addressed?

Test count: 73 scenarios, 0 failed. Lint, typecheck clean.

## Response to @brent.edwards Review Thanks for the review, Brent. All items addressed: ### P1: `set_enabled()` duplicate names — Fixed Changed `self._enabled_order = list(names)` to `self._enabled_order = list(dict.fromkeys(names))`, which deduplicates while preserving insertion order. Added BDD scenario "Set enabled list deduplicates duplicate names" that passes `["simple-keyword", "arce", "simple-keyword"]` and asserts the result is `["simple-keyword", "arce"]`. ### P2: Stale scenario count in PR body — Fixed Updated PR description from "40 BDD scenarios" to "73 BDD scenarios" and added mentions of thread safety tests, MappingProxyType validators, and other additions since the original draft. ### P2: Missing `__all__` — Fixed Added `__all__` to both `strategy.py` (7 public symbols) and `strategy_stubs.py` (8 public symbols, isort-sorted per RUF022). ### Process notes - **Merge conflict**: Rebased onto latest master (`fae438a7`). Conflict in `acms/__init__.py` was from the analyzer plugin PR — resolved by keeping both sides. PR is now `mergeable: true`. - **Milestone/priority mismatch**: Already addressed — PR is on v3.4.0 milestone. - **Luis's stale REQUEST_CHANGES**: Dismissal requires repo admin. @CoreRasurae — could you re-review or dismiss your previous review since all 14 findings are addressed? **Test count**: 73 scenarios, 0 failed. Lint, typecheck clean.
Member

Verification of @hamza.khyari Response (Round 2)

Checked out the branch locally and ran the full quality gate suite. Results below.

P1: set_enabled() deduplication — Verified

Confirmed strategy_registry.py now uses list(dict.fromkeys(names)). BDD scenario "Set enabled list deduplicates duplicate names" passes with the expected ["simple-keyword", "arce"] output.

P2: __all__ exports — Verified

  • strategy.py: 7 symbols in __all__
  • strategy_stubs.py: 8 symbols in __all__
  • ruff check --select RUF022 passes clean on both files.

P2: Scenario count — Minor discrepancy

PR body and response comment both state 73 BDD scenarios. Actual test run reports 72 scenarios passed, 0 failed (64 plain Scenarios + 2 Scenario Outlines expanding to 8 rows = 72). Off by one — likely a counting error in the PR description. Not blocking.

Quality gates

Session Result
nox -s lint PASS — All checks passed!
nox -s typecheck PASS — 0 errors, 0 warnings, 0 informations
nox -s unit_tests -- features/context_strategy_registry.feature PASS — 72 scenarios, 228 steps, 0 failures

Verdict

All code fixes from my original review are confirmed. The only remaining item is the 72 vs 73 scenario count in the PR body — please update the description to say "72" when convenient.

No blocking issues from my side. The stale REQUEST_CHANGES from @CoreRasurae is the only remaining gate for merge.

## Verification of @hamza.khyari Response (Round 2) Checked out the branch locally and ran the full quality gate suite. Results below. ### P1: `set_enabled()` deduplication — Verified Confirmed `strategy_registry.py` now uses `list(dict.fromkeys(names))`. BDD scenario "Set enabled list deduplicates duplicate names" passes with the expected `["simple-keyword", "arce"]` output. ### P2: `__all__` exports — Verified - `strategy.py`: 7 symbols in `__all__` - `strategy_stubs.py`: 8 symbols in `__all__` - `ruff check --select RUF022` passes clean on both files. ### P2: Scenario count — Minor discrepancy PR body and response comment both state **73 BDD scenarios**. Actual test run reports **72 scenarios passed, 0 failed** (64 plain Scenarios + 2 Scenario Outlines expanding to 8 rows = 72). Off by one — likely a counting error in the PR description. Not blocking. ### Quality gates | Session | Result | |---------|--------| | `nox -s lint` | PASS — `All checks passed!` | | `nox -s typecheck` | PASS — `0 errors, 0 warnings, 0 informations` | | `nox -s unit_tests -- features/context_strategy_registry.feature` | PASS — 72 scenarios, 228 steps, 0 failures | ### Verdict All code fixes from my original review are confirmed. The only remaining item is the **72 vs 73 scenario count** in the PR body — please update the description to say "72" when convenient. No blocking issues from my side. The stale `REQUEST_CHANGES` from @CoreRasurae is the only remaining gate for merge.
brent.edwards left a comment

Approved by Claude Opus.

Approved by Claude Opus.
hamza.khyari deleted branch feature/m6-acms-strategy-registry 2026-03-05 23:40:26 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
Reference
cleveragents/cleveragents-core!565
No description provided.