code-review

code-review

Description: Automated code quality analysis, static analysis, security scanning, and best practices enforcement

Category: Code Quality Assurance

Complexity: High (multi-tool integration + analysis)

---

Purpose

Perform comprehensive code review to ensure quality, security, maintainability, and compliance with architectural decisions (ADRs) and specifications (SPEC). Identifies issues before code reaches production.

---

Capabilities

1. Static Analysis

• Linting: pylint, ruff, flake8 integration
• Type checking: mypy, pyright for type safety
• Code style: PEP 8 compliance, formatting issues
• Complexity metrics: cyclomatic complexity, cognitive complexity
• Code smells: duplicate code, long methods, god classes

2. Security Vulnerability Scanning

• Dependency vulnerabilities: bandit, safety checks
• Security anti-patterns: SQL injection, XSS, CSRF risks
• Secret detection: hardcoded credentials, API keys
• OWASP Top 10: Common security vulnerabilities
• CWE mapping: Common Weakness Enumeration references

3. Best Practices Enforcement

• Design patterns: Proper pattern usage
• SOLID principles: Violation detection
• DRY principle: Code duplication analysis
• Error handling: Exception management review
• Resource management: Memory leaks, file handle leaks

4. Code Coverage Analysis

• Line coverage: Percentage of executed lines
• Branch coverage: Conditional path coverage
• Function coverage: Tested vs untested functions
• Gap identification: Uncovered critical paths

5. Architectural Compliance

• ADR validation: Code aligns with architecture decisions
• SPEC compliance: Implementation matches specifications
• Dependency rules: No circular dependencies, proper layering
• API contract adherence: Matches CTR definitions

6. Performance Analysis

• Inefficient algorithms: O(n²) where O(n) possible
• Memory usage: Excessive allocations
• Database queries: N+1 query problems
• Caching opportunities: Repeated expensive operations

7. Documentation Quality

• Docstring coverage: Missing or inadequate documentation
• Comment quality: Outdated or misleading comments
• API documentation: Public method documentation
• Type hints: Missing type annotations

8. Automated Fix Suggestions

• Auto-fixable issues: Formatting, imports, simple refactorings
• Refactoring recommendations: Extract method, simplify conditions
• Performance optimizations: Suggest faster alternatives
• Security patches: Fix common vulnerabilities

---

Code Review Workflow

graph 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]

---

Usage Instructions

Review Single File

code-review analyze --file src/auth/service.py

Output:

=== 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)

Review Entire Module

code-review analyze --module src/auth

Review Changed Files (Git)

code-review analyze --diff HEAD~1..HEAD

Review with Auto-fix

code-review analyze --file src/auth/service.py --auto-fix

---

Analysis Categories

1. Security Analysis

Tools: bandit, safety, semgrep

Checks:

• SQL injection vulnerabilities
• XSS attack vectors
• CSRF token validation
• Hardcoded secrets
• Insecure randomness
• Path traversal risks
• Command injection
• Insecure deserialization
• Weak cryptography

Severity Levels:

• CRITICAL: Immediate security risk (block merge)
• HIGH: Serious vulnerability (requires fix)
• MEDIUM: Potential security issue (review required)
• LOW: Security best practice violation

Example:

# 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')  # ✓

---

2. Code Quality Analysis

Tools: pylint, ruff, radon

Checks:

• PEP 8 compliance
• Cyclomatic complexity
• Maintainability index
• Code duplication
• Dead code
• Unused imports/variables
• Magic numbers
• Long functions/classes

Complexity Thresholds:

• A (1-5): Simple, easy to maintain
• B (6-10): Moderate complexity
• C (11-20): High complexity (refactor recommended)
• D (21-50): Very high complexity (refactor required)
• F (>50): Extreme complexity (immediate refactor)

Example:

# 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)

---

3. Type Safety Analysis

Tools: mypy, pyright

Checks:

