Pr Review Assistant

PR Review Assistant Skill

Purpose

Philosophy-aware pull request reviews that go beyond syntax and style to check alignment with amplihack's core development principles. This skill reviews PRs not just for correctness, but for ruthless simplicity, modular architecture, and zero-BS implementation.

When to Use This Skill

• PR Code Reviews: Review PRs against amplihack philosophy principles
• Philosophy Compliance: Check that code embodies ruthless simplicity and brick module design
• Refactoring Suggestions: Identify over-engineering and suggest concrete simplifications
• Architecture Verification: Verify modular design and clear contracts
• Test Coverage: Assess test adequacy for changed functionality
• Design Assessment: Catch over-engineering before it gets merged

Core Philosophy: What We Review For

1. Ruthless Simplicity

Every line of code must justify its existence. We ask:

• Can this be simpler? Does each function do one thing well?
• Is this necessary now? Or is it future-proofing?
• Are there unnecessary abstractions? Extra layers that don't add value?
• Can we remove lines? The best code is code that doesn't exist.

2. Modular Architecture (Brick & Studs)

Code should be organized as self-contained modules with clear connections:

• Brick = Self-contained module with ONE clear responsibility
• Stud = Public contract (functions, API, data models) others connect to
• Regeneratable = Can be rebuilt from specification without breaking connections

3. Zero-BS Implementation

No shortcuts, stubs, or technical debt:

• No TODOs in code = Actually implement or don't include it
• No NotImplementedError = Except in abstract base classes
• No mock data = Real functionality from the start
• No dead code = Remove unused code
• Every function works = Or it doesn't exist

4. Quality Over Speed

• Robust implementations = Better than quick fixes
• Long-term maintainability = Not short-term gains
• Clear error handling = Errors visible, not swallowed
• Tested behavior = Verify contracts at module boundaries

Review Process

Step 1: Understand the Changes

Start by understanding what the PR changes:

1. Read the PR description to understand intent 2. Identify affected modules and their scope 3. Note the dependencies changed or added 4. Understand the problem being solved

Step 2: Check Philosophy Alignment

Review each change against amplihack principles:

Ruthless Simplicity Check

• Is every line necessary?
• Are there unnecessary abstractions?
• Could this be implemented more simply?
• Is there future-proofing or speculation?
• Are there duplicate or similar functions?
• Could conditional logic be simplified?

Module Structure Check

• Does the change respect module boundaries?
• Are public contracts clear and documented?
• Are internal utilities isolated?
• Does the module have ONE clear responsibility?
• Are there circular dependencies?

Zero-BS Check

• Are there TODOs or NotImplementedError calls?
• Are mock or test data exposed in production code?
• Is error handling explicit and visible?
• Are all functions working implementations?
• Is there dead code or unused variables?

Step 3: Identify Over-Engineering

Look for common over-engineering patterns:

• Over-abstraction: Base classes, protocols, factories for no clear benefit
• Generic "frameworks": Building infrastructure for hypothetical needs
• Premature optimization: Complex algorithms for non-critical paths
• Configuration complexity: 50-line config when 5-line default would work
• Future-proofing: "We might need this someday" code
• Excessive layering: More indirection than necessary
• Over-parameterization: Functions with 8+ parameters instead of simpler approach

Step 4: Verify Brick Module Structure

If new modules or module changes:

• Single responsibility? What is the ONE thing this module does?
• Clear public interface? What's exported and why?
• Internal isolation? Are utilities contained within module?
• Dependencies documented? What does it depend on?
• Tests included? Does spec define test requirements?
• Examples provided? Is usage clear?
• Regeneratable? Could this be rebuilt from a specification?

Step 5: Check Test Coverage

Adequate testing is crucial:

• Contract verification: Tests verify public interface behavior
• Edge cases covered: Null, empty, boundary conditions tested
• Error paths tested: Exceptions raised when expected
• Integration tested: Module connections verified
• Coverage adequate: 85%+ for changed code

Step 6: Provide Constructive Feedback

When suggesting changes:

1. Be specific: Reference file:line numbers 2. Explain why: What principle is violated? 3. Suggest how: Provide concrete examples 4. Be respectful: Focus on code, not person 5. Acknowledge good work: Recognize what's done well

Concrete Review Checklist

Ruthless Simplicity

• [ ] Every function has single clear purpose
• [ ] No unnecessary abstraction layers
• [ ] No future-proofing or speculation
• [ ] No duplicate logic or functions
• [ ] Conditional logic is straightforward
• [ ] Variable names are clear and self-documenting
• [ ] Function signatures aren't over-parameterized

Modular Architecture

• [ ] Module has ONE clear responsibility
• [ ] Public interface is minimal and clear
• [ ] Internal utilities properly isolated
• [ ] Dependencies are explicit
• [ ] No circular dependencies
• [ ] Clear contracts at boundaries
• [ ] Module can be understood independently

Zero-BS Implementation

• [ ] No TODOs, NotImplementedError, or stubs
• [ ] No mock/test data in production code
• [ ] No dead code or unused imports
• [ ] Error handling is explicit and visible
• [ ] All functions have working implementations
• [ ] No swallowed exceptions
• [ ] Clear logging/error messages for debugging

Test Coverage

• [ ] Public interface is tested
• [ ] Edge cases covered
• [ ] Error conditions tested
• [ ] Integration points verified
• [ ] Coverage adequate (85%+)
• [ ] Tests verify contract, not implementation

Documentation

• [ ] Docstrings are clear and complete
• [ ] Public interface documented
• [ ] Examples provided for new features
• [ ] Module README updated if needed
• [ ] Type hints present and accurate

Example Reviews

Example 1: Identifying Over-Engineering

PR: Add user permission checking to API

Code Changed:

class PermissionValidator:
    def __init__(self):
        self.cache = {}

    def validate(self, user, resource):
        if user in self.cache:
            return self.cache[user]

        result = self._complex_validation(user, resource)
        self.cache[user] = result
        return result

    def _complex_validation(self, user, resource):
        # Complex business logic...
        pass

Review Comment:

```` FILE: permissions.py (lines 10-25)

This over-engineers the permission checking with caching that may not be needed. The caching layer adds complexity without proven benefit:

1. Cache can become stale if user permissions change 2. Unclear when/if cache should be invalidated 3. In-memory cache doesn't scale across processes 4. Permission checks are usually not in hot paths

SUGGESTION - Start simpler:

def check_permission(user, resource):
    """Check if user can access resource."""
    # Direct implementation
    return user.has_access_to(resource)

If caching is needed later, add it when profiling shows it helps.

This aligns with ruthless simplicity: don't add complexity until proven necessary.

### Example 2: Identifying Lack of Regeneration Documentation

**PR**: Add new authentication module

**Code Changed**: New file `~/.amplihack/.claude/tools/auth/auth.py`

**Review Comment**:

FILE: .claude/tools/auth/ (new module)

Good: Module has clear responsibilities (JWT validation, token refresh). Issue: Missing specification documentation for module regeneration.

Brick Philosophy Requirement: Modules should be regeneratable from spec.

ACTION NEEDED:

1. Create Specs/authentication.md with:

• Clear purpose and scope
• Public interface (exported functions)
• Dependencies listed
• Test requirements
• Usage examples

This enables future rebuilding if requirements change.

See .claude/skills/pr-review-assistant/ for specification template.

````

Example 3: Identifying Zero-BS Issues

PR: Add data processing pipeline

Code Changed:

def process_data(data):
    """Process data through pipeline."""
    # TODO: Add error handling
    result = validate(data)
    if not result:
        return None  # swallowed error

    # TODO: Implement caching
    return transform(result)

