Review Git Changes

Instructions

Perform thorough code review of current working copy changes, optionally compare to plan, and identify engineering issues.

Phase 1: Discover Changes & Context

Step 1: Get Current Changes

# See changed files
git status

# See detailed diff
git diff

# See staged changes separately
git diff --cached

# See both staged and unstaged
git diff HEAD

Step 2: Identify Related Plan (if exists)

Search for related plan file:

• Check .plans/ directory for relevant plan
• Look for plan mentioned in branch name
• Ask user if unsure which plan applies
• If no plan exists, review against general best practices

If plan exists:

• Read the entire plan
• Understand intended design and architecture
• Note specific requirements and constraints

Step 3: Categorize Changed Files

Group files by type:

• New files: Created from scratch
• Modified files: Existing files changed
• Deleted files: Removed files
• Renamed/moved files: Organizational changes

Create todo list with one item per changed file to review.

Phase 2: Systematic File Review

For EACH changed file in the todo list:

Step 1: Read Current State

• Read the entire file in its current state
• Understand what it does
• Note its responsibilities

Step 2: Analyze the Changes

Read git diff to see exactly what changed:

• What was added?
• What was removed?
• What was modified?

Step 3: Assess Against Plan (if applicable)

If plan exists, check:

• Does implementation match plan?

  • Are planned features implemented correctly?
  • Is architecture followed?
  • Are file names as specified?

• Scope adherence:

  • Is this change in the plan?
  • Is it necessary for the plan's goals?
  • Is it scope creep?

• REMOVAL SPEC compliance:

  • If plan said to remove code, was it removed?
  • Is old code still present when it shouldn't be?

Step 4: Identify Engineering Issues

Check for Bad Engineering:

• Bugs: Logic errors, off-by-one, race conditions
• Poor error handling: Swallowed errors, generic catches
• Missing validation: No input validation, no null checks
• Hard-coded values: Magic numbers, hardcoded URLs/paths
• Tight coupling: Unnecessary dependencies between modules
• Violation of SRP: Class/function doing too many things
• Incorrect patterns: Misuse of design patterns
• Type issues: Use of any, missing types, wrong types
• Missing edge cases: Doesn't handle empty/null/error cases

Check for Over-Engineering:

• Unnecessary abstraction: Too many layers for simple logic
• Premature optimization: Complex code for unmeasured performance
• Framework overuse: Using heavy library for simple task
• Feature creep: Adding features not in requirements
• Gold plating: Excessive polish on non-critical code
• YAGNI violations: Code for "might need later" scenarios
• Complexity without benefit: Complicated when simple works

Check for Suboptimal Solutions:

• Duplication: Copy-pasted code instead of extraction
• Reinventing wheel: Custom code when standard library exists
• Wrong tool: Using inappropriate data structure/algorithm
• Inefficient approach: O(n²) when O(n) is obvious
• Poor naming: Unclear variable/function names
• Missing reuse: Existing utilities/types not used
• Inconsistent patterns: Doesn't match codebase style
• Technical debt: Quick hack instead of proper solution

Step 5: Check Code Quality

• Readability: Is code clear and self-documenting?
• Maintainability: Will this be easy to change later?
• Testability: Can this be tested easily?
• Performance: Any obvious performance issues?
• Security: Any security vulnerabilities?
• Consistency: Matches existing codebase patterns?

Step 6: Record Findings

Store in memory:

File: path/to/file.ts
Change Type: [New|Modified|Deleted]
Plan Compliance: [Matches|Deviates|Not in plan]
Issues:
  Bad Engineering:
    - [Specific issue with line number]
  Over-Engineering:
    - [Specific issue with line number]
  Suboptimal:
    - [Specific issue with line number]
Severity: [CRITICAL|HIGH|MEDIUM|LOW]

Step 7: Update Todo

Mark file as reviewed in todo list.

Phase 3: Cross-File Analysis

After reviewing all files:

Step 1: Architectural Impact

• How do changes affect overall system architecture?
• Are there breaking changes?
• Do changes introduce new dependencies?
• Is there architectural drift from the plan?

Step 2: Pattern Consistency

• Are changes consistent with each other?
• Do they follow same patterns?
• Any conflicting approaches?

Step 3: Completeness Check

• Are all related changes present?
• Missing files that should be changed?
• Orphaned references?

Phase 4: Generate Review Report

Create report at .reviews/code-review-[timestamp].md:

# Code Review Report
**Date**: [timestamp]
**Branch**: [branch name]
**Related Plan**: [plan file or "None"]
**Files Changed**: X
**Issues Found**: Y

---

## Executive Summary
- **Critical Issues**: X (must fix before merge)
- **High Priority**: Y (should fix)
- **Medium Priority**: Z (consider fixing)
- **Low Priority**: W (suggestions)

**Overall Assessment**: [APPROVE|REQUEST CHANGES|REJECT]

---

## Plan Compliance (if applicable)

### Matches Plan ✅
- Feature X implemented correctly
- Architecture follows design
- File naming conventions followed

### Deviates from Plan ⚠️
- Implementation differs from planned approach in [area]
- Missing feature Y from plan
- Scope creep: Added Z not in plan

### REMOVAL SPEC Status
- ✅ Old code removed from `file.ts:50-100`
- ❌ Legacy function still exists in `auth.ts:42` (should be removed)

---

## Issues by Severity

### CRITICAL: Bad Engineering

