Loading...
Loading...
Code review skill for quality, standards compliance, and best practices
npx skill4agent add vladm3105/aidoc-flow-framework code-reviewgraph TD
A[Code Submission] --> B[Static Analysis]
B --> C{Linting Pass?}
C -->|Fail| D[Report Linting Issues]
C -->|Pass| E[Type Checking]
E --> F{Type Safety?}
F -->|Fail| G[Report Type Errors]
F -->|Pass| H[Security Scan]
H --> I{Security Issues?}
I -->|Critical| J[Block Merge]
I -->|Warning| K[Flag for Review]
I -->|Pass| L[Complexity Analysis]
L --> M{Complexity High?}
M -->|Yes| N[Suggest Refactoring]
M -->|No| O[Coverage Analysis]
O --> P{Coverage < 90%?}
P -->|Yes| Q[Request More Tests]
P -->|No| R[ADR Compliance]
R --> S{ADR Violations?}
S -->|Yes| T[Report Violations]
S -->|No| U[SPEC Compliance]
U --> V{SPEC Mismatch?}
V -->|Yes| W[Report Mismatches]
V -->|No| X[Generate Review Report]
D --> X
G --> X
J --> X
K --> X
N --> X
Q --> X
T --> X
W --> X
X --> Y[Review Complete]code-review analyze --file src/auth/service.py=== Code Review Report: src/auth/service.py ===
Issues Found: 8
[CRITICAL] Security Issue (Line 45)
- SQL Injection vulnerability in user_login()
- Direct string concatenation in SQL query
- CWE-89: SQL Injection
→ Fix: Use parameterized queries
[ERROR] Type Safety (Line 78)
- Missing return type annotation for validate_token()
- mypy: Function return type cannot be inferred
→ Fix: Add return type hint -> bool
[WARNING] Complexity (Line 112-156)
- Cyclomatic complexity: 12 (threshold: 10)
- Method authenticate_user() too complex
→ Suggestion: Extract validation logic to separate methods
[WARNING] Code Duplication (Lines 201-215, 234-248)
- 15 lines duplicated with 95% similarity
→ Suggestion: Extract common logic to validate_password_strength()
[INFO] Documentation (Line 89)
- Missing docstring for public method reset_password()
→ Suggestion: Add docstring with parameter and return descriptions
[INFO] Performance (Line 167)
- N+1 query detected in get_user_permissions()
→ Suggestion: Use JOIN or prefetch related data
Coverage: 87% (Below 90% threshold)
- Uncovered lines: 45-47, 89-91, 134-136
→ Action: Add tests for error handling paths
ADR Compliance: 1 violation
- ADR-003 requires JWT tokens, code uses session cookies
→ Action: Update implementation to use JWT
Total Score: 72/100 (Acceptable with improvements)code-review analyze --module src/authcode-review analyze --diff HEAD~1..HEADcode-review analyze --file src/auth/service.py --auto-fix# CRITICAL: SQL Injection
query = f"SELECT * FROM users WHERE username = '{username}'" # ❌
# FIX
query = "SELECT * FROM users WHERE username = %s"
cursor.execute(query, (username,)) # ✓
# CRITICAL: Hardcoded Secret
API_KEY = "sk-1234567890abcdef" # ❌
# FIX
API_KEY = os.environ.get('API_KEY') # ✓# Complexity: 12 (High)
def process_order(order): # ❌
if order.status == 'pending':
if order.payment_verified:
if order.items_available:
if order.shipping_valid:
# ... nested logic
pass
# FIX: Complexity: 4 (Simple)
def process_order(order): # ✓
if not can_process_order(order):
return False
return execute_order_processing(order)
def can_process_order(order):
return (order.status == 'pending' and
order.payment_verified and
order.items_available and
order.shipping_valid)# Type error: Missing return type
def calculate_total(items): # ❌
return sum(item.price for item in items)
# FIX
def calculate_total(items: list[Item]) -> Decimal: # ✓
return sum(item.price for item in items)
# Type error: None not handled
def get_user(user_id: int) -> User: # ❌
return database.query(User).get(user_id) # May return None
# FIX
def get_user(user_id: int) -> User | None: # ✓
return database.query(User).get(user_id)# Performance issue: N+1 queries
def get_users_with_permissions(): # ❌
users = User.query.all()
for user in users:
user.permissions = Permission.query.filter_by(user_id=user.id).all()
return users
# FIX: Single query with JOIN
def get_users_with_permissions(): # ✓
return User.query.options(
joinedload(User.permissions)
).all()
# Performance issue: Inefficient algorithm O(n²)
def find_duplicates(items): # ❌
duplicates = []
for i, item1 in enumerate(items):
for item2 in items[i+1:]:
if item1 == item2:
duplicates.append(item1)
return duplicates
# FIX: O(n) with set
def find_duplicates(items): # ✓
seen = set()
duplicates = set()
for item in items:
if item in seen:
duplicates.add(item)
seen.add(item)
return list(duplicates)# Violation: Single Responsibility Principle
class UserManager: # ❌
def create_user(self, data):
# Validates data
# Saves to database
# Sends email
# Logs action
pass
# FIX: Separate responsibilities
class UserValidator: # ✓
def validate(self, data): pass
class UserRepository:
def save(self, user): pass
class EmailService:
def send_welcome_email(self, user): pass
class AuditLogger:
def log_user_creation(self, user): pass# Poor documentation
def process(data): # ❌
# Process the data
return result
# Good documentation
def process_user_registration( # ✓
user_data: dict[str, Any]
) -> User:
"""
Process new user registration with validation and persistence.
Args:
user_data: Dictionary containing user registration information
Required keys: username, email, password
Optional keys: full_name, phone_number
Returns:
User: Newly created user instance with assigned ID
Raises:
ValidationError: If user_data fails validation
DatabaseError: If user cannot be persisted
DuplicateUserError: If username or email already exists
Example:
>>> user = process_user_registration({
... 'username': 'johndoe',
... 'email': 'john@example.com',
... 'password': 'SecureP@ss123'
... })
>>> user.id
12345
"""
validated_data = UserValidator.validate(user_data)
user = User.create(validated_data)
return UserRepository.save(user)# ADR-003: All authentication must use JWT tokens
# Violation
@app.route('/login', methods=['POST']) # ❌
def login():
# ... authentication logic
session['user_id'] = user.id # Using sessions, not JWT
return jsonify({'success': True})
# Compliant
@app.route('/login', methods=['POST']) # ✓
def login():
# ... authentication logic
token = jwt.encode(
{'user_id': user.id, 'exp': datetime.utcnow() + timedelta(hours=1)},
SECRET_KEY,
algorithm='HS256'
)
return jsonify({'token': token})# SPEC-AUTH-V1: Password validation requirements
# SPEC violation: Missing uppercase requirement
def validate_password(password: str) -> bool: # ❌
return (
len(password) >= 8 and
any(c.islower() for c in password) and
any(c.isdigit() for c in password)
)
# SPEC compliant
def validate_password(password: str) -> bool: # ✓
return (
8 <= len(password) <= 20 and
any(c.isupper() for c in password) and # Missing requirement
any(c.islower() for c in password) and
any(c.isdigit() for c in password) and
any(c in '!@#$%^&*' for c in password)
)score = (
security_score * 0.30 + # 30% weight
quality_score * 0.25 + # 25% weight
type_safety_score * 0.15 + # 15% weight
coverage_score * 0.15 + # 15% weight
documentation_score * 0.10 + # 10% weight
performance_score * 0.05 # 5% weight
)
# Score categories
90-100: Excellent (merge approved)
80-89: Good (minor improvements recommended)
70-79: Acceptable (improvements required)
60-69: Needs work (significant issues)
<60: Poor (major rework needed)ReadBashGrepGlob# .github/workflows/code-review.yml
name: Automated Code Review
on: [pull_request]
jobs:
code-review:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Run code review
run: |
code-review analyze --diff origin/main..HEAD
- name: Check quality gate
run: |
if [ $CODE_REVIEW_SCORE -lt 70 ]; then
echo "Code quality below threshold"
exit 1
fireports/code-review/metrics/code-quality.json.code-review.yml