Review Comment:

```` FILE: pipeline.py (lines 5-15)

Zero-BS Issues Found:

1. LINE 6: TODO comments - implement or remove

• "Add error handling" - Either add it now or create issue
• "Implement caching" - Not needed yet; add if profiling shows benefit

2. LINE 10: Swallowed error

• validate() likely raised exception, but returns None
• Make error explicit: raise ValueError("Invalid data")
• Or catch and re-raise with context

3. LINE 8: Silent failures

• If None is returned, caller doesn't know why
• Either raise exception or return Result type

SUGGESTION:

def process_data(data):
    """Process data through pipeline.

    Raises:
        ValueError: If data validation fails
    """
    result = validate(data)  # Raises if invalid
    return transform(result)

All code is production-ready, no TODOs.

### Example 4: Identifying Missing Tests

**PR**: Add user role management

**Code Changed**: New class `RoleManager` in user module

**Review Comment**:

FILE: user.py (lines 50-100)

New functionality added without adequate tests.

ISSUE: RoleManager public interface not tested

• add_role(user, role) - add success case
• remove_role(user, role) - add success case
• get_roles(user) - test empty, single, multiple
• is_admin(user) - test true/false cases

EDGE CASES MISSING:

• [ ] duplicate role assignment (should be idempotent)
• [ ] removing non-existent role (should not error)
• [ ] get_roles on user with no roles (should return empty)

CONTRACT VERIFICATION:

• [ ] Functions accept documented types
• [ ] Return types match documentation
• [ ] Exceptions raised as documented

ADD: tests/test_role_manager.py with 85%+ coverage

````

Example 5: Recognizing Good Design

PR: Add simplified config loader

Code Changed:

def load_config(path):
    """Load YAML config from file.

    Args:
        path: Path to config file

    Returns:
        dict: Parsed config

    Raises:
        FileNotFoundError: If file doesn't exist
        ValueError: If YAML is invalid
    """
    with open(path) as f:
        try:
            return yaml.safe_load(f)
        except yaml.YAMLError as e:
            raise ValueError(f"Invalid YAML: {e}") from e

Review Comment:

EXCELLENT - This is exactly the simplicity we want:

✅ Single responsibility: Load and parse YAML
✅ Clear error handling: Explicit exceptions with context
✅ No TODOs or stubs: Production ready
✅ Complete documentation: Clear args, returns, exceptions
✅ No over-engineering: Direct implementation
✅ Testable: Clear behavior to verify

This is a model example of ruthless simplicity.

Feedback Template

When commenting on PRs, use this structure:

**FILE**: path/to/file.py (lines X-Y)

**ISSUE**: [Principle violated - Simplicity/Modularity/Zero-BS/Tests/Docs]

