Review PR

PR Review Skill

Overview

RAGElo is a Python library and CLI for evaluating RAG (Retrieval-Augmented Generation) agents using Elo-based tournament ranking. This skill reviews pull requests to ensure they:

• Actually implement what the PR description claims
• Follow existing codebase patterns (the Chameleon Principle)
• Match the code style of the existing codebase
• Align with RAGElo's established architecture (Factory+Registry, async evaluators, Pydantic configs)
• Address security concerns properly
• Introduce no breaking changes to the public API (or have proper migration paths)
• Relate to existing GitHub issues appropriately

The Chameleon Principle

The codebase should feel like it was architected by one mind, not assembled by mercenaries.

Every PR must introduce changes that blend seamlessly with existing patterns. When reviewing:

• Find the existing pattern first - Before accepting any change, search for how similar problems are already solved
• Reject foreign patterns - If the PR introduces a pattern that doesn't exist elsewhere, flag it ruthlessly
• Suggest the existing way - Always recommend the established approach over novel solutions
• Be ruthless - Pattern violations are not style nits; they are architectural debt

This principle applies to everything: architecture, code organization, naming conventions, testing approaches, and code style. AI agents are notorious for ignoring existing patterns and producing the same generic slop everywhere. The reviewer must catch this.

Output Format

The agent must produce:

1. Summary - Brief overview of what the PR does
2. Implementation Verification - Does the code actually do what the PR claims?
3. Breaking Changes - Any backward-incompatible changes to the public API and migration paths
4. Pattern & Style Conformance - Does the PR follow existing codebase patterns and style?
5. Architecture Alignment - Is the PR consistent with RAGElo's Factory+Registry architecture?
6. Code Quality & Testing - Test coverage, test quality, code organization
7. Issue Linkage - Related GitHub issues
8. Security Concerns - Issues identified against security requirements
9. Recommendations - Required changes, suggestions, and approval status

Step 1: Gather PR Context

Get the PR description

gh pr view --json title,body,number | jq '.'

The PR description tells you what the author claims the PR does. Your job is to verify this claim against the actual code.

Get the list of commits

git log origin/master..HEAD --oneline

Get the full diff

git diff origin/master..HEAD

Get list of changed files

git diff --name-only origin/master..HEAD

CRITICAL: Do not rely on commit messages or PR descriptions alone. You MUST read the actual code changes to understand what was done. The PR description may be incomplete, misleading, or outright wrong.

Step 2: Verify Implementation Matches PR Description

This is a primary concern. Compare what the PR description claims against what the code actually does:

• Does the code implement all features/fixes mentioned in the description?
• Does the code do anything NOT mentioned in the description?
• Are there discrepancies between stated behavior and actual behavior?

If there are discrepancies:

⚠️ Implementation Mismatch

PR claims: [what the description says]

Code actually does: [what you found in the diff]

Missing: [features claimed but not implemented]

Undocumented: [changes made but not mentioned]

Step 3: Check for Breaking Changes

RAGElo is a published library (pip install ragelo). We never introduce breaking changes without proper migration paths.

Identify breaking changes in:

• Public Python API (functions in ragelo/__init__.py, factory functions like get_retrieval_evaluator(), get_llm_provider(), get_agent_ranker())
• CLI interface (Typer commands in ragelo/cli/)
• Pydantic configuration classes (removed fields, changed types, changed defaults)
• Data model types (ragelo/types/evaluables.py, ragelo/types/results.py)
• Experiment JSON/JSONL serialization format
• Enum values in ragelo/types/types.py (removing or renaming enum members)

For each breaking change:

🚨 Breaking Change Identified

What breaks: [Describe the incompatibility]

Affected consumers: [Who/what depends on this]

Migration path provided: [Yes/No - describe if present]

Required action: [How to make this backward compatible, or document migration]

Step 4: Check for Related GitHub Issues

Browse recent open issues

gh issue list --limit 50 --json number,title | jq -r '.[] | "\(.number): \(.title)"'

Search by keywords from the PR

gh issue list --search "<keyword>" --json number,title | jq -r '.[] | "\(.number): \(.title)"'

If there's an obvious tracking issue that should be linked but isn't in the PR description:

💡 Suggested Issue Link: This PR appears to address issue #123 ([issue title]). Consider adding Closes #123 or Relates to #123 to the PR description.

Step 5: Pattern Conformance Analysis (Be Ruthless)

The cardinal rule: Find the existing pattern FIRST

Before evaluating ANY code change, actively search the codebase for how similar problems are already solved.

Pattern discovery process

1. Identify the problem category

   • New evaluator? → Check existing evaluators in ragelo/evaluators/retrieval_evaluators/ and ragelo/evaluators/answer_evaluators/
   • New LLM provider? → Check ragelo/llm_providers/openai_client.py and ragelo/llm_providers/ollama_client.py
   • Configuration? → Check existing configs in ragelo/types/configurations/
   • New data type? → Check ragelo/types/evaluables.py and ragelo/types/results.py
   • Prompt template? → Check existing Jinja2 templates in evaluator classes

