pr-review-assistant

What this skill does

# 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**:

```python
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:
```python
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 mod