Skip to content

feat(phase1): tool reranking, rejection, reflection, context repository, extended metrics, agent recommender - #10

Open
cherninkiy wants to merge 1 commit into
mainfrom
dev
Open

feat(phase1): tool reranking, rejection, reflection, context repository, extended metrics, agent recommender#10
cherninkiy wants to merge 1 commit into
mainfrom
dev

Conversation

@cherninkiy

Copy link
Copy Markdown
Owner

Summary

Implements all Phase 1 tasks from docs/HOLOBIONT_ROADMAP.md — transforming RawLLM with multi-agent foundations: tool reranking/rejection, self-reflection cycle, context prompt repository, extended metrics with trajectory tracking, and a lightweight agent recommender.

Changes

New files:

  • core/tool_management.pyToolReranker (confidence-based reranking with success history, recency, context relevance) + ToolRejectionHandler (7 rejection reasons, duplicate detection, parameter constraints, alternative suggestions)
  • core/reflection.pyErrorAnalyzer (8 error categories with pattern matching), CorrectionGenerator (auto-correction with try-except wrapping), ReflectionLoop (full reflection cycle with history tracking)
  • core/context_repository.pyContextPromptRepository (template storage/retrieval), SemanticIndex (keyword-based similarity search), PromptTemplate (render with variables), 6 default templates, singleton accessor
  • core/agents/recommender.pyTrajectoryEncoder (4-dim latent vectors), AgentRecommender (history-based scoring with task similarity), CommitteeBuilder (diversity-aware committee selection)

Modified files:

  • core/metrics.py — Extended log_execution() with success_score, trajectory_id, step_number, task_type; added get_execution_events(), build_agent_history(); aggregate_by_plugin() now reports avg_success_score and trajectory_count

New test files:

  • tests/test_tool_management.py — 17 tests (reranking, rejection, duplicates, constraints, alternatives)
  • tests/test_context_repository.py — 21 tests (CRUD, semantic search, import/export, defaults, singleton)
  • tests/test_reflection.py — 30 tests (error analysis, correction generation, reflection loop, integration)
  • tests/test_agent_recommender.py — 4 tests (encoder roundtrip, recommendation ranking, success rate fallback, committee constraints)
  • tests/test_metrics.py — Extended with 10 new tests (success_score clamping, trajectory counting, agent history, mixed scores)

Test Results

99 passed in 0.17s

Phase 1 Completion Criteria

  • All tools go through reranking
  • System can argumentatively reject a tool with explanation
  • ToolReflection cycle works automatically on errors
  • Context Repository provides relevant prompts via semantic search
  • Metrics include success_score and trajectory_id

Known Gaps

  • prompt_buildir.pybuild_startup_prompt() does not yet accept a context_repository parameter (minor wiring task, does not block Phase 2)

- ContextPromptRepository: Prompt templates with variables, semantic search, 5 default templates, success tracking
- Tool Reflection Cycle: 8 error categories, auto-fix suggestions, iterative improvement, automatic try-except wrapping
- Tests: 51 total (21 context repository, 30 reflection)
- Docs: Recommender MVP completed, roadmap moved to docs/
- Fixes: PR review issues, flake8 compliance, .gitignore cleanup
- Refactor: Simplified agents/init.py

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements Phase 1 intelligence modules, featuring a ContextPromptRepository for semantic prompt retrieval, a ReflectionLoop for automated error analysis and correction, and an AgentRecommender system. The update includes enhancements to the metrics logging system and comprehensive unit tests. Reviewer feedback identifies several performance optimization opportunities, such as consolidating loops in trajectory calculations and pre-normalizing error patterns. Additionally, the feedback recommends moving inline imports and constants to the module level for PEP 8 compliance and adopting timezone-aware UTC timestamps for consistency across the codebase.