2. Search for analogous implementations Use semantic_search or grep_search to find 3+ examples of the same pattern to confirm it's established.

3. Document the existing pattern

   • Where is it used?
   • What are its characteristics?
   • Why was it chosen?

RAGElo-specific pattern conformance checklist

Factory + Registry pattern:

• New components registered via decorator: @Factory.register(EnumType.NAME)
• Enum value added to ragelo/types/types.py (RetrievalEvaluatorTypes, AnswerEvaluatorTypes, LLMProviderTypes, AgentRankerTypes)
• Factory create() method works with the new component
• Convenience function updated (e.g., get_retrieval_evaluator(), get_llm_provider())

Evaluator pattern:

• Inherits from BaseRetrievalEvaluator or BaseAnswerEvaluator
• Implements evaluate_async() as the core abstract method
• Uses self.llm_provider for LLM calls — does not instantiate its own provider
• Returns proper result type (RetrievalEvaluatorResult, AnswerEvaluatorResult, PairwiseGameEvaluatorResult)
• Uses Jinja2 Template for prompt formatting (not f-strings or .format())
• Has a corresponding Pydantic config class in ragelo/types/configurations/

Configuration pattern:

• Config class inherits from appropriate base (BaseEvaluatorConfig, BaseRetrievalEvaluatorConfig, BaseAnswerEvaluatorConfig, LLMProviderConfig)
• Uses Pydantic BaseModel conventions (type annotations, defaults, validators)
• Config class is discoverable via get_config_class() introspection

LLM Provider pattern:

• Inherits from BaseLLMProvider
• Implements call_async() returning LLMResponseType[T_Schema]
• Accepts LLMProviderConfig (or subclass) in constructor
• Registered via @LLMProviderFactory.register(LLMProviderTypes.NAME)
• Supports structured output via response_schema parameter

Data model pattern:

• Core types (Query, Document, AgentAnswer, PairwiseGame) used as-is — not subclassed unnecessarily
• Result types inherit from EvaluatorResult
• LLM answer schemas in ragelo/types/answer_formats.py use Pydantic BaseModel
• Extra CSV columns → metadata dict pattern respected

Prompt templating pattern:

• Jinja2 Template objects, not raw string formatting
• Template variables match established names: {{ query.query }}, {{ document.text }}, {{ answer.text }}, {{ game.agent_a_answer.text }}
• Metadata accessed via {{ query.metadata.column_name }}

CLI pattern:

• Uses Typer (ragelo/cli/)
• CLI parameters dynamically generated from Pydantic config classes
• Follows existing subcommand structure

Red flags to ruthlessly reject

🚫 Novel patterns when established ones exist

• "Let's use a different approach here" → NO. Use the existing approach.

🚫 Inconsistent naming

• user_id in one place, userId in another → NO. Match existing convention (snake_case throughout).

🚫 Bypassing the Factory+Registry pattern

• Directly instantiating evaluators instead of using get_retrieval_evaluator() → NO. Use the factory.

🚫 New dependencies for solved problems

• "Let's add library X" when existing code solves it → NO. Use existing solution.

🚫 Different error handling

• Custom exception hierarchies that don't match existing → NO. Use established patterns.

🚫 f-strings or .format() for LLM prompts

• All prompts must use Jinja2 Template objects → NO exceptions.

🚫 Synchronous-only evaluator implementations

• All evaluators must implement evaluate_async() → NO synchronous-only variants.

When flagging pattern violations

Always show the existing pattern as evidence:

Pattern Violation: PR uses f-string for prompt formatting

Existing Pattern: Codebase uses Jinja2 Template for all LLM prompts

Evidence: Found in ragelo/evaluators/retrieval_evaluators/reasoner_evaluator.py, ragelo/evaluators/answer_evaluators/pairwise_evaluator.py

Required Fix: Convert to Jinja2 Template

Step 6: Code Style Conformance (Detect AI Slop)

AI coding agents are notorious for producing generic, pattern-ignorant code. The codebase must read as if a single person wrote it. Flag these common AI slop indicators:

Linting & Formatting

RAGElo uses ruff for linting and formatting:

• Rules: E, F, I (errors, pyflakes, isort)
• Line length: 119 characters
• Type checking: mypy with pydantic.mypy plugin

Verify with:

ruff check ragelo/
ruff format --check ragelo/
mypy ragelo/

Class Design

Single Responsibility Principle:

• Each class has one clear responsibility
• Classes are not "god objects" doing everything

Dependency Injection:

• BaseLLMProvider is injected into evaluators, not instantiated inside them
• Evaluators receive config objects, not raw kwargs

Imports

Absolute imports from ragelo package:

• ✅ from ragelo.types.evaluables import Query
• ❌ from .evaluables import Query
• ❌ from ..types.evaluables import Query

Note: from __future__ import annotations is used throughout the codebase for forward references.