ComputerConnection/skills/review

Code Review: $ARGUMENTS

Systematic review to catch what humans miss.

---

Philosophy

• Understand first, critique second - Read to understand, not to find fault
• Intent matters - What is this code trying to do?
• Bugs > style - Catch the bugs before nitpicking formatting
• Be constructive - Every criticism comes with a suggestion
• Context is king - A "bad" pattern might be the right choice here

---

Phase 1: Context Gathering

What Am I Reviewing?

# If PR number
gh pr view [number] --comments
gh pr diff [number]

# If branch
git log main..[branch] --oneline
git diff main...[branch]

# If file
git log -10 --follow [file]
git diff HEAD~5 [file]

Review Metadata

┌─────────────────────────────────────────────────────────────┐
│  REVIEW: [Description]                                      │
├─────────────────────────────────────────────────────────────┤
│  Author: [who]                                              │
│  Size: [lines added/removed]                                │
│  Files: [count]                                             │
│  Type: [feature|bugfix|refactor|docs|test]                  │
├─────────────────────────────────────────────────────────────┤
│  Related:                                                   │
│  • Issue: [link]                                            │
│  • Previous PRs: [links]                                    │
│  • Docs: [links]                                            │
└─────────────────────────────────────────────────────────────┘

Understand the Goal

• What problem does this solve?
• What was the previous behavior?
• What should the new behavior be?
• How should I test this mentally?

---

Phase 2: First Pass - The Big Picture

Architecture Review

Questions to answer:
- [ ] Does this change make sense architecturally?
- [ ] Is it in the right place?
- [ ] Does it follow existing patterns?
- [ ] Is it the right level of abstraction?
- [ ] Does it introduce new dependencies? Are they justified?

Scope Check

• [ ] Does this do one thing? (single responsibility)
• [ ] Could this be smaller?
• [ ] Is there scope creep from the original issue?
• [ ] Are there unrelated changes mixed in?

Design Decisions

• [ ] What design decisions were made?
• [ ] Do I agree with them?
• [ ] Are there alternatives that should be considered?

---

Phase 3: Second Pass - Line by Line

Bug Hunting Checklist

Logic Errors

• [ ] Off-by-one errors (loops, arrays, ranges)
• [ ] Null/undefined handling
• [ ] Edge cases (empty, zero, negative, max values)
• [ ] Race conditions (async operations)
• [ ] Error handling (what if this fails?)

Security Issues

• [ ] SQL injection (raw queries, string interpolation)
• [ ] XSS (unsanitized user input in HTML)
• [ ] Auth/authz (permission checks)
• [ ] Sensitive data exposure (logs, errors, responses)
• [ ] Input validation (trusting client data)

Performance Issues

• [ ] N+1 queries (loops with DB calls)
• [ ] Missing indexes (new query patterns)
• [ ] Memory leaks (listeners not cleaned up)
• [ ] Expensive operations in hot paths
• [ ] Large payloads (overfetching data)

Maintainability Issues

• [ ] Hard-to-read code (needs refactor or comments)
• [ ] Magic numbers/strings (should be constants)
• [ ] Copy-paste code (should be extracted)
• [ ] Missing tests for complex logic
• [ ] Unclear naming

---

Phase 4: Detailed Review Notes

Finding Template

## [Severity]: [Short Description]

**File**: `path/to/file.ts:123`

