yi-john-huang/sdd-review

SDD Code Review

Perform comprehensive code reviews in the style of Linus Torvalds - direct, thorough, and focused on what matters: correctness, simplicity, and long-term maintainability.

Review Philosophy

"Talk is cheap. Show me the code." — Linus Torvalds

This review focuses on:

1. Correctness - Does it actually work? Does it handle edge cases?
2. Simplicity - Is it more complex than necessary?
3. Maintainability - Will future developers understand this?
4. Convention Adherence - Does it follow project patterns?

Workflow

Step 1: Identify Review Scope

Determine what to review:

• Single file: /sdd-review src/services/UserService.ts
• Directory: /sdd-review src/services/
• Git diff: /sdd-review HEAD~3..HEAD
• PR/MR: /sdd-review PR-123 or /sdd-review MR-45

Step 2: Load Project Context

Before reviewing:

1. Read project steering documents from .spec/steering/
2. Understand existing patterns in the codebase
3. Check if there's a related spec in .spec/specs/

Step 3: Perform Review

Code Correctness Checks

## Correctness Issues

### Critical
- [ ] Logic errors that will cause bugs
- [ ] Race conditions or threading issues
- [ ] Resource leaks (files, connections, memory)
- [ ] Unhandled error conditions

### Important
- [ ] Edge cases not handled
- [ ] Assumptions that may not hold
- [ ] Off-by-one errors
- [ ] Type mismatches or unsafe casts

Simplicity Assessment

Ask these questions:

• Could this be done with less code?
• Is there a standard library function for this?
• Is this abstraction earning its keep?
• Would a junior developer understand this in 6 months?

Pattern Violations

Check against project conventions:

## Pattern Violations

### Naming
- [ ] Variables don't follow naming convention
- [ ] Functions named for implementation, not purpose

### Structure
- [ ] Logic in wrong layer (controller doing business logic)
- [ ] Missing separation of concerns
- [ ] Circular dependencies introduced

### Error Handling
- [ ] Swallowed exceptions
- [ ] Generic error messages
- [ ] Missing error propagation

Step 4: Provide Feedback

Structure feedback with clear categories:

# Code Review: {file/PR description}

## Summary
Brief overall assessment (1-2 sentences)

## 🚨 Must Fix (Blocking)
Issues that must be resolved before merge:
1. **Line 42**: Memory leak - connection never closed
   ```diff
   - const conn = await getConnection();
   + const conn = await getConnection();
   + try { ... } finally { conn.close(); }

⚠️ Should Fix (Non-blocking)

Issues that should be addressed but won't block:

1. Line 78: Magic number should be a named constant
2. Line 103: Consider extracting this to a helper function

💡 Suggestions (Optional)

Improvements that would be nice but are truly optional:

1. Line 156: This could be simplified with Array.flatMap()

✅ What's Good

Acknowledge good patterns to reinforce them:

1. Good use of dependency injection
2. Clear separation of concerns
3. Comprehensive error handling in auth module

### Step 5: Verify Tests

For any code changes:
1. Check if tests exist for modified code
2. Verify edge cases are tested
3. Run existing tests to ensure no regressions

```bash
# Run tests for affected files
npm test -- --findRelatedTests {changed-files}

Review Severity Levels

| Level | Meaning | Action Required | |-------|---------|-----------------| | 🚨 Critical | Bug, security issue, data loss risk | Must fix before merge | | ⚠️ Warning | Code smell, potential issue | Should fix, discuss if disagree | | 💡 Info | Suggestion, style preference | Optional, author's choice |

Common Issues to Watch For

TypeScript/JavaScript Specific

• any type usage without justification
• Missing null/undefined checks
• Promises not awaited
• Event listener memory leaks
• Mutable shared state

General

• Functions doing too much
• Deep nesting (> 3 levels)
• Boolean parameters (use options object)
• Comments explaining what instead of why
• Dead code or unused imports

Integration with SDD Workflow

When reviewing implementation:

1. Compare against requirements in .spec/specs/{feature}/requirements.md
2. Verify design patterns from .spec/specs/{feature}/design.md
3. Check task completion against .spec/specs/{feature}/tasks.md

Example Review

# Code Review: UserAuthService.ts

## Summary
Good overall structure but has a critical security issue and some error handling gaps.

## 🚨 Must Fix
1. **Line 67**: Password stored in plain text in error log
   ```typescript
   // BAD: Leaks credentials
   logger.error(`Login failed for ${email} with password ${password}`);

   // GOOD: Never log credentials
   logger.error(`Login failed for ${email}`);

⚠️ Should Fix

1. Line 89: Catch block swallows all errors
2. Line 112-130: This block should be extracted to a private method

✅ What's Good

• Clean separation between auth logic and data access
• Good use of TypeScript discriminated unions for auth result
• Comprehensive input validation