• Missing type hints
• Type incompatibilities
• Invalid type casting
• Optional value handling
• Generic type usage
• Protocol compliance

Example:

# 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)

---

4. Performance Analysis

Checks:

• Inefficient algorithms
• Memory leaks
• N+1 query problems
• Unnecessary loops
• Missing caching
• Blocking I/O operations
• Large memory allocations

Example:

# 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)

---

5. Best Practices Analysis

Checks:

• SOLID principles
• DRY (Don't Repeat Yourself)
• KISS (Keep It Simple)
• YAGNI (You Aren't Gonna Need It)
• Error handling patterns
• Resource management
• Logging practices

Example:

# 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

---

6. Documentation Analysis

Checks:

• Missing docstrings
• Incomplete docstrings
• Outdated comments
• Type hint documentation
• API documentation
• Example usage

Example:

# 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)

---

7. ADR Compliance Analysis

Checks:

• Architecture decision adherence
• Technology stack compliance
• Design pattern usage
• API design standards
• Data flow patterns

Example:

# 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})

---

8. SPEC Compliance Analysis

Checks:

• Implementation matches specifications
• API signatures correct
• Data structures match schemas
• Business logic correct
• Error handling as specified

Example:

# 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)
    )

---

Review Score Calculation

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)

---

Tool Access

Required tools:

• Read: Read source code files
• Bash: Execute analysis tools (pylint, mypy, bandit)
• Grep: Search for patterns and violations
• Glob: Find code files

Required libraries:

• pylint
• ruff
• mypy / pyright
• bandit
• safety
• radon
• coverage
• semgrep

---

Integration Points

With test-automation

• Verify test coverage meets thresholds
• Identify untested code paths
• Validate test quality

With doc-validator

• Check code documentation quality
• Validate traceability references in code
• Ensure code comments follow standards

With security-audit

• Share security vulnerability findings
• Coordinate security fixes
• Track security metrics

With refactor-flow

• Identify refactoring opportunities
• Provide complexity metrics
• Suggest code improvements

---

Auto-fix Capabilities

Safe Auto-fixes (Applied Automatically)

• Import sorting
• Code formatting (black, autopep8)
• Trailing whitespace removal
• Missing blank lines
• Unused imports removal
• Simple type hint additions

Suggested Fixes (Requires Approval)

• Extract method refactorings
• Complexity reduction
• Performance optimizations
• Security vulnerability patches
• Documentation additions

---

CI/CD Integration

# .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
          fi

---

Best Practices

1. Review early and often: Don't wait until PR stage
2. Fix critical issues immediately: Security and correctness first
3. Track metrics over time: Monitor code quality trends
4. Automate in CI/CD: Prevent poor code from merging
5. Prioritize issues: Focus on high-impact problems
6. Provide context: Explain why issues matter
7. Suggest solutions: Don't just identify problems
8. Balance strictness: Don't block progress over minor style issues

---

Limitations

1. Cannot understand business logic intent
2. May produce false positives (requires human review)
3. Cannot detect all security vulnerabilities
4. Performance analysis limited to obvious patterns
5. ADR/SPEC compliance requires manual rule definition
6. Tool installation and configuration required

---

Future Enhancements

1. AI-powered code understanding and suggestions
2. Learning from historical code reviews
3. Custom rule engine for project-specific standards
4. Visual code quality dashboards
5. Automated dependency updates
6. Predictive bug detection
7. Code smell trend analysis

---

Success Criteria

• Zero critical security vulnerabilities
• Code quality score ≥80/100
• Type safety: 100% type hints on public APIs
• Coverage ≥90%
• Complexity: No functions with complexity >10
• Zero ADR violations
• Zero SPEC mismatches

---

Notes

• Review reports saved to reports/code-review/
• Historical metrics tracked in metrics/code-quality.json
• Auto-fixes require approval unless configured otherwise
• Security issues always block merge
• Can be customized per project with .code-review.yml config