kivo360/backend/omoi_os/sandbox_skills/code-review

Code Review

Perform thorough code reviews using this structured approach.

Review Checklist

1. Security (CRITICAL)

• [ ] Injection vulnerabilities: SQL, command, XSS, template injection
• [ ] Authentication: Proper auth checks, secure session handling
• [ ] Authorization: Access control on all sensitive operations
• [ ] Secrets: No hardcoded credentials, API keys, tokens
• [ ] Input validation: All user input validated and sanitized
• [ ] Output encoding: Proper encoding for context (HTML, URL, JS)

2. Logic & Correctness

• [ ] Requirements met: Code implements what was specified
• [ ] Edge cases: Null/empty/boundary conditions handled
• [ ] Error handling: Exceptions caught and handled appropriately
• [ ] Race conditions: Concurrent access handled correctly
• [ ] State management: State transitions are valid

3. Performance

• [ ] N+1 queries: Database queries in loops
• [ ] Memory leaks: Unclosed resources, retained references
• [ ] Unnecessary computation: Repeated calculations, missing caching
• [ ] Blocking operations: Async operations not blocking main thread
• [ ] Large payloads: Response sizes reasonable

4. Maintainability

• [ ] Single responsibility: Each function/class has one purpose
• [ ] DRY: No duplicated code
• [ ] Naming: Clear, descriptive names for variables/functions
• [ ] Comments: Complex logic explained, not obvious code
• [ ] Type hints: Types annotated (Python/TypeScript)

5. Testing

• [ ] Test coverage: New code has tests
• [ ] Test quality: Tests verify behavior, not implementation
• [ ] Edge case tests: Boundary conditions tested
• [ ] Integration tests: System interactions tested

Review Output Format

## Code Review: {file/component}

### Summary
{1-2 sentence overall assessment}

### 🔴 Critical Issues
{Must fix before merge}

#### Issue 1: {Title}
- **Location**: `file.py:42`
- **Problem**: {Description}
- **Impact**: {Security/correctness impact}
- **Fix**: {Suggested solution}

### 🟡 Suggestions
{Should consider fixing}

#### Suggestion 1: {Title}
- **Location**: `file.py:88`
- **Current**: {What it does now}
- **Suggested**: {Better approach}
- **Reason**: {Why this is better}

### 🟢 Positive Notes
{Good patterns to highlight}

- Good use of {pattern} at `file.py:100`
- Clear error handling in {function}

### Verdict
- [ ] ✅ Approve
- [ ] 🔄 Request Changes
- [ ] 💬 Needs Discussion

Security Patterns to Flag

Python

# 🔴 SQL Injection
cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")  # BAD
cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,))  # GOOD

# 🔴 Command Injection
os.system(f"ls {user_input}")  # BAD
subprocess.run(["ls", user_input], check=True)  # GOOD

# 🔴 Path Traversal
open(f"/data/{user_filename}")  # BAD
safe_path = Path("/data") / user_filename
if safe_path.resolve().is_relative_to(Path("/data")):  # GOOD
    open(safe_path)

TypeScript/JavaScript

// 🔴 XSS
element.innerHTML = userInput;  // BAD
element.textContent = userInput;  // GOOD

// 🔴 Prototype Pollution
Object.assign(target, JSON.parse(userInput));  // BAD
// Validate shape before merging

// 🔴 SSRF
fetch(userProvidedUrl);  // BAD
// Validate URL against allowlist

Performance Patterns to Flag

# 🔴 N+1 Query
for user in users:
    orders = db.query(Order).filter(Order.user_id == user.id).all()  # BAD

# ✅ Eager Loading
users = db.query(User).options(joinedload(User.orders)).all()  # GOOD

# 🔴 Repeated Computation
for item in items:
    expensive = calculate_expensive_thing()  # BAD (repeated)
    process(item, expensive)

# ✅ Cache Result
expensive = calculate_expensive_thing()  # GOOD (once)
for item in items:
    process(item, expensive)

Review Commands

# View changes in a PR
git diff main...HEAD

# Check specific file
git diff main -- path/to/file.py

# View commit history
git log --oneline main..HEAD

# Check for secrets (use git-secrets or similar)
git secrets --scan