review-changes

name	review-changes
description	Code review of current git changes, compare to related plan if exists, identify bad engineering, over-engineering, or suboptimal solutions. Use when user asks to review changes, check git diff, validate implementation quality, or assess code changes.

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 2.5: Issue/Task Coverage Verification (MANDATORY)

CRITICAL: Before completing the review, verify that 100% of the original issue/task requirements are implemented.

Step 1: Identify Source Requirements

Locate the original requirements from:

• GitHub issue (gh issue view <number>)
• PR description referencing issues
• Task ticket or plan file in .plans/
• Commit messages describing the work

Step 2: Extract ALL Requirements

Create exhaustive checklist:

• Functional requirements (what it should do)
• Acceptance criteria (how to verify)
• Edge cases mentioned
• Error handling requirements

Step 3: Verify Each Requirement

#	Requirement	Status	Evidence
1	[requirement]	✅/❌/⚠️	file:line or "Missing"

Step 4: Coverage Assessment

Requirements Coverage = (Implemented / Total) × 100%

Anything less than 100% = REQUEST CHANGES immediately

Add to report:

## Issue/Task Coverage

**Source**: [Issue #X / Plan file]
**Coverage**: X% (Y of Z requirements)

### Missing Requirements
- ❌ [Requirement X]: Not implemented
- ❌ [Requirement Y]: Partially implemented - [what's missing]

**VERDICT**: Coverage incomplete. Cannot approve until 100% implemented.

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:

• 100% of issue/task requirements verified as implemented
• 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

CRITICAL: If issue/task coverage is less than 100%, the review MUST request changes regardless of code quality.