houseofmvps/ultraship/code-review

Code Review

Review code the way a principal engineer would — not just "does it work?" but "will this cause problems at 3am?"

Review Dimensions

Every review should evaluate these dimensions, in order of importance:

Use an LSP if one is connected. If LSP tools are available (check your tools for a language server — TypeScript, Pyright, gopls, rust-analyzer), use them instead of grep-guessing. find references on a changed function tells you the real blast radius; go to definition confirms a call signature actually matches; rename/diagnostics surface type errors the eye misses. A review that verifies call sites with an LSP catches breakage that a text-only review ships. If no LSP is connected, fall back to Grep/Glob and say so.

1. Correctness

The code must do what it claims to do.

• Does the logic match the requirements/spec?
• Are edge cases handled? (empty input, null, max values, concurrent access)
• Are error paths tested, not just happy paths?
• Does it handle the "what if this is called twice?" scenario?
• Are race conditions possible? (async operations, shared state, database transactions)

2. Security

Think like an attacker for every piece of new code.

• Input validation: Is user input validated before use? (URL params, request body, query strings)
• IDOR: Can User A access User B's data by changing an ID? (check every route with :id params)
• Injection: Is user input ever interpolated into SQL, shell commands, or HTML?
• Auth: Are new endpoints protected by auth middleware? Are permissions checked, not just authentication?
• Secrets: Are any credentials hardcoded? Any new env vars documented?
• Data exposure: Do API responses leak internal fields? (password hashes, internal IDs, other users' data)

3. Performance

Will this work at 10x the current load?

• N+1 queries: Database calls inside loops. The #1 performance killer in web apps.
• Missing indexes: New columns used in WHERE/JOIN without index.
• Unbounded queries: findMany() without take/limit. Will return 1M rows when the table grows.
• Sync I/O: readFileSync, execSync in request handlers. Blocks the event loop.
• Sequential awaits: Independent awaits that should be Promise.all().
• Memory leaks: Module-scoped arrays with .push(), event listeners added in request handlers.
• Over-fetching: Selecting all columns when only 2 are needed. Returning full objects when IDs suffice.

4. Maintainability

Will the next person (including future-you) understand this in 6 months?

• Naming: Do variable/function names describe what they do, not how they do it?
• Complexity: Can any function be broken into smaller, testable pieces?
• Abstraction level: Is the code at a consistent level of abstraction? (mixing HTTP parsing with business logic is a smell)
• DRY violations: Is the same logic duplicated in multiple places?
• Dead code: Are there unused functions, imports, or variables?
• Comments: Are they explaining "why," not "what"? Comments that restate the code are noise.

5. Architecture

Does this fit the existing patterns, or does it introduce divergence?

• Pattern consistency: Does the new code follow the patterns established in the codebase?
• Coupling: Does this create tight coupling between modules that should be independent?
• Layer violations: Is a UI component making direct database calls? Is an API route doing business logic inline?
• Interface design: Are the function signatures clean? Could the API be simpler?

Confidence Scoring

Every finding should include a confidence level:

Don't cry wolf. A review that flags 30 "high" issues when only 3 are real trains the developer to ignore reviews. Be precise.

Output Format for /ship

When invoked by /ship, output findings with severity levels (critical/high/medium/low/info) in the same format as other auditors:

{
  "category": "code-quality",
  "findings": [
    { "severity": "high", "category": "code-quality", "file": "path", "line": N, "message": "description" }
  ]
}

Review Checklist (use mentally, don't output)

• [ ] Every new function has tests
• [ ] Every new route has auth middleware (if the app has auth)
• [ ] Every database query has appropriate indexes
• [ ] Every user input is validated
• [ ] No secrets in code
• [ ] No console.logs left in production code
• [ ] Error handling returns appropriate status codes
• [ ] API responses don't leak internal fields
• [ ] New dependencies are justified (not just convenience)
• [ ] The change is reversible (can be rolled back without data loss)

Key Principles

• Review the change, not the file. Focus on what's new or modified. Don't nit-pick pre-existing code unless it's directly related to the change.
• Offer fixes, not just complaints. "This has an N+1 query" is unhelpful. "This has an N+1 query — move the query outside the loop and pass the results as a lookup map" is a review.
• Distinguish between blocking and non-blocking. Be explicit: "This must be fixed before merge" vs. "This is a suggestion for a follow-up PR."
• Assume good intent. The developer made the best choice they could with the information they had. Your job is to add information, not judgment.