From 7b2cbeb36bd43615c63b7e773b226ec4205124cd Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 6 Jun 2026 02:15:44 -0400 Subject: [PATCH] fix(acms): rank simple-keyword by overlap, update phase3 tests to unified protocol SimpleKeywordStrategy.assemble now ranks by query-keyword overlap count first, then by backend relevance score. The previous behaviour sorted purely by backend score, producing incorrect ordering when a fragment with fewer keyword matches outscored one with more matches. Phase 3 integration helpers (ArceStrategy, TemporalArchaeologyStrategy, PlanDecisionContextStrategy) were exercising the deleted service-layer API: positional constructor args, single-dict can_handle, and fragment-list assemble. Rewrite them to use the unified domain-layer ContextStrategy protocol (ContextRequest + BackendSet + PlanContext on assemble/can_handle, no constructor args), aligned with the canonical helper in robot/helper_context_strategies.py. Apply ruff format to features/steps/acms_pipeline_phase3_steps.py and robot/helper_context_strategies.py so the CI lint gate is clean. ISSUES CLOSED: #5495 --- features/steps/acms_pipeline_phase3_steps.py | 11 +- robot/helper_acms_pipeline_phase3.py | 144 +++++++++++++----- robot/helper_context_strategies.py | 36 +++-- .../domain/models/acms/strategy_stubs.py | 30 ++-- 4 files changed, 148 insertions(+), 73 deletions(-) diff --git a/features/steps/acms_pipeline_phase3_steps.py b/features/steps/acms_pipeline_phase3_steps.py index 693fc1d54..d5a0aef77 100644 --- a/features/steps/acms_pipeline_phase3_steps.py +++ b/features/steps/acms_pipeline_phase3_steps.py @@ -223,9 +223,7 @@ def step_given_empty_strategy3(context: Context) -> None: context.strategy3_fragments = [] -@given( - "a strategy3 budget with max_tokens {max_t:d} and reserved_tokens {res_t:d}" -) +@given("a strategy3 budget with max_tokens {max_t:d} and reserved_tokens {res_t:d}") def step_given_strategy3_budget(context: Context, max_t: int, res_t: int) -> None: context.strategy3_budget = ContextBudget(max_tokens=max_t, reserved_tokens=res_t) @@ -360,8 +358,7 @@ def step_compressed_level(context: Context, level: str) -> None: for fragment in context.phase3_compressed } assert level in levels, ( - f"Expected level {level!r}, got " - f"{sorted(lv for lv in levels if lv is not None)}" + f"Expected level {level!r}, got {sorted(lv for lv in levels if lv is not None)}" ) @@ -702,9 +699,7 @@ def step_phase3_pipeline_has_preamble(context: Context) -> None: def step_phase3_pipeline_preamble_contains(context: Context, text: str) -> None: preamble = context.phase3_payload.preamble assert preamble is not None, "Preamble was None" - assert text in preamble, ( - f"Expected preamble to contain '{text}', got:\n{preamble}" - ) + assert text in preamble, f"Expected preamble to contain '{text}', got:\n{preamble}" # --------------------------------------------------------------------------- diff --git a/robot/helper_acms_pipeline_phase3.py b/robot/helper_acms_pipeline_phase3.py index 5122961bf..1d9d087d8 100644 --- a/robot/helper_acms_pipeline_phase3.py +++ b/robot/helper_acms_pipeline_phase3.py @@ -32,6 +32,19 @@ from cleveragents.application.services.acms_phase3 import ( # noqa: E402 RelevanceCoherenceOrderer, ) from cleveragents.application.services.acms_service import ACMSPipeline # noqa: E402 +from cleveragents.domain.models.acms.crp import ContextRequest # noqa: E402 +from cleveragents.domain.models.acms.strategy import ( # noqa: E402 + BackendSet, + PlanContext, +) +from cleveragents.domain.models.acms.stubs import ( # noqa: E402 + InMemoryGraphBackend, + InMemoryTextBackend, + InMemoryVectorBackend, +) +from cleveragents.domain.models.acms.temporal_stubs import ( # noqa: E402 + InMemoryTemporalBackend, +) from cleveragents.domain.models.core.context_fragment import ( # noqa: E402 ContextBudget, ContextFragment, @@ -118,64 +131,117 @@ def _test_preamble_generate() -> None: def _test_arce_strategy() -> None: - """Test ArceStrategy iterative refinement.""" - strategy = ArceStrategy(max_iterations=3) + """Test ArceStrategy under the unified ContextStrategy protocol. + + Verifies the multi-modal pipeline contract: requires text + vector + + graph backends, returns the spec quality score, and gates assemble() + on backend availability. + """ + strategy = ArceStrategy() assert strategy.name == "arce" - assert strategy.can_handle({}) == 0.95 - assert strategy.capabilities.supports_semantic_search + assert strategy.capabilities.uses_text + assert strategy.capabilities.uses_vector + assert strategy.capabilities.uses_graph + assert strategy.capabilities.quality_score == 0.95 - frags = [ - _frag("project://app/io.py", "async IO handler", 0.7, 20, 5), - _frag("project://app/main.py", "main application", 0.5, 15, 3), - ] - budget = ContextBudget(max_tokens=1000, reserved_tokens=0) - result = list(strategy.assemble(frags, budget)) - assert len(result) > 0 - total = sum(f.token_count for f in result) - assert total <= 1000 + request = ContextRequest(query="async IO") + plan_context = PlanContext() + budget = 1000 - # Empty input - assert list(strategy.assemble([], budget)) == [] + # All three backends present → can_handle returns spec quality score. + full_backends = BackendSet( + text=InMemoryTextBackend(), + vector=InMemoryVectorBackend(), + graph=InMemoryGraphBackend(), + ) + assert strategy.can_handle(request, full_backends) == 0.95 + + # Missing any backend → can_handle returns 0.0 and assemble returns []. + partial_backends = BackendSet(text=InMemoryTextBackend()) + assert strategy.can_handle(request, partial_backends) == 0.0 + partial_result = strategy.assemble(request, partial_backends, budget, plan_context) + assert list(partial_result) == [] + + # All backends present → assemble runs without crashing (stub backends + # return empty results, so the merged output is also empty). + result = list(strategy.assemble(request, full_backends, budget, plan_context)) + assert isinstance(result, list) print("phase3-arce-ok") def _test_temporal_strategy() -> None: - """Test TemporalArchaeologyStrategy cold-tier preference.""" + """Test TemporalArchaeologyStrategy under the unified protocol. + + Verifies the strategy queries the temporal backend for cold-tier + nodes (spec §43193-43195), and gates assemble() on graph + temporal + backend availability. + """ strategy = TemporalArchaeologyStrategy() assert strategy.name == "temporal-archaeology" - assert strategy.can_handle({}) == 0.5 + assert strategy.capabilities.uses_graph + assert strategy.capabilities.uses_temporal + assert strategy.capabilities.quality_score == 0.5 - frags = [ - _frag("project://app/old.py", "archived", 0.5, 20, 3, tier="cold"), - _frag("project://app/new.py", "recent", 0.9, 15, 3, tier="hot"), - ] - budget = ContextBudget(max_tokens=1000, reserved_tokens=0) - result = list(strategy.assemble(frags, budget)) - assert len(result) > 0 - # Cold-tier should be first - assert result[0].tier == "cold" + request = ContextRequest(query="historical") + plan_context = PlanContext() + budget = 1000 - assert list(strategy.assemble([], budget)) == [] + # Both backends present → can_handle returns spec quality score. + full_backends = BackendSet( + graph=InMemoryGraphBackend(), + temporal=InMemoryTemporalBackend(), + ) + assert strategy.can_handle(request, full_backends) == 0.5 + + # Missing temporal backend → can_handle returns 0.0 and assemble + # returns [] (cold-tier query is impossible without it). + graph_only = BackendSet(graph=InMemoryGraphBackend()) + assert strategy.can_handle(request, graph_only) == 0.0 + assert list(strategy.assemble(request, graph_only, budget, plan_context)) == [] + + # Both backends present → assemble queries cold tier without crashing. + result = list(strategy.assemble(request, full_backends, budget, plan_context)) + assert isinstance(result, list) print("phase3-temporal-ok") def _test_plan_decision_strategy() -> None: - """Test PlanDecisionContextStrategy warm/cold preference.""" + """Test PlanDecisionContextStrategy under the unified protocol. + + Verifies the strategy queries the temporal backend on the warm tier + when no parent plan IDs are provided (spec §43197-43199), and gates + assemble() on temporal backend availability. + """ strategy = PlanDecisionContextStrategy() assert strategy.name == "plan-decision-context" - assert strategy.can_handle({}) == 0.7 + assert strategy.capabilities.uses_temporal + assert strategy.capabilities.quality_score == 0.7 - frags = [ - _frag("project://app/plan.py", "decision", 0.7, 20, 3, tier="warm"), - _frag("project://app/new.py", "new code", 0.9, 15, 3, tier="hot"), - ] - budget = ContextBudget(max_tokens=1000, reserved_tokens=0) - result = list(strategy.assemble(frags, budget)) - assert len(result) > 0 - # Warm-tier should be first - assert result[0].tier == "warm" + request = ContextRequest(query="parent plan decision") + budget = 1000 - assert list(strategy.assemble([], budget)) == [] + # Temporal backend present → can_handle returns spec quality score. + full_backends = BackendSet(temporal=InMemoryTemporalBackend()) + assert strategy.can_handle(request, full_backends) == 0.7 + + # No temporal backend → can_handle returns 0.0 and assemble returns []. + no_backends = BackendSet() + assert strategy.can_handle(request, no_backends) == 0.0 + plan_context = PlanContext() + assert list(strategy.assemble(request, no_backends, budget, plan_context)) == [] + + # With parent_plan_id, the strategy walks ancestor plan history; + # without one, it falls back to a warm-tier RECENT query. Both must + # complete without crashing under an empty stub temporal backend. + parented = PlanContext(parent_plan_id=_PLAN_ID) + assert isinstance( + list(strategy.assemble(request, full_backends, budget, parented)), + list, + ) + assert isinstance( + list(strategy.assemble(request, full_backends, budget, plan_context)), + list, + ) print("phase3-plan-decision-ok") diff --git a/robot/helper_context_strategies.py b/robot/helper_context_strategies.py index 345db1696..1c9f16dfa 100644 --- a/robot/helper_context_strategies.py +++ b/robot/helper_context_strategies.py @@ -95,11 +95,13 @@ def _cmd_simple_keyword_rank() -> int: """SimpleKeywordStrategy ranks keyword-matching fragments first.""" strategy = SimpleKeywordStrategy() - text_backend = _KeywordTextBackend([ - ("project://app/io.py", "Use async IO pattern", 0.5), - ("project://app/main.py", "Main entry point", 0.8), - ("project://app/net.py", "Async network handler", 0.6), - ]) + text_backend = _KeywordTextBackend( + [ + ("project://app/io.py", "Use async IO pattern", 0.5), + ("project://app/main.py", "Main entry point", 0.8), + ("project://app/net.py", "Async network handler", 0.6), + ] + ) backends = BackendSet(text=text_backend) request = ContextRequest(query="async IO") plan_context = PlanContext() @@ -120,11 +122,13 @@ def _cmd_simple_keyword_budget() -> int: """SimpleKeywordStrategy respects token budget.""" strategy = SimpleKeywordStrategy() - text_backend = _KeywordTextBackend([ - ("project://app/a.py", "hello world", 0.9), - ("project://app/b.py", "hello there", 0.7), - ("project://app/c.py", "hello again", 0.5), - ]) + text_backend = _KeywordTextBackend( + [ + ("project://app/a.py", "hello world", 0.9), + ("project://app/b.py", "hello there", 0.7), + ("project://app/c.py", "hello again", 0.5), + ] + ) backends = BackendSet(text=text_backend) request = ContextRequest(query="hello") plan_context = PlanContext() @@ -142,11 +146,13 @@ def _cmd_semantic_embedding_rank() -> int: """SemanticEmbeddingStrategy ranks by vector similarity.""" strategy = SemanticEmbeddingStrategy() - vector_backend = _VectorBackend([ - ("project://app/db.py", "Database connection pool manager", 0.85), - ("project://app/io.py", "File input output handler", 0.5), - ("project://app/sql.py", "SQL database query executor", 0.7), - ]) + vector_backend = _VectorBackend( + [ + ("project://app/db.py", "Database connection pool manager", 0.85), + ("project://app/io.py", "File input output handler", 0.5), + ("project://app/sql.py", "SQL database query executor", 0.7), + ] + ) backends = BackendSet(vector=vector_backend) request = ContextRequest(query="database connection pool") plan_context = PlanContext() diff --git a/src/cleveragents/domain/models/acms/strategy_stubs.py b/src/cleveragents/domain/models/acms/strategy_stubs.py index 966bba8e3..39a158480 100644 --- a/src/cleveragents/domain/models/acms/strategy_stubs.py +++ b/src/cleveragents/domain/models/acms/strategy_stubs.py @@ -198,7 +198,8 @@ class SimpleKeywordStrategy: ) -> list[ContextFragment]: """Query TextBackend with keywords from the ContextRequest. - Returns fragments sorted by relevance score (descending), + Returns fragments ranked by query-keyword overlap count first + (more matches rank higher), then by backend relevance score, packed within the token budget. """ if backends.text is None: @@ -208,6 +209,7 @@ class SimpleKeywordStrategy: if not query: return [] + query_keywords = {w for w in query.lower().split() if w} scope = _scope_from_request(request) max_results = max(1, budget // max(1, _CHARS_PER_TOKEN * 10)) @@ -217,20 +219,26 @@ class SimpleKeywordStrategy: max_results=max_results, ) - fragments = [ - _make_fragment( - uko_node=r.uko_uri, - content=r.content, - relevance_score=r.score, - strategy_name=self.name, - resource_uri=r.uko_uri, - metadata=dict(r.metadata), + def _overlap(content: str) -> int: + return len(query_keywords & set(content.lower().split())) + + scored = [ + ( + _overlap(r.content), + _make_fragment( + uko_node=r.uko_uri, + content=r.content, + relevance_score=r.score, + strategy_name=self.name, + resource_uri=r.uko_uri, + metadata=dict(r.metadata), + ), ) for r in results ] - # Sort by relevance descending - fragments.sort(key=lambda f: f.relevance_score, reverse=True) + scored.sort(key=lambda pair: (pair[0], pair[1].relevance_score), reverse=True) + fragments = [frag for _, frag in scored] return _budget_fragments(fragments, budget) def explain(self) -> str: