Smithery is now a part of Arcade.dev! Read more in our announcement here
Philosophy-aware PR reviews checking alignment with amplihack principles. Use when reviewing PRs to ensure ruthless simplicity, modular design, and zero-BS implementation. Suggests simplifications,...
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.
Every line of code must justify its existence. We ask:
Code should be organized as self-contained modules with clear connections:
No shortcuts, stubs, or technical debt:
Start by understanding what the PR changes:
Review each change against amplihack principles:
Look for common over-engineering patterns:
If new modules or module changes:
Adequate testing is crucial:
When suggesting changes:
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:
```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 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:
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**:
```python
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:
```python
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
EDGE CASES MISSING:
CONTRACT VERIFICATION:
ADD: tests/test_role_manager.py with 85%+ coverage
### Example 5: Recognizing Good Design
**PR**: Add simplified config loader
**Code Changed**:
```python
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.
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]
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
# 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'))
# 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
# 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)
# 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)
When reviewing, ask these questions:
A successful PR review using this skill:
The skill produces:
Philosophy Compliance Report
Specific Recommendations
GitHub Comments (optional)
All reviews anchor in these documents:
~/.amplihack/.claude/context/PHILOSOPHY.md - Core development philosophy~/.amplihack/.claude/context/PATTERNS.md - Approved patterns and anti-patternsSpecs/ - Module specifications for architecture verification~/.amplihack/.claude/context/DISCOVERIES.md - Known issues and solutionsThis skill should evolve based on usage:
Document learnings in ~/.amplihack/.claude/context/DISCOVERIES.md.
