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 for relevant plan tickets (tk list --tag 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:

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 ticket (tagged plan )
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 a tk ticket tagged review with the report as its body. Use todos_oneshot(title: "Review: <branch-or-summary>", description: "<report content>", tags: "review", type: "task") .

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`

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

- Fix logic bug in auth.ts:125
 - Security issue

- Add error handling in client.ts:67
 - Stability issue

- Remove temporary file temp-processor.ts
 - Plan violation

Should Fix

- Extract duplicated validation logic

- Simplify over-abstracted formatter

- Use existing types instead of duplicates

Consider

- Rename verbose files

- Add more inline documentation

- 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**: tk ticket (tagged `review`)

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.

After completing the review, follow handbook 15.04
 to create tk
 tickets for all surfaced issues.

review-changes

Safety Notice

Copy this and send it to your AI assistant to learn

See changed files

See detailed diff

See staged changes separately

See both staged and unstaged

Issue/Task Coverage

Missing Requirements

Code Review Report

Executive Summary

Plan Compliance (if applicable)

Matches Plan ✅

Deviates from Plan ⚠️

REMOVAL SPEC Status

Issues by Severity

CRITICAL: Bad Engineering

Logic Bug in `src/services/auth.ts:125`

Source Transparency

Related Skills

4-step-program

chrome-devtools

finishing-a-development-branch

review-changes

Safety Notice

Copy this and send it to your AI assistant to learn

See changed files

See detailed diff

See staged changes separately

See both staged and unstaged

Issue/Task Coverage

Missing Requirements

Code Review Report

Executive Summary

Plan Compliance (if applicable)

Matches Plan ✅

Deviates from Plan ⚠️

REMOVAL SPEC Status

Issues by Severity

CRITICAL: Bad Engineering

Logic Bug in src/services/auth.ts:125

Source Transparency

Related Skills

4-step-program

chrome-devtools

finishing-a-development-branch

Logic Bug in `src/services/auth.ts:125`