**WHAT**: [Describe what's in the code]

**WHY IT'S PROBLEMATIC**: [How it violates amplihack principles]

**SUGGESTION**: [Concrete code example or approach]

**REFERENCE**: [Link to relevant philosophy, principle, or example]

Integration with GitHub

Posting Review Comments

The skill can post review comments to GitHub PRs using:

gh pr comment <PR-NUMBER> -b "Review comment here"
# Or for specific file reviews:
gh pr diff <PR-NUMBER> | grep "^---" | head -1
# Then post review with specific file:line references

Review Workflow

1. Fetch PR details: Get PR number, branch, changed files 2. Analyze changes: Run review against philosophy 3. Generate feedback: Compile specific, actionable comments 4. Post review: Create GitHub review with all comments 5. Summary: Post overall assessment

Common Over-Engineering Patterns to Catch

Pattern 1: Configuration Complexity

# OVER-ENGINEERED: 50-line config class
class ConfigManager:
    def __init__(self, env_file, schema_file, validators):
        self.config = load_yaml(env_file)
        self.schema = load_json(schema_file)
        self.validators = validators
        # 40 more lines...

# SIMPLE: 5 lines
config = yaml.safe_load(open('.env.yaml'))

Pattern 2: Factory Pattern When Not Needed

# OVER-ENGINEERED: Factory for single implementation
class ValidationFactory:
    def create_validator(self, type):
        if type == "email":
            return EmailValidator()
        # ... more types

# SIMPLE: Direct function
def validate_email(email):
    return "@" in email and "." in email

Pattern 3: Generic Base Classes for One Use

# OVER-ENGINEERED: Base class never subclassed
class BaseRepository(ABC):
    @abstractmethod
    def find(self, id): pass
    # ... 20 abstract methods

class UserRepository(BaseRepository):
    # Forced to implement all abstract methods
    # But only uses 3 of them

# SIMPLE: Direct class
class UserRepository:
    def find(self, id):
        return self.db.query(User).get(id)

Pattern 4: Premature Optimization

# OVER-ENGINEERED: Complex caching for cache that's not needed
cache = LRUCache(maxsize=1000)
stats = CacheStats()
lock = threading.Lock()
# ... complex logic

# SIMPLE: None - profile first, optimize if needed
result = function(args)

Key Questions to Ask

When reviewing, ask these questions:

1. Can this be simpler? If yes, why isn't it? 2. Is this necessary now? Or is it future-proofing? 3. What's the ONE thing this does? If there are many things, split it. 4. Who will use this? Is the interface clear for them? 5. What can go wrong? Are errors handled explicitly? 6. Is this testable? Can the contract be verified? 7. Will this need to change? Is it flexible without over-engineering? 8. Could this be deleted? Better than could it be refactored? 9. Does this follow our patterns? Or is it unique? 10. Am I confident this works? Or is it speculative?

Success Criteria

A successful PR review using this skill:

• [ ] Reviews code against amplihack philosophy, not just style
• [ ] Identifies over-engineering with concrete suggestions
• [ ] Verifies module structure and brick design
• [ ] Checks test coverage adequacy
• [ ] Provides specific file:line references
• [ ] Offers concrete, actionable suggestions
• [ ] Recognizes and acknowledges good design
• [ ] Posts comprehensive GitHub review comments
• [ ] Helps team learn and improve

Output

The skill produces:

1. Philosophy Compliance Report

• Ruthless Simplicity check
• Modular Architecture check
• Zero-BS Implementation check
• Test Coverage assessment
• Overall assessment

2. Specific Recommendations

• Over-engineering identified with examples
• Simplification suggestions with code
• Module structure feedback
• Test gaps to address

3. GitHub Comments (optional)

• Detailed review with file:line references
• Inline code suggestions
• Summary of findings
• Constructive, respectful tone

Philosophy References

All reviews anchor in these documents:

• ~/.amplihack/.claude/context/PHILOSOPHY.md - Core development philosophy
• ~/.amplihack/.claude/context/PATTERNS.md - Approved patterns and anti-patterns
• Specs/ - Module specifications for architecture verification
• ~/.amplihack/.claude/context/DISCOVERIES.md - Known issues and solutions

Tips for Effective Reviews

1. Be Specific: Reference exact lines and code 2. Explain Why: What principle is violated and why it matters 3. Suggest How: Provide concrete code examples 4. Respect Constraints: Some complexity may be necessary 5. Acknowledge Good Work: Praise what's done well 6. Ask Questions: "Have you considered?" invites discussion 7. Learn Together: Reviews are teaching opportunities 8. Iterate: Suggest improvements, don't demand perfection 9. Consider Context: External constraints matter 10. Stay Focused: Review philosophy alignment, not personal style

Related Skills and Workflows

• Module Spec Generator: Creates specifications for regeneratable modules
• Builder Agent: Implements code from specifications
• Reviewer Agent: Philosophy compliance verification
• Tester Agent: Test generation and validation
• Document-Driven Development: Uses specs as source of truth

Feedback and Evolution

This skill should evolve based on usage:

• What patterns do we keep finding?
• What suggestions lead to better code?
• What philosophy principles are most violated?
• How can we catch issues earlier?

Document learnings in ~/.amplihack/.claude/context/DISCOVERIES.md.

PR Review Assistant - Detailed Examples

Example 1: Over-Engineering with Unnecessary Abstraction

The PR

A PR adds a new feature to handle user notifications across multiple channels (email, SMS, push).

Original Code (Over-Engineered)

# notification/channel.py
from abc import ABC, abstractmethod
from typing import Protocol

class NotificationChannel(ABC):
    """Base class for all notification channels."""

    def send(self, message: str) -> bool:
        """Send notification through channel."""
        self.validate()
        result = self._send_internal(message)
        self.log_result(result)
        return result

    @abstractmethod
    def _send_internal(self, message: str) -> bool:
        pass

    def validate(self) -> None:
        """Validate channel configuration."""
        raise NotImplementedError()

    def log_result(self, result: bool) -> None:
        """Log sending result."""
        raise NotImplementedError()

class EmailChannel(NotificationChannel):
    def _send_internal(self, message: str) -> bool:
        return send_email(self.email, message)

    def validate(self) -> None:
        if not self.email:
            raise ValueError("Email required")

    def log_result(self, result: bool) -> None:
        logger.info(f"Email: {result}")

class SMSChannel(NotificationChannel):
    def _send_internal(self, message: str) -> bool:
        return send_sms(self.phone, message)

    def validate(self) -> None:
        if not self.phone:
            raise ValueError("Phone required")

    def log_result(self, result: bool) -> None:
        logger.info(f"SMS: {result}")

# Usage
channels = {
    "email": EmailChannel(email="user@example.com"),
    "sms": SMSChannel(phone="+1234567890"),
}

for channel in channels.values():
    channel.send("Hello!")

Review Comment

````markdown FILE: notification/channel.py

ISSUE: Over-engineering with unnecessary abstraction layers

WHAT: Multiple notification channels implemented using abstract base class pattern with template method. Each channel must implement validate(), \_send_internal(), log_result().

WHY IT'S PROBLEMATIC:

1. Abstraction not justified: Base class adds ceremony without clear benefit

• Each channel has completely different behavior
• Common behavior is just try->log pattern, which is trivial
• Template method pattern over-kills simple use case

2. Violation of YAGNI: You Aren't Gonna Need It

• No evidence we'll add 5+ channels justifying abstraction
• If we later add Slack, Telegram - that's 2 more. Still manageable without base class.

3. Implementation burden: Forces unnecessary methods

• validate() and log_result() must exist on every channel
• But they're not used consistently
• Subclasses forced to implement things they don't need

4. Harder to understand:

• Developer must understand inheritance hierarchy to find actual logic
• 50+ lines to implement what could be 10

SUGGESTION: Use simple direct functions

# notification/email.py
def send_email(to: str, message: str) -> bool:
    """Send email notification."""
    if not to:
        raise ValueError("Recipient email required")

    result = _send_via_smtp(to, message)
    logger.info(f"Email sent to {to}: {result}")
    return result

# notification/sms.py
def send_sms(phone: str, message: str) -> bool:
    """Send SMS notification."""
    if not phone:
        raise ValueError("Recipient phone required")

    result = _send_via_provider(phone, message)
    logger.info(f"SMS sent to {phone}: {result}")
    return result

# Usage - simple and clear
send_email("user@example.com", "Hello!")
send_sms("+1234567890", "Hello!")

````

BENEFITS:

• 60% fewer lines of code
• Clear what each function does
• No inheritance to understand
• Easy to test each function independently
• Still extensible if needed later

REFERENCE: Ruthless Simplicity principle - minimize abstractions

### Learning Points
- Abstract base classes for 2-3 implementations are usually over-engineering
- Template method pattern adds ceremony
- Direct functions are often clearer than inheritance hierarchies
- Start simple; add abstraction if needed for real cases (5+ similar items)

---

## Example 2: Missing Specification and Regeneration Docs

### The PR
A PR adds a new authentication module for JWT token handling.

### Structure

.claude/tools/auth/ ├── init.py ├── jwt_handler.py ├── tokens.py └── tests/ └── test_jwt.py

````

Review Comment

**FILE**: .claude/tools/auth/ (new module)

**ISSUE**: Module added without specification documentation

**WHAT**:
New authentication module with JWT token handling, validation, and refresh.
Module structure is clear and tests are comprehensive.

**WHY IT'S PROBLEMATIC**:

1. **No regeneration spec**: Module can't be rebuilt from documentation
   - Brick philosophy requires regeneratable modules
   - Future developers can't understand module contract without reading all code
   - If requirements change, no spec to update first

2. **Public interface unclear**: What's meant to be exported?
   - Is everything in __init__.py the public API?
   - What are internal utilities vs public functions?
   - Missing clear "studs" (connection points)

3. **Contract not documented**:
   - What exceptions can be raised?
   - What are the type requirements?
   - How do modules depending on this one use it?

**ACTION NEEDED**: Create Specs/authentication.md

Authentication Module Specification

Purpose

Handle JWT token creation, validation, and refresh for user authentication.

Scope

Handles: Token generation, validation, refresh logic Does NOT Handle: User management, password hashing, authorization

Public Interface (The "Studs")

Functions

• create_token(user_id: str, expires_in: int = 3600) -> str

Creates JWT token for user, valid for expires_in seconds Raises: ValueError if user_id is empty

• validate_token(token: str) -> dict

Validates token and returns decoded payload Returns: dict with user_id, created_at, expires_at Raises: ValueError if token invalid/expired, KeyError if signature wrong

• refresh_token(token: str) -> str

Creates new token from valid refresh token Raises: ValueError if token expired or invalid

Dependencies

External: PyJWT (2.8+) Internal: None

Test Requirements

• ✅ Valid token validates successfully
• ✅ Expired token raises ValueError
• ✅ Invalid signature raises KeyError
• ✅ Refresh creates new token with reset expiry
• ✅ Empty user_id raises ValueError

Example Usage

from auth import create_token, validate_token

# Create token for user
token = create_token("user_123")

# Later: validate token
payload = validate_token(token)
print(f"User: {payload['user_id']}")

Regeneration Notes

Module can be rebuilt from this spec while preserving:

• ✅ Public interface (all studs preserved)
• ✅ Dependencies (PyJWT, no internal deps)
• ✅ Error behavior (exceptions documented)
• ✅ Module structure (single responsibility)

**REFERENCE**: Brick philosophy - modules must be regeneratable

Benefits of Spec

1. Next developer knows what this module does without reading code 2. If requirements change, we update spec first 3. Builder agent can regenerate if issues found 4. Clear contracts prevent breaking changes 5. Tests can verify spec is implemented correctly

Learning Points

• Every new module needs a specification in Specs/
• Specs enable regeneration of modules
• Public interface must be clear ("studs")
• Specifications come BEFORE or WITH code, not after

---

Example 3: Zero-BS Issues - TODOs and Error Handling

The PR

A PR adds data validation pipeline for user input.

Original Code

# validation/pipeline.py

def validate_input(data: dict) -> dict:
    """Validate and transform user input.

    TODO: Add rate limiting
    """
    # Validate each field
    errors = {}

    # TODO: Implement comprehensive field validation
    for field in ["email", "password", "name"]:
        if field not in data:
            # This is a problem - error is silently ignored
            pass

    # Transform data
    try:
        result = transform_data(data)
    except Exception:
        # Swallowed exception - what went wrong?
        return None

    # TODO: Add audit logging
    return result

def transform_data(data: dict) -> dict:
    """Transform data fields."""
    # Complex transformation logic...
    # NotImplementedError will be added later
    raise NotImplementedError("Schema validation to be implemented")

Review Comment

````markdown FILE: validation/pipeline.py

ISSUE: Zero-BS violations - TODOs, swallowed errors, unimplemented code

WHAT: Input validation pipeline with multiple incomplete implementations, TODOs, and error handling that swallows exceptions.

PROBLEMS FOUND:

1. LINE 6: TODO in code

• Rate limiting comment suggests incomplete feature
• Either implement it now or file an issue - don't leave TODO in code
• Deployed code with TODOs clutters codebase

2. LINE 12-14: Swallowed error (silent failure)

• Missing required field is not reported
• Returns dict without fields instead of raising error
• Caller doesn't know validation failed

CURRENT:

   if field not in data:
       pass  # Silent - bad!

````

SHOULD BE:

if field not in data:
    raise ValueError(f"Required field missing: {field}")

3. LINE 19-21: Caught exception with no context

• Exception is caught but result is None
• Caller doesn't know what failed
• Impossible to debug

CURRENT:

   try:
       result = transform_data(data)
   except Exception:
       return None  # What failed?

SHOULD BE:

   try:
       result = transform_data(data)
   except Exception as e:
       raise ValueError(f"Data transformation failed: {e}") from e

4. LINE 23: TODO for audit logging

• Another incomplete feature
• Either implement or document in requirements

5. LINE 32: NotImplementedError

• This should NOT be in code
• Either implement the schema validation or remove it
• NotImplementedError breaks at runtime

SUGGESTION: Zero-BS implementation

def validate_input(data: dict) -> dict:
    """Validate and transform user input.

    Args:
        data: Input dictionary with email, password, name

    Returns:
        Validated and transformed data

    Raises:
        ValueError: If required field missing or data invalid
    """
    # Validate required fields
    required = ["email", "password", "name"]
    for field in required:
        if field not in data:
            raise ValueError(f"Required field missing: {field}")

    # Transform data
    try:
        result = transform_data(data)
    except Exception as e:
        raise ValueError(f"Data transformation failed: {e}") from e

    return result

def transform_data(data: dict) -> dict:
    """Transform and validate data schema.

    Raises:
        ValueError: If schema validation fails
    """
    # Actual schema validation implementation
    # Raise errors with clear messages
    ...

KEY CHANGES:

• ✅ No TODOs - either implement or don't include
• ✅ Errors are explicit and visible
• ✅ Clear error messages for debugging
• ✅ All functions are working implementations
• ✅ Caller knows what succeeded/failed

REFERENCE: Zero-BS Implementation principle

````

Learning Points

• TODO comments in code = incomplete work - don't merge
• Swallowed exceptions = impossible to debug
• None returns for errors = ambiguous
• NotImplementedError should only be in abstract base classes
• Every function must be production-ready

---

Example 4: Missing Test Coverage and Module Contract

The PR

A PR adds a new caching layer for database queries.

Original Code

# caching/cache.py

class QueryCache:
    """Simple query result cache."""

    def __init__(self, ttl: int = 300):
        self.ttl = ttl
        self.cache = {}

    def get(self, key: str):
        """Get cached value."""
        if key in self.cache:
            entry = self.cache[key]
            if time.time() - entry["time"] < self.ttl:
                return entry["value"]
            else:
                del self.cache[key]
        return None

    def set(self, key: str, value) -> None:
        """Set cache value."""
        self.cache[key] = {
            "value": value,
            "time": time.time()
        }

    def clear(self) -> None:
        """Clear all cache."""
        self.cache.clear()

Review Comment

````markdown FILE: caching/cache.py

ISSUE: No test coverage for public interface

WHAT: New QueryCache class with get/set/clear interface. No tests in PR.

WHAT'S MISSING:

Test Coverage Gaps

Tests should verify public contract:

Basic Functionality (MISSING)

• [ ] get() returns None for unknown keys
• [ ] set() stores values
• [ ] get() retrieves stored values
• [ ] get() returns None after TTL expires
• [ ] clear() removes all cached items

Edge Cases (MISSING)

• [ ] get() with empty string key
• [ ] set() with None value
• [ ] get() called immediately after set()
• [ ] Multiple set() calls to same key (overwrites)
• [ ] clear() on empty cache

Contract Verification (MISSING)

• [ ] set() returns None (as documented)
• [ ] get() always returns value or None
• [ ] TTL works correctly (test with actual time)

Concurrent Access (MISSING)

• [ ] Multiple threads accessing cache simultaneously
• [ ] Race condition if item expires during get()

ACTION NEEDED: Create tests/test_cache.py

import pytest
import time
from caching.cache import QueryCache

class TestQueryCache:
    def setup_method(self):
        """Create fresh cache for each test."""
        self.cache = QueryCache(ttl=1)

    def test_get_missing_key_returns_none(self):
        """Getting non-existent key returns None."""
        assert self.cache.get("unknown") is None

    def test_set_and_get_value(self):
        """Setting value stores it and get retrieves it."""
        self.cache.set("key", "value")
        assert self.cache.get("key") == "value"

    def test_get_returns_none_after_ttl_expiry(self):
        """Value expires after TTL seconds."""
        self.cache.set("key", "value")
        time.sleep(1.1)  # Wait for expiry
        assert self.cache.get("key") is None

    def test_get_returns_value_before_ttl_expiry(self):
        """Value available before TTL expires."""
        self.cache.set("key", "value")
        time.sleep(0.5)  # Before expiry
        assert self.cache.get("key") == "value"

    def test_clear_removes_all(self):
        """Clear removes all cached items."""
        self.cache.set("key1", "value1")
        self.cache.set("key2", "value2")
        self.cache.clear()
        assert self.cache.get("key1") is None
        assert self.cache.get("key2") is None

    def test_set_with_none_value(self):
        """Storing None as value works correctly."""
        self.cache.set("key", None)
        # Should distinguish between "not cached" and "cached as None"
        assert self.cache.get("key") is None  # Or should this be different?

    def test_overwrite_existing_key(self):
        """Setting same key twice overwrites."""
        self.cache.set("key", "value1")
        self.cache.set("key", "value2")
        assert self.cache.get("key") == "value2"

    def test_set_returns_none(self):
        """set() returns None as documented."""
        result = self.cache.set("key", "value")
        assert result is None

    def test_clear_returns_none(self):
        """clear() returns None as documented."""
        result = self.cache.clear()
        assert result is None

````

COVERAGE TARGET: 85%+

EDGE CASE ISSUE FOUND: The current implementation can't distinguish between:

• "Value not in cache" → get() returns None
• "Value cached as None" → get() returns None

This ambiguity could be a problem. Consider using a different approach:

def get(self, key: str, default=_NOT_FOUND):
    """Get cached value, with default if not found or expired."""
    if key in self.cache:
        entry = self.cache[key]
        if time.time() - entry["time"] < self.ttl:
            return entry["value"]
        else:
            del self.cache[key]
    return default

This makes the contract clearer.

REFERENCE: Test coverage of public interface

````

Learning Points

• Every public function needs tests
• Test edge cases and error conditions
• Verify return types match documentation
• Consider ambiguous cases (None as value vs missing)
• Tests define the contract - verify it works as documented

---

Example 5: Recognizing Good Design

The PR

A PR adds a simple configuration loader.

Code

# config/loader.py

def load_config(path: str) -> dict:
    """Load YAML configuration from file.

    Args:
        path: Path to YAML configuration file

    Returns:
        Configuration as dictionary

    Raises:
        FileNotFoundError: If config file doesn't exist
        ValueError: If YAML syntax is invalid
    """
    try:
        with open(path) as f:
            return yaml.safe_load(f)
    except FileNotFoundError:
        raise
    except yaml.YAMLError as e:
        raise ValueError(f"Invalid YAML in {path}: {e}") from e

def merge_configs(base: dict, override: dict) -> dict:
    """Merge override config into base config.

    Args:
        base: Base configuration dictionary
        override: Configuration to merge in

    Returns:
        Merged configuration with override values taking precedence
    """
    result = base.copy()
    result.update(override)
    return result

Test Code

# tests/test_config.py

import pytest
from config.loader import load_config, merge_configs

class TestLoadConfig:
    def test_load_valid_yaml(self, tmp_path):
        """Load valid YAML returns parsed dict."""
        config_file = tmp_path / "config.yaml"
        config_file.write_text("key: value\nnumber: 42")

        result = load_config(str(config_file))
        assert result == {"key": "value", "number": 42}

    def test_missing_file_raises_error(self):
        """Missing file raises FileNotFoundError."""
        with pytest.raises(FileNotFoundError):
            load_config("/nonexistent/path.yaml")

    def test_invalid_yaml_raises_error(self, tmp_path):
        """Invalid YAML raises ValueError."""
        config_file = tmp_path / "config.yaml"
        config_file.write_text("key: value\n  - invalid indent")

        with pytest.raises(ValueError, match="Invalid YAML"):
            load_config(str(config_file))

class TestMergeConfigs:
    def test_merge_with_override(self):
        """Override values take precedence."""
        base = {"a": 1, "b": 2}
        override = {"b": 99}

        result = merge_configs(base, override)
        assert result == {"a": 1, "b": 99}

    def test_merge_adds_new_keys(self):
        """New keys from override are added."""
        base = {"a": 1}
        override = {"b": 2}

        result = merge_configs(base, override)
        assert result == {"a": 1, "b": 2}

    def test_merge_preserves_base(self):
        """Original base dict not modified."""
        base = {"a": 1}
        override = {"b": 2}

        merge_configs(base, override)
        assert base == {"a": 1}  # Unchanged

Review Comment

**FILE**: config/loader.py

**ASSESSMENT**: Excellent example of ruthless simplicity

✅ **Ruthless Simplicity**

- Single responsibility: load YAML, merge configs
- Direct implementation, no unnecessary abstractions
- No over-parameterization
- Clear what each function does

✅ **Zero-BS Implementation**

- No TODOs or stubs
- Error handling is explicit (FileNotFoundError, ValueError)
- Clear error messages for debugging
- Production-ready code

✅ **Clear Contracts**

- Documented args, returns, and exceptions
- Type hints present
- Users know exactly what to expect

✅ **Test Coverage**

- Public interface fully tested
- Edge cases covered (missing file, invalid YAML)
- Original data not modified verified
- 90%+ coverage

✅ **Module Structure**

- Single responsibility
- No unnecessary configuration classes
- Easy to use and extend
- Could be regenerated from specification

**LEARNING POINTS**:

- This is the simplicity we aim for
- Sometimes the best code is the straightforward code
- Clear documentation + good tests = maintainable
- No need for frameworks when direct approach works

**READY TO MERGE**: All principles aligned, tests comprehensive.

Why This is Good Design

1. Minimal code: Does exactly what's needed, nothing more 2. Clear errors: You know what failed and why 3. Testable: Easy to verify behavior 4. Documented: Clear contracts and examples 5. No over-engineering: Direct approach without abstraction 6. Regeneratable: Could be rebuilt from specification

---

Summary: Key Patterns

What to Catch and Fix

1. Over-abstraction → Suggest direct functions/classes 2. TODOs in code → Implement or remove 3. Swallowed errors → Make explicit with context 4. Missing tests → Specify test requirements 5. No specs → Request Specs/ documentation 6. Future-proofing → Start simple, extend when needed

What to Praise

1. Ruthless simplicity → Direct implementations 2. Clear contracts → Good documentation and types 3. Good tests → Coverage of edge cases 4. Explicit errors → Users know what failed 5. Module specs → Regeneratable modules

Philosophy Alignment

Every review should anchor in:

• Ruthless Simplicity: Question every line
• Modular Design: Clear boundaries and contracts
• Zero-BS Implementation: Production-ready, no shortcuts
• Quality Over Speed: Long-term maintainability
• Brick Philosophy: Modules should be regeneratable

PR Review Assistant - Quick Start Guide

What is This Skill?

A Claude Code skill that reviews pull requests against amplihack's core philosophy:

• Ruthless Simplicity: Every line must justify its existence
• Modular Architecture: Clear contracts and boundaries (Brick & Studs)
• Zero-BS Implementation: Production-ready code, no shortcuts
• Quality Over Speed: Long-term maintainability over quick wins

When to Use

"Review this PR for philosophy alignment"
"Check if this module follows brick design"
"Is there over-engineering in this PR?"
"Suggest simplifications for these changes"

How to Use

Option 1: Review a PR (GitHub)

Claude, review PR #123 against our philosophy.
Focus on:
- Over-engineering patterns
- Module structure
- Test coverage

Claude will:

1. Analyze PR changes 2. Check philosophy alignment 3. Identify issues with specific file:line references 4. Suggest concrete improvements 5. Post review comments to GitHub

Option 2: Review Code Files

Claude, review these changes in user_service.py
for ruthless simplicity and over-engineering.

Claude will:

1. Read the file 2. Assess against philosophy 3. Identify issues 4. Suggest simplifications 5. Explain why changes align (or don't) with principles

Option 3: Analyze Module Structure

Claude, verify that this authentication module
follows brick design principles.

Claude will:

1. Check public interface clarity 2. Verify single responsibility 3. Assess module boundaries 4. Suggest improvements 5. Recommend specification documentation

What Gets Reviewed

1. Ruthless Simplicity Check

✓ Every function has ONE clear purpose
✓ No unnecessary abstractions or indirection
✓ No future-proofing or speculation
✓ No duplicate logic
✓ Clear, self-documenting names
✓ Minimal parameters and branching

Common Issues Found:

• Base classes for 2-3 implementations (unnecessary)
• Configuration frameworks that aren't needed
• Factory patterns for single implementations
• Premature optimization
• Generic "helper" functions

2. Modular Architecture Check

✓ Module has ONE clear responsibility
✓ Public interface is minimal and clear
✓ Internal utilities properly isolated
✓ Dependencies are explicit
✓ No circular dependencies
✓ Clear contracts at boundaries

Common Issues Found:

• Unclear what's public vs private
• Dependencies not documented
• Modules doing too many things
• No specification for regeneration
• Missing tests for module contract

3. Zero-BS Implementation Check

✓ No TODOs or NotImplementedError in production code
✓ No mock/test data in production
✓ No dead code or unused imports
✓ Error handling is explicit
✓ All functions are working implementations
✓ No swallowed exceptions
✓ Clear error messages

Common Issues Found:

• TODO comments in code
• Functions that return None on error
• Caught exceptions with no context
• Code paths that can't happen
• Silent failures

4. Test Coverage Check

✓ Public interface has tests
✓ Edge cases covered
✓ Error conditions tested
✓ Integration points verified
✓ Coverage adequate (85%+)
✓ Tests verify contract, not implementation

Common Issues Found:

• No tests for new public functions
• Edge cases not covered
• Error paths not tested
• Coverage below 85%
• Tests that only verify implementation

5. Documentation Check

✓ Docstrings are clear and complete
✓ Public interface documented
✓ Examples provided for new features
✓ Type hints present and accurate
✓ Module README updated if needed
✓ Specification created for new modules

Common Issues Found:

• Missing docstrings
• No type hints
• Vague error documentation
• No usage examples
• No module specification

Review Output

The skill produces:

1. Compliance Report

RUTHLESS SIMPLICITY: ✓ PASS
- Code is straightforward with no unnecessary abstractions
- Each function has single clear purpose
- No over-parameterization

MODULAR ARCHITECTURE: ✓ PASS
- Module has clear single responsibility
- Public interface is minimal
- Dependencies are explicit

ZERO-BS IMPLEMENTATION: ⚠ ISSUES FOUND
- 2 TODO comments (lines 45, 67)
- 1 swallowed exception (line 52)

TEST COVERAGE: ✗ NEEDS WORK
- Missing tests for validate_token()
- No edge case coverage

DOCUMENTATION: ⚠ PARTIAL
- Module spec missing (required for new modules)
- Good docstrings on new functions

OVERALL: NEEDS IMPROVEMENTS BEFORE MERGE

2. Specific Recommendations

FILE: auth/token.py (line 45)

ISSUE: TODO comment in production code
"# TODO: Add rate limiting"

SUGGESTION: Either implement it or file an issue.
TODO in code = incomplete work. Don't merge.

---

FILE: auth/token.py (line 52)

ISSUE: Swallowed exception

CURRENT CODE:
try:
    result = validate_signature(token)
except Exception:
    return None

PROBLEM: Caller doesn't know why validation failed.
Impossible to debug.

SUGGESTION:
try:
    result = validate_signature(token)
except SignatureError as e:
    raise ValueError(f"Invalid token: {e}") from e

3. GitHub Review (Optional)

If reviewing a GitHub PR, creates detailed review:

**Philosophy Compliance Review**

This PR has good structure and clear functionality.
Before merge, address these items:

**Issues Found:**
- 2 TODO comments (production code should be complete)
- 1 swallowed exception (makes debugging impossible)
- Missing test for edge case
- Module spec missing (required for brick design)

**Suggestions:**
- See inline comments for specific code
- Create Specs/authentication.md per template
- Add test for expired token case

**Strengths:**
- Clear module structure
- Good error handling overall
- Comprehensive docstrings
- Type hints throughout

Common Review Scenarios

Scenario 1: Over-Engineering

DETECTED: Unnecessary abstraction
- Abstract base class for 2 implementations
- Template method pattern adds complexity

SUGGESTION: Use simple direct classes
- Each class is 10 lines, not 20 in hierarchy
- Easier to understand and test

Scenario 2: Missing Tests

DETECTED: New public function without tests
- check_permission(user, resource) → bool
- No tests for true/false cases
- No tests for edge cases

ACTION: Add tests/test_permissions.py
- ✓ Valid user → True
- ✓ Invalid user → False
- ✓ Empty user string
- ✓ Null resource

Scenario 3: TODOs in Code

DETECTED: TODO comment at line 34
"# TODO: Add caching"

PROBLEM: Incomplete code shouldn't be merged

SOLUTION:
Option A: Implement the caching now
Option B: Remove TODO, file issue for future
Option C: Remove the comment and the commented-out code

Scenario 4: No Module Spec

DETECTED: New module without specification

REQUIRED: Create Specs/module-name.md

Template includes:
- Purpose (one sentence)
- Public interface (exported functions/classes)
- Dependencies (external and internal)
- Test requirements
- Example usage

This enables module regeneration in future.

How to Respond to Feedback

If You Agree

Claude, I agree. Let me fix these issues:
1. Remove TODOs and implement properly
2. Add test for edge case
3. Create module specification

If You Disagree

Claude, I disagree about the abstraction.
Here's why it's necessary:
[explain context]

Is there a simpler approach I'm missing?

If You Have Questions

Claude, I'm not sure about this suggestion.
Can you explain:
1. What principle is violated?
2. Why is the simpler approach better?
3. Are there cases where my approach makes sense?

Philosophy References

All reviews anchor in these principles:

• Ruthless Simplicity: The solution should be as simple as possible, but no simpler
• Brick Architecture: Self-contained modules with clear connection points
• Zero-BS Implementation: Production-ready code, no shortcuts or TODOs
• Quality Over Speed: Long-term maintainability over quick implementation
• Regeneratable Modules: Any module can be rebuilt from its specification

See ~/.amplihack/.claude/context/PHILOSOPHY.md for full philosophy.

Tips for Better Reviews

1. Ask Questions: "Why do we need this abstraction?" invites dialogue 2. Provide Examples: Show simpler code alongside your suggestion 3. Acknowledge Good Work: "I like how you handled errors here" 4. Be Specific: Reference exact line numbers and code 5. Explain Why: Help reviewee understand the principle, not just the rule 6. Suggest Alternatives: Offer multiple approaches, not just one "right" way 7. Learn Together: Reviews are teaching opportunities

What Happens After Review

1. Issues identified: Developer reviews feedback 2. Discussion: Can ask questions or explain context 3. Updates made: Code is revised based on feedback 4. Re-review: Verify changes address concerns 5. Approval: If philosophy aligned, ready to merge

Common Questions

Q: Does this check syntax and style? A: No. Use linters for style. This checks philosophy alignment.

Q: Can I ignore feedback? A: You can, but philosophy is the development foundation. Consider why.

Q: What if my code violates philosophy? A: Discuss it. Sometimes constraints require trade-offs. Let's talk.

Q: Can I request a re-review? A: Yes, after making changes, ask Claude to re-review specific parts.

Q: Is this used for every PR? A: Recommended for significant changes, especially new modules or refactoring.

Next Steps

1. Request a review: Ask Claude to review a PR or code 2. Read feedback: Understand what's suggested and why 3. Ask questions: Clarify anything you don't understand 4. Iterate: Improve code based on feedback 5. Merge: When philosophy aligned, ready to go

See Also

• SKILL.md - Complete skill documentation
• EXAMPLES.md - Detailed review examples
• ~/.amplihack/.claude/context/PHILOSOPHY.md - Core development philosophy
• Specs/ - Module specifications directory
• ~/.amplihack/.claude/context/DISCOVERIES.md - Known issues and solutions

PR Review Assistant Skill

Philosophy-aware pull request reviews that check alignment with amplihack principles. Use when reviewing PRs to ensure ruthless simplicity, modular design, and zero-BS implementation. Suggests simplifications, identifies over-engineering, verifies brick module structure.

Files in This Skill

• SKILL.md - Complete skill documentation with full review process and examples
• QUICK_START.md - Getting started guide with common scenarios
• EXAMPLES.md - Detailed examples of reviews with feedback
• REVIEW_CHECKLIST.md - Complete checklist for reviewing code
• README.md - This file

Quick Start

Basic Usage

Claude, review this PR for philosophy alignment:
- Check for over-engineering
- Verify module structure
- Suggest simplifications

Claude will:

1. Analyze the code 2. Check against amplihack principles 3. Identify issues with specific file:line references 4. Suggest concrete improvements 5. Explain alignment with philosophy

Review Focuses

This skill reviews for:

1. Ruthless Simplicity

• Is every line necessary?
• Are there unnecessary abstractions?
• Could this be simpler?

2. Modular Architecture (Brick & Studs)

• Does the module have ONE clear responsibility?
• Are public contracts clear?
• Are module boundaries well-defined?

3. Zero-BS Implementation

• No TODOs in production code
• All functions are working implementations
• Error handling is explicit
• No swallowed exceptions

4. Test Coverage

• Public interface tested
• Edge cases covered
• Error paths tested
• 85%+ coverage

5. Documentation

• Clear docstrings and type hints
• Module specifications for new modules
• Usage examples

Review Process

Step 1: Request a Review

Claude, review this code for philosophy alignment.

Or:

Claude, check this PR (#123) against our development philosophy.

Step 2: Understand the Feedback

Claude provides:

• Compliance Report - How code aligns with each principle
• Specific Issues - Problems with file:line references
• Concrete Suggestions - Code examples showing improvements
• GitHub Review - Optional detailed review as comments

Step 3: Address Feedback

Review the suggestions and:

• Implement improvements suggested
• Ask questions if clarification needed
• Explain context if you disagree

Step 4: Re-Review

Claude, I've addressed the issues. Can you re-review?

Step 5: Merge

Once philosophy aligned, ready to merge!

Common Patterns to Catch

Over-Engineering

DETECTED: Unnecessary abstraction
ISSUE: Base class for 2 implementations
SUGGESTION: Use simple direct classes

Missing Tests

DETECTED: New public function untested
ISSUE: validate_token() has no tests
SUGGESTION: Add tests for true/false/edge cases

TODOs in Code

DETECTED: TODO comment at line 34
ISSUE: Incomplete work shouldn't be merged
SOLUTION: Implement it or remove the comment

Module Issues

DETECTED: New module without specification
ISSUE: Module can't be regenerated
ACTION: Create Specs/module-name.md

Example Review Output

RUTHLESS SIMPLICITY: ✓ PASS
- Code is straightforward
- Each function has single purpose
- No over-abstraction

MODULAR ARCHITECTURE: ✓ PASS
- Clear single responsibility
- Public interface minimal
- Dependencies explicit

ZERO-BS IMPLEMENTATION: ⚠ NEEDS WORK
- 2 TODO comments (lines 45, 67)
- 1 swallowed exception (line 52)

TEST COVERAGE: ✗ GAPS
- Missing edge case tests
- Coverage at 72%

DOCUMENTATION: ⚠ PARTIAL
- Module spec missing
- Good docstrings overall

VERDICT: NEEDS IMPROVEMENTS BEFORE MERGE

When to Use

• New features: Verify design before big investment
• Refactoring: Ensure simplification actual, not more complexity
• New modules: Check brick design and specifications
• Bug fixes: Verify error handling and tests
• API changes: Check for over-engineering or missing documentation
• Architecture changes: Verify against philosophy principles

Philosophy References

All reviews anchor in:

1. Ruthless Simplicity - Every line must justify its existence 2. Brick Philosophy - Self-contained, regeneratable modules 3. Zero-BS Implementation - Production-ready, no shortcuts 4. Quality Over Speed - Long-term maintainability 5. Modular Design - Clear boundaries and contracts

See ~/.amplihack/.claude/context/PHILOSOPHY.md for complete philosophy.

Acceptance Criteria for Code

Before Merge

• [ ] Ruthless Simplicity: ✓ PASS
• [ ] Modular Architecture: ✓ PASS
• [ ] Zero-BS Implementation: ✓ PASS
• [ ] Test Coverage: 85%+
• [ ] Documentation: Complete
• [ ] No blocking philosophy issues

Tips for Better Results

1. Be Specific: Ask about specific code sections 2. Provide Context: Explain what the code is doing 3. Ask Questions: "Why would this be better?" invites dialogue 4. Listen to Feedback: There's usually wisdom in the suggestions 5. Iterate: Multiple review rounds are normal

How the Skill Works

The skill:

1. Understands Context - Reads code and PR information 2. Analyzes Philosophy Alignment - Checks against 5 core principles 3. Identifies Issues - Finds problems with specific references 4. Suggests Improvements - Provides concrete examples 5. Explains Rationale - Teaches why changes align with philosophy 6. Posts Review - Optional GitHub review comments

Learning Outcomes

Through this skill, you learn:

• What constitutes "ruthless simplicity"
• How to design modules with clear boundaries
• Why zero-BS implementation matters
• What makes tests adequate
• How to recognize over-engineering
• How to write reviewable code

Common Questions

Q: Does this check Python style? A: No. Use linters for style (black, flake8). This checks philosophy.

Q: Can I disagree with feedback? A: Absolutely! That's how we improve. Explain your reasoning and let's discuss.

Q: What if I think the code is fine? A: You might be right. Ask Claude to explain the principle and see if you learn something.

Q: How often should I use this? A: For significant changes. Small bug fixes might not need a full review.

Q: Can I request re-review? A: Yes, after making changes: "Re-review my changes to see if issues are addressed"

Q: What if I don't understand feedback? A: Ask Claude to explain more clearly or provide additional examples.

See Also

• SKILL.md - Complete documentation with review process
• QUICK_START.md - Getting started in 5 minutes
• EXAMPLES.md - Real review examples with before/after
• REVIEW_CHECKLIST.md - Printable checklist for reviews
• .claude/context/PHILOSOPHY.md - Core development philosophy
• Specs/ - Module specifications directory

Feedback

This skill evolves based on usage:

• What patterns do we keep finding?
• What suggestions lead to better code?
• What's missing from the review process?
• How can we make this more helpful?

Share learnings in ~/.amplihack/.claude/context/DISCOVERIES.md.

Getting Started

1. Pick a PR or code file to review 2. Ask Claude: "Review this code for philosophy alignment" 3. Read the feedback and understand why 4. Implement improvements 5. Ask for re-review 6. Merge when philosophy aligned

That's it! Philosophy-aware code review in action.

PR Review Checklist

Use this checklist when reviewing code for philosophy alignment.

Pre-Review

• [ ] Understand PR purpose and scope
• [ ] Identify affected modules and files
• [ ] Note any dependencies added or removed
• [ ] Read PR description and context

Ruthless Simplicity

Code Complexity

• [ ] Each function has single, clear purpose
• [ ] Functions are easy to understand in one read
• [ ] No excessive nesting or branching
• [ ] No duplicate or similar logic
• [ ] No unnecessary helper functions

Abstractions

• [ ] No unnecessary base classes or inheritance
• [ ] No factory patterns for single implementation
• [ ] No generic frameworks built for hypothetical needs
• [ ] No template method patterns over-killing simple cases
• [ ] Interfaces justified by actual use

Parameters and Configuration

• [ ] Functions aren't over-parameterized (< 5 params)
• [ ] No configuration objects when simple args work
• [ ] Default values provided where sensible
• [ ] No boolean flags creating code paths

Names and Clarity

• [ ] Variable names are self-documenting
• [ ] Function names describe what they do
• [ ] Class names describe responsibility
• [ ] Avoid cryptic abbreviations
• [ ] Comments explain WHY, not WHAT

Future-Proofing

• [ ] No "we might need this someday" code
• [ ] Features aren't speculative
• [ ] Current needs met, not hypothetical ones
• [ ] Extensibility comes through clear design, not speculation

Modular Architecture (Brick & Studs)

Module Responsibility

• [ ] Module has ONE clear responsibility
• [ ] Module name describes what it does
• [ ] Responsibilities are explicit
• [ ] No "utility" modules doing everything
• [ ] Clear why this module exists

Public Interface

• [ ] Exports are clear and minimal
• [ ] __all__ defined or obvious
• [ ] Public functions documented
• [ ] Clear what's meant for external use
• [ ] No private functions in public interface

Internal Organization

• [ ] Internal utilities isolated
• [ ] Internal modules prefixed with underscore
• [ ] Clear separation of concerns
• [ ] Related code grouped together
• [ ] No internal details leaked outside

Dependencies

• [ ] All external dependencies listed
• [ ] All internal dependencies explicit
• [ ] No circular dependencies
• [ ] Dependencies are justified
• [ ] Version constraints specified

Module Structure

• [ ] Files organized logically
• [ ] Tests co-located with module
• [ ] Examples provided
• [ ] README or specification exists
• [ ] Module can be understood independently

Zero-BS Implementation

Code Completeness

• [ ] No TODO comments in code
• [ ] No FIXME or HACK comments (except in issues)
• [ ] No NotImplementedError (except abstract classes)
• [ ] No stubbed-out functions
• [ ] All functions fully implemented

Production Readiness

• [ ] No mock or test data in production code
• [ ] No commented-out code blocks
• [ ] No dead code or unused imports
• [ ] No debug print statements
• [ ] No logging from every function

Error Handling

• [ ] Errors explicitly handled
• [ ] No swallowed exceptions
• [ ] No silent failures
• [ ] Error messages are clear
• [ ] Exception type is specific (not just Exception)

Visibility

• [ ] Errors are visible during development
• [ ] Problems don't hide until production
• [ ] Debugging information available
• [ ] Clear what happened when things fail
• [ ] Stack traces are preserved (use from e)

Test Coverage

Public Interface Testing

• [ ] Public functions have tests
• [ ] Test happy path (expected behavior)
• [ ] Test error cases (raises correct exceptions)
• [ ] Test boundary conditions
• [ ] Test empty/null inputs

Edge Cases

• [ ] Empty lists/strings tested
• [ ] None/null values tested
• [ ] Max/min values tested
• [ ] Boundary conditions tested
• [ ] Invalid input tested

Error Paths

• [ ] Each raised exception tested
• [ ] Error messages verified
• [ ] Exception type verified
• [ ] Context preserved (not just Exception)

Contract Verification

• [ ] Return types match documentation
• [ ] Raised exceptions match documentation
• [ ] Accepted types match documentation
• [ ] Documentation matches actual behavior

Coverage and Quality

• [ ] Coverage adequate (85%+)
• [ ] Critical paths fully tested
• [ ] Tests are independent
• [ ] Tests use realistic data
• [ ] Tests document behavior

Integration Testing

• [ ] Module connections verified
• [ ] Dependencies called correctly
• [ ] Data flows through layers
• [ ] External service calls appropriate

Documentation

Docstrings

• [ ] All public functions documented
• [ ] Clear one-line summary
• [ ] Args section complete with types
• [ ] Returns section describes output
• [ ] Raises section lists exceptions
• [ ] Examples provided for complex functions

Type Hints

• [ ] Type hints present
• [ ] Types are accurate
• [ ] Return types specified
• [ ] Optional types use Optional[]
• [ ] Avoid Any unless necessary

Comments

• [ ] Comments explain WHY, not WHAT
• [ ] Complex logic is explained
• [ ] Non-obvious decisions noted
• [ ] Links to related issues/docs

Module Documentation

• [ ] Module purpose clear
• [ ] Public interface documented
• [ ] Dependencies listed
• [ ] Example usage provided
• [ ] For new modules: spec created in Specs/

README Updates

• [ ] Module README updated if needed
• [ ] New features documented
• [ ] Breaking changes noted
• [ ] Migration guide for changes
• [ ] Usage examples updated

New Modules (Additional Checks)

Module Specification

• [ ] Specs/module-name.md created
• [ ] Purpose section complete
• [ ] Public interface documented
• [ ] Dependencies listed
• [ ] Test requirements defined
• [ ] Example usage included

Brick Design

• [ ] Module is regeneratable from spec
• [ ] Public contracts (studs) defined
• [ ] Module boundaries clear
• [ ] Single responsibility
• [ ] Can be rebuilt independently

Structure

• [ ] Consistent with existing modules
• [ ] init.py exports clear
• [ ] core.py has main logic
• [ ] models.py for data structures
• [ ] utils.py for internal utilities
• [ ] tests/ with comprehensive tests

Refactoring Changes (Additional Checks)

No Regressions

• [ ] Tests still pass
• [ ] No public interface changes
• [ ] Behavior unchanged
• [ ] Performance not degraded
• [ ] Error handling preserved

Simplification

• [ ] Complexity reduced
• [ ] Clarity improved
• [ ] Lines of code decreased
• [ ] Easier to understand
• [ ] Maintenance easier

Cleanup

• [ ] Dead code removed
• [ ] Unused imports removed
• [ ] No new technical debt
• [ ] Deprecation handled properly

Breaking Changes (If Any)

• [ ] Clearly marked breaking
• [ ] Deprecation path provided
• [ ] Migration guide included
• [ ] Affected modules identified
• [ ] Version number updated

Review Decision

Ready to Merge

• [ ] Ruthless Simplicity: ✓ PASS
• [ ] Modular Architecture: ✓ PASS
• [ ] Zero-BS Implementation: ✓ PASS
• [ ] Test Coverage: ✓ ADEQUATE (85%+)
• [ ] Documentation: ✓ COMPLETE
• [ ] No blocking issues remaining

Needs Improvements

List specific issues to address:

1. [ ] Issue 1 2. [ ] Issue 2 3. [ ] Issue 3

Conditional Approval

• [ ] Approve with changes needed (non-blocking)
• [ ] Changes needed before merge (blocking)
• [ ] Specific items required

Post-Review Actions

• [ ] Feedback provided in clear comments
• [ ] Specific file:line references included
• [ ] Suggestions are concrete, not vague
• [ ] Respectful and constructive tone
• [ ] Learning opportunity highlighted
• [ ] Good work acknowledged

Learning Points

After this review, document:

• [ ] What pattern did we see?
• [ ] What principle was violated?
• [ ] How do we prevent this?
• [ ] Update DISCOVERIES.md if needed?
• [ ] Should we create tooling/linting for this?

Reviewer Notes

Space for specific findings and observations:

[Review notes here]

---

Quick Reference: Common Issues

Over-Engineering

• [ ] Unnecessary abstraction layers
• [ ] Premature optimization
• [ ] Configuration complexity
• [ ] Generic "framework" building
• [ ] Feature flags for non-existent features

Missing Tests

• [ ] New public functions untested
• [ ] Edge cases uncovered
• [ ] Error paths not tested
• [ ] Coverage below 85%

Zero-BS Violations

• [ ] TODO comments
• [ ] Swallowed exceptions
• [ ] Silent failures (None returns)
• [ ] NotImplementedError in non-abstract code
• [ ] Dead code

Module Issues

• [ ] No specification document
• [ ] Unclear public interface
• [ ] Circular dependencies
• [ ] Mixed responsibilities
• [ ] Unclear what's public vs private

Documentation Gaps

• [ ] Missing docstrings
• [ ] No type hints
• [ ] Examples missing
• [ ] Module spec missing
• [ ] README not updated

---

Print-Friendly Summary

RUTHLESS SIMPLICITY: Every line justified, no unnecessary abstractions MODULAR ARCHITECTURE: Single responsibility, clear boundaries, no circular deps ZERO-BS IMPLEMENTATION: Production-ready, no TODOs, explicit error handling TEST COVERAGE: 85%+ coverage, contract verified, edge cases tested DOCUMENTATION: Complete docstrings, type hints, examples, module specs

If all these pass → READY TO MERGE If any fail → NEEDS IMPROVEMENTS