#### Logic Bug in `src/services/auth.ts:125`
```typescript
// Current code:
if (user.role = 'admin') { // Assignment instead of comparison
  grantAccess()
}

Issue: Assignment operator used instead of equality check. This always evaluates to true and grants everyone admin access. Severity: CRITICAL - Security vulnerability Fix: Change = to ===

Unhandled Promise in src/api/client.ts:67

// Current code:
fetchData().then(data => process(data)) // No error handling

Issue: Promise rejection not handled, will crash silently Severity: CRITICAL - Application stability Fix: Add .catch() or use try/catch with async/await

HIGH: Over-Engineering

Unnecessary Abstraction in src/utils/formatter.ts

// Current code: 50 lines of abstraction
class FormatterFactory {
  createFormatter(type: string): IFormatter { /* ... */ }
}
class StringFormatter implements IFormatter { /* ... */ }
// ... only used once for simple string formatting

Issue: Complex factory pattern for single use case Severity: HIGH - Maintenance burden Better Approach: Simple function formatString(value: string): string

MEDIUM: Suboptimal Solutions

Code Duplication in src/components/

Files: user-form.tsx, admin-form.tsx Issue: Both contain identical validation logic (30 lines duplicated) Severity: MEDIUM - Maintenance issue Better Approach: Extract to src/utils/form-validation.ts

Not Using Existing Type in src/types/user.ts

// Current code:
interface UserData {
  id: string
  email: string
  name: string
}
// But `User` type already exists with same fields in `src/models/user.ts`

Issue: Duplicate type definition Severity: MEDIUM - Type inconsistency risk Fix: Import and use existing User type

LOW: Suggestions

Verbose Naming in src/services/database-connection-manager.ts

Issue: Overly verbose filename Suggestion: Simplify to src/services/database.service.ts

---

Issues by Category

Bad Engineering (X issues)

• Logic bugs: X
• Missing error handling: Y
• Type issues: Z
• Missing validation: W

Over-Engineering (Y issues)

• Unnecessary abstraction: X
• Premature optimization: Y
• Feature creep: Z

Suboptimal Solutions (Z issues)

• Code duplication: X
• Reinventing wheel: Y
• Poor naming: Z
• Not using existing code: W

---

Architectural Assessment

Positive Changes

• Clean separation of concerns in new modules
• Proper use of dependency injection
• Good test coverage added

Concerns

• New dependency introduced without discussion
• Breaking change to public API
• Coupling between previously independent modules

Technical Debt Introduced

• Quick hack in auth.ts:200 marked with TODO
• Temporary file temp-processor.ts added
• Migration code that should be removed later

---

Detailed File Reviews

src/services/auth.ts (Modified)

Plan Compliance: Matches Changes: 45 lines added, 20 removed Issues:

• CRITICAL: Logic bug at line 125 (assignment vs equality)
• HIGH: Missing error handling at line 89 Positive:
• Good use of existing types
• Clear function names

src/components/user-form.tsx (New)

Plan Compliance: Not in plan (scope creep) Changes: 150 lines added Issues:

• MEDIUM: Duplicates logic from admin-form.tsx
• LOW: Could use existing form validation utility Positive:
• Clean component structure
• Good TypeScript usage

[Continue for all changed files]

---

Statistics

By Severity:

• Critical: X
• High: Y
• Medium: Z
• Low: W

By Category:

• Bad Engineering: X
• Over-Engineering: Y
• Suboptimal: Z

By File Type:

• Services: X issues
• Components: Y issues
• Utils: Z issues

---

Recommendations

Must Fix (Before Merge)

1. Fix logic bug in auth.ts:125 - Security issue
2. Add error handling in client.ts:67 - Stability issue
3. Remove temporary file temp-processor.ts - Plan violation

Should Fix

1. Extract duplicated validation logic
2. Simplify over-abstracted formatter
3. Use existing types instead of duplicates

Consider

1. Rename verbose files
2. Add more inline documentation
3. Improve variable naming in complex functions

---

Overall Assessment

Recommendation: REQUEST CHANGES

Reasoning:

• 2 critical security/stability issues must be fixed
• Over-engineering in several areas adds unnecessary complexity
• Code duplication will cause maintenance issues
• Some scope creep not discussed in plan

After Fixes: Changes will be solid. Core implementation is sound, just needs cleanup and bug fixes.

### Phase 5: Summary for User

Provide concise summary:

```markdown
# Code Review Complete

## Assessment: [APPROVE|REQUEST CHANGES|REJECT]

## Critical Issues (X)
- Security bug in `auth.ts:125` - assignment vs equality
- Unhandled promise in `client.ts:67` - will crash

## High Priority (Y)
- Over-engineering in `formatter.ts` - unnecessary abstraction
- Missing error handling in multiple files

## Medium Priority (Z)
- Code duplication between forms
- Not using existing types

## Plan Compliance
- ✅ Core features implemented correctly
- ⚠️ Some scope creep (user-form not in plan)
- ❌ REMOVAL SPEC incomplete (legacy code still exists)

## Must Fix Before Merge
1. Fix logic bug in auth.ts
2. Add error handling
3. Remove legacy code per REMOVAL SPEC

**Full Report**: `.reviews/code-review-[timestamp].md`

Critical Principles

• NEVER EDIT FILES - This is review only, not implementation
• BE THOROUGH - Review every changed file
• BE SPECIFIC - Point to exact line numbers
• BE CONSTRUCTIVE - Explain why and suggest better approach
• CHECK PLAN - If plan exists, verify compliance
• VERIFY REMOVAL SPEC - Ensure old code was removed
• IDENTIFY PATTERNS - Note systemic issues across files
• BE HONEST - Don't approve bad code to be nice
• SUGGEST ALTERNATIVES - Don't just criticize, help improve

Success Criteria

A complete code review includes:

• All changed files reviewed
• Plan compliance verified (if plan exists)
• All engineering issues identified and categorized
• Severity assigned to each issue
• Specific line numbers referenced
• Alternative approaches suggested
• Overall assessment provided
• Structured report generated