**Code**:
\`\`\`typescript
// Current
problematic code here
\`\`\`

**Issue**: [What's wrong]

**Suggestion**:
\`\`\`typescript
// Suggested
better code here
\`\`\`

**Why**: [Explanation of the improvement]

Severity Levels

| Level | Meaning | Blocks Merge? | |-------|---------|---------------| | BLOCKER | Bug, security issue, will break prod | Yes | | MAJOR | Significant issue, should fix | Probably | | MINOR | Improvement opportunity | No | | NIT | Style preference, take it or leave it | No | | QUESTION | Clarification needed | Maybe | | PRAISE | This is good, keep it up! | No |

---

Phase 5: Security Deep Dive

OWASP Quick Check

[ ] A01: Broken Access Control
    - Are permissions checked?
    - Can users access others' data?

[ ] A02: Cryptographic Failures
    - Is sensitive data encrypted?
    - Are secrets hardcoded?

[ ] A03: Injection
    - SQL injection via user input?
    - Command injection?
    - Template injection?

[ ] A04: Insecure Design
    - Is the design fundamentally secure?

[ ] A05: Security Misconfiguration
    - Debug mode disabled?
    - Default credentials removed?

[ ] A06: Vulnerable Components
    - Are dependencies up to date?
    - Any known vulnerabilities?

[ ] A07: Auth Failures
    - Proper password handling?
    - Session management secure?

[ ] A08: Data Integrity Failures
    - Is input validated?
    - Is data sanitized?

[ ] A09: Logging Failures
    - Are security events logged?
    - Are logs protected from injection?

[ ] A10: SSRF
    - Does user input control URLs?
    - Are internal endpoints protected?

---

Phase 5b: Rust/Tauri Deep Dive

If reviewing Rust or Tauri code, add these checks:

Rust-Specific

[ ] Memory Safety
    - Any unsafe blocks? Are they justified?
    - Proper lifetime annotations?
    - No use-after-free patterns?

[ ] Error Handling
    - Using Result<T, E> properly?
    - No unwrap() in production paths?
    - Errors propagated with context (anyhow/thiserror)?

[ ] Concurrency
    - Arc/Mutex used correctly?
    - No deadlock potential?
    - Async boundaries clear?

[ ] Performance
    - Unnecessary clones?
    - Large structs passed by value?
    - Allocations in hot loops?

Tauri-Specific

[ ] IPC Security
    - Commands properly scoped?
    - Input validated before Rust processing?
    - No path traversal in file operations?

[ ] Frontend-Backend Contract
    - TypeScript types match Rust structs?
    - Serialization/deserialization tested?
    - Error responses handled in frontend?

[ ] Permissions
    - Capabilities properly configured?
    - No overly permissive allowlists?
    - File system access scoped correctly?

[ ] State Management
    - Tauri state thread-safe?
    - No race conditions between commands?
    - State properly initialized?

---

Phase 6: Testing Review

Test Coverage

• [ ] Are there tests for this change?
• [ ] Do tests cover the happy path?
• [ ] Do tests cover edge cases?
• [ ] Do tests cover error conditions?
• [ ] Are tests actually testing behavior (not implementation)?

Test Quality

• [ ] Are tests readable?
• [ ] Do tests have good names describing behavior?
• [ ] Are tests independent (no shared state)?
• [ ] Do tests fail for the right reasons?

Missing Tests

## Missing Test Coverage

- [ ] Test for [scenario]: When [input], expect [output]
- [ ] Test for [edge case]: When [condition], expect [behavior]
- [ ] Test for [error]: When [failure], expect [handling]

---

Phase 7: Review Summary

Review Output

┌─────────────────────────────────────────────────────────────┐
│  REVIEW SUMMARY: [PR/Change Description]                    │
├─────────────────────────────────────────────────────────────┤
│  Verdict: APPROVE / REQUEST CHANGES / NEEDS DISCUSSION      │
├─────────────────────────────────────────────────────────────┤
│  Stats:                                                     │
│  • Blockers: [count]                                        │
│  • Major: [count]                                           │
│  • Minor: [count]                                           │
│  • Nits: [count]                                            │
│  • Praise: [count]                                          │
├─────────────────────────────────────────────────────────────┤
│  Top Issues:                                                │
│  1. [Most important issue]                                  │
│  2. [Second most important]                                 │
│  3. [Third most important]                                  │
├─────────────────────────────────────────────────────────────┤
│  What's Good:                                               │
│  • [Positive observation]                                   │
│  • [Another positive]                                       │
└─────────────────────────────────────────────────────────────┘

For GitHub PR Comment

## Review Summary

**Verdict**: [APPROVE / REQUEST CHANGES]

### Blockers (must fix)
- [ ] [Issue 1]
- [ ] [Issue 2]

### Should Fix
- [ ] [Issue 3]
- [ ] [Issue 4]

### Consider
- [ ] [Suggestion 1]
- [ ] [Suggestion 2]

### What's Good
- [Positive feedback]

---
*Reviewed with `/review` skill*

---

Self-Review Mode

When reviewing your own code before submitting:

┌─────────────────────────────────────────────────────────────┐
│  SELF-REVIEW CHECKLIST                                      │
├─────────────────────────────────────────────────────────────┤
│  Before requesting review:                                  │
│  [ ] I've run the tests locally                             │
│  [ ] I've tested the change manually                        │
│  [ ] I've reviewed my own diff line by line                 │
│  [ ] I've removed debug code and console.logs               │
│  [ ] I've added/updated tests                               │
│  [ ] I've updated documentation if needed                   │
│  [ ] The PR description explains the what and why           │
│  [ ] I've linked the related issue                          │
│  [ ] I'm not embarrassed by any of this code                │
└─────────────────────────────────────────────────────────────┘

---

Anti-Patterns

• Nitpicking style while missing bugs
• "LGTM" without actually reviewing
• Reviewing without understanding context
• Being harsh or condescending
• Blocking on personal preferences
• Not acknowledging good work
• Reviewing too much at once (fatigue)

---

Output

1. Review context and scope
2. Architecture assessment
3. Detailed findings by severity
4. Security checklist results
5. Testing coverage assessment
6. Review summary with verdict
7. GitHub-ready comment (if PR)