Comment on lines +40 to +45
count = len(events_list)
avg_success = sum(float(e.get("success_score", 0.0)) for e in events_list) / count
avg_latency = sum(float(e.get("execution_time_ms", 0.0)) for e in events_list) / count
avg_step = (
sum(float(e.get("step_number", 0.0) or 0.0) for e in events_list) / count
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The trajectory metrics are calculated using multiple generator expressions, which results in iterating over events_list three times. This can be optimized into a single loop for better performance, especially as the number of events grows.

Suggested change
count = len(events_list)
avg_success = sum(float(e.get("success_score", 0.0)) for e in events_list) / count
avg_latency = sum(float(e.get("execution_time_ms", 0.0)) for e in events_list) / count
avg_step = (
sum(float(e.get("step_number", 0.0) or 0.0) for e in events_list) / count
)
count = len(events_list)
total_success = 0.0
total_latency = 0.0
total_step = 0.0
for e in events_list:
total_success += float(e.get("success_score", 0.0))
total_latency += float(e.get("execution_time_ms", 0.0))
total_step += float(e.get("step_number", 0.0) or 0.0)
return [
max(0.0, min(1.0, total_success / count)),
max(0.0, min(1.0, (total_latency / count) / 10000.0)),
max(0.0, min(1.0, (total_step / count) / 20.0)),
max(0.0, min(1.0, count / 20.0)),
]

Comment on lines +181 to +193
chosen: list[dict[str, Any]] = []
seen_task_types: set[str] = set()
for agent in candidate_agents:
task_type = str(agent.get("task_type", "general"))
if task_type not in seen_task_types:
chosen.append(agent)
seen_task_types.add(task_type)

# Add remaining candidates to fill committee capacity.
for agent in candidate_agents:
if agent not in chosen:
chosen.append(agent)
return chosen

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation uses if agent not in chosen, which performs a full dictionary equality check against every element in the chosen list on every iteration. Using a set of agent_ids to track selected agents is more efficient and idiomatic for this type of filtering.

Suggested change
chosen: list[dict[str, Any]] = []
seen_task_types: set[str] = set()
for agent in candidate_agents:
task_type = str(agent.get("task_type", "general"))
if task_type not in seen_task_types:
chosen.append(agent)
seen_task_types.add(task_type)
# Add remaining candidates to fill committee capacity.
for agent in candidate_agents:
if agent not in chosen:
chosen.append(agent)
return chosen
chosen: list[dict[str, Any]] = []
seen_task_types: set[str] = set()
seen_ids: set[str] = set()
for agent in candidate_agents:
task_type = str(agent.get("task_type", "general"))
agent_id = str(agent.get("agent_id"))
if task_type not in seen_task_types:
chosen.append(agent)
seen_task_types.add(task_type)
seen_ids.add(agent_id)
# Add remaining candidates to fill committee capacity.
for agent in candidate_agents:
agent_id = str(agent.get("agent_id"))
if agent_id not in seen_ids:
chosen.append(agent)
seen_ids.add(agent_id)
return chosen


from __future__ import annotations

import logging

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Add import re here and remove the inline import in store_prompt to adhere to PEP 8 guidelines regarding module-level imports.

Suggested change
import logging
import logging
import re

Comment on lines +139 to +152
stopwords = {
"the", "a", "an", "is", "are", "was", "were", "be", "been",
"being", "have", "has", "had", "do", "does", "did", "will",
"would", "could", "should", "may", "might", "must", "shall",
"can", "need", "to", "of", "in", "for", "on", "with", "at",
"by", "from", "as", "into", "through", "during", "before",
"after", "above", "below", "between", "under", "again",
"further", "then", "once", "here", "there", "when", "where",
"why", "how", "all", "each", "few", "more", "most", "other",
"some", "such", "no", "nor", "not", "only", "own", "same",
"so", "than", "too", "very", "just", "and", "but", "if", "or",
"because", "until", "while", "although", "though", "this",
"that", "these", "those", "it", "its"
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The stopwords set is redefined on every call to _extract_keywords. Moving this to a class-level or module-level constant would improve performance, especially during batch indexing operations where this method is called frequently.

"""
# Auto-detect variables if not provided
if variables is None:
import re

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The re module is imported inside the method. Per PEP 8, imports should be at the top of the file. This also avoids the overhead of repeated import checks during runtime.

Comment on lines +434 to +435
data["template_id"] = template_id
template = PromptTemplate.from_dict(data)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This implementation modifies the data dictionary in-place, which is part of the templates_data input. This side effect might be unexpected for the caller. It is safer to create a new dictionary or pass the ID explicitly during instantiation.

Suggested change
data["template_id"] = template_id
template = PromptTemplate.from_dict(data)
template = PromptTemplate.from_dict({**data, "template_id": template_id})

Comment thread core/reflection.py

import logging
import textwrap
from datetime import datetime

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import timezone from datetime to support UTC timestamp generation for consistency with other modules in the repository.

Suggested change
from datetime import datetime
from datetime import datetime, timezone

Comment thread core/reflection.py
Comment on lines +168 to +171
self._error_patterns = {
category: [pattern.lower() for pattern in patterns]
for category, patterns in self._error_patterns.items()
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This normalization loop runs on every instantiation of ErrorAnalyzer. It is more efficient to define the patterns in lowercase directly in the dictionary literal above and remove this processing step.

Comment thread core/reflection.py
"tool_call": tool_call,
"error_analysis": error_analysis.to_dict(),
"correction": correction.to_dict(),
"timestamp": datetime.now().isoformat(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using datetime.now() creates a naive datetime object. It is better practice to use datetime.now(timezone.utc) for consistent, timezone-aware timestamps, matching the pattern used in core/metrics.py.

Suggested change
"timestamp": datetime.now().isoformat(),
"timestamp": datetime.now(timezone.utc).isoformat(),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant