review-pr

name	review-pr
description	Reviews pull requests for code quality, pattern conformance, architecture alignment, security, and completeness. Use when asked to review a PR, validate code changes, or check if a PR is ready for merge.

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.

Comments and Docstrings

Minimal, meaningful documentation:

• No useless comments stating the obvious
• Docstrings only when function signature is not self-explanatory

Style Violation: Excessive comments/docstrings

def get_evaluator(name: str) -> BaseEvaluator:
    """Get an evaluator by name.
    
    Args:
        name: The name of the evaluator
    
    Returns:
        The evaluator instance
    """

Required Fix: Remove docstring - the function signature is self-explanatory

Async Conventions

• Core evaluation logic is in evaluate_async() methods
• call_async_fn() utility used for sync→async bridge (from ragelo.utils)
• asyncio.wait() used for bounded concurrent execution in evaluate_experiment()
• No mixing of sync and async patterns within the same flow

---

Step 7: Testing Quality

Testing Patterns in RAGElo

RAGElo uses pytest with pytest-asyncio and pytest-mock.

Key testing conventions:

• MockLLMProvider in tests/conftest.py returns deterministic responses based on the requested Pydantic schema
• experiment fixture loads test data from tests/data/ (2 queries, 4 docs, 2 agents)
• OpenAI integration tests gated with @pytest.mark.requires_openai (skipped unless --runopenai)
• Tests organized under tests/unit/ and tests/cli/

Test Quality Checklist

• Uses MockLLMProvider or similar fixture — not hand-rolled mocks that mirror implementation
• Tests the public interface (factory functions, evaluate_experiment()) not internal methods
• Uses existing fixtures (experiment, llm_provider_config, etc.)
• OpenAI-dependent tests properly gated with @pytest.mark.requires_openai
• Async tests use pytest-asyncio conventions

Test Coverage Priority:

1. Happy paths (highest priority)
2. Important edge cases (malformed input, missing fields)
3. Error handling paths
4. NOT: Trivial cases that add no value

AI Slop Test Indicators:

• Testing obvious getters/setters
• Excessive mocking that mirrors implementation details
• Tests that break when implementation changes (brittle)
• Not using existing fixtures and patterns from conftest.py

Running Tests

# All tests (OpenAI skipped)
pytest tests/

# With OpenAI integration tests
pytest tests/ --runopenai

# Single file
pytest tests/unit/test_experiment.py -v

---

Step 8: Architecture Alignment

RAGElo Architecture Overview

Verify the PR respects these architectural patterns:

Factory + Registry Pattern:

• All major components use decorator-based factory registration
• Enum types in ragelo/types/types.py define valid component names
• Registration: @Factory.register(EnumType.NAME) on the class
• Instantiation: via factory functions (get_retrieval_evaluator(), get_llm_provider(), get_agent_ranker())

Evaluator Hierarchy:

BaseEvaluator (ragelo/evaluators/base_evaluator.py) — async-first, abstract
├── BaseRetrievalEvaluator — evaluates document relevance (Query + Document -> score)
│   Implementations: Reasoner, RDNAM, DomainExpert, FewShot, CustomPrompt
└── BaseAnswerEvaluator — evaluates answer quality (Query + AgentAnswer -> score/winner)
    Implementations: Pairwise, ChatPairwise, CustomPairwise, DomainExpert, CustomPrompt

LLM Providers:

• BaseLLMProvider → call_async(input, response_schema) → LLMResponseType[T]
• Implementations: OpenAIProvider, OllamaProvider

Data Model Layering:

• Core evaluables in ragelo/types/evaluables.py: Query, Document, AgentAnswer, PairwiseGame
• Results in ragelo/types/results.py: EvaluatorResult and subclasses
• LLM answer schemas in ragelo/types/answer_formats.py: Pydantic models for structured LLM output
• Configs in ragelo/types/configurations/: one config class per component

Experiment Orchestration:

• Experiment class in ragelo/types/experiment.py is the central orchestrator
• Loads from CSV, manages evaluation state, caches to avoid redundant LLM calls, persists to JSON/JSONL

If architecture violations are found:

⚠️ Architecture Violation: [Describe the violation]

Expected pattern: [Explain the correct RAGElo pattern with file references]

Required Fix: [Specific remediation]

---

Step 9: Security Review

Review security concerns relevant to a library that makes LLM API calls. Do NOT output a checklist. Instead, for each concern identified, use this format:

Security Requirements to Verify

• No hardcoded API keys, secrets, or credentials (use environment variables)
• API keys handled via SecretStr or similar — not logged or printed
• Jinja2 templates not vulnerable to template injection from user-supplied metadata
• No arbitrary code execution from user-supplied prompts or configuration
• Dependencies properly declared in pyproject.toml with version constraints
• Cached results (JSON/JSONL files) don't leak sensitive information

Output Format for Security Issues

For each security concern:

🔴 Security Concern: [Brief title]

Issue: [What the code does wrong or fails to do]

Risk: [Potential exploit, impact, or vulnerability]

Required Fix: [Specific remediation steps]

If no security concerns are identified, state: "No security concerns identified in this review."

---

Step 10: Generate Review Summary

Compile findings into a structured review:

# PR Review: [PR Title]

## Summary
[1-2 sentence overview of what the PR does]

## Implementation Verification
[✅ Matches PR description / ⚠️ Discrepancies found]
[List any mismatches between description and code]

## Breaking Changes
[🚨 Breaking changes found / ✅ No breaking changes]
[List any breaking changes with migration path assessment]

## Pattern & Style Conformance
[✅ Passes / ⚠️ Issues Found]
[List any violations with required fixes]

## Architecture Alignment
[✅ Aligned / ⚠️ Violations Found]
[List any architecture violations]

## Code Quality & Testing
[✅ Good / ⚠️ Issues Found]
[List any testing or code quality issues]

## Issue Linkage
- **Related Issues**: [List or "None identified"]

## Security Concerns
[List concerns in Security Concern format, or "No security concerns identified"]

## Recommendations

### Required Changes (Blocking)
1. [Critical issues that must be fixed]

### Suggested Improvements (Non-blocking)
1. [Nice-to-have improvements]

## Approval Status
[✅ Approved / ⚠️ Approved with comments / 🚫 Changes requested]

---

What NOT to Do

❌ Do not approve without reading the actual code changes ❌ Do not trust PR descriptions without verification ❌ Do not skip pattern and style conformance checking ❌ Do not accept novel patterns when established ones exist ❌ Do not treat pattern violations as style preferences ❌ Do not ignore security concerns (especially API key handling) ❌ Do not allow breaking changes to the public API without migration paths ❌ Do not let the codebase feel like it was built by mercenaries ❌ Do not accept AI slop (generic, pattern-ignorant code) ❌ Do not accept tests that don't use existing fixtures from conftest.py ❌ Do not accept f-strings or .format() for LLM prompt construction ❌ Do not accept synchronous-only evaluator implementations ❌ Do not accept components that bypass the Factory+Registry pattern

What TO Do

✅ Verify code actually implements what the PR description claims ✅ Flag all breaking changes to the public Python API and CLI ✅ Search for existing patterns BEFORE evaluating changes - This is mandatory ✅ Cite specific file:line references when flagging violations ✅ Verify code style matches existing codebase (ruff rules, 119 char lines, snake_case) ✅ Verify new components follow Factory+Registry pattern with enum types ✅ Verify evaluators are async-first and use Jinja2 templates ✅ Verify testing follows existing patterns (MockLLMProvider, fixtures, @pytest.mark.requires_openai) ✅ Check for related GitHub issues ✅ Review API key handling and credential security ✅ Ruthlessly reject foreign patterns - Suggest the existing way instead ✅ Be a chameleon - Changes must blend seamlessly with existing code ✅ Flag monolith page files and demand decomposition into _lib/ and _components/ ✅ Flag duplicated UI patterns and utility functions — demand shared components/modules ✅ Flag complex inline JSX callbacks — demand named handler functions ✅ Provide specific, actionable feedback ✅ Distinguish between blocking and non-blocking issues