---
name: code-review
description: Systematic code smell detection and Go-specific review checklist. Load before any pre-merge review.
---

# Code Review

## Overview

Code review is smell detection and principle verification, not style policing. The goal is to identify patterns that make code hard to understand, change, test, or safely operate.

**This skill is read-only analysis.** When you find issues, document them with specific file:line references and prioritize by architectural impact. For fixes, load the `refactoring` or `clean-code` skill.

## When to Use

- Pre-merge review of any pull request
- Before committing a significant change
- When asked to "review this code"
- After the GREEN phase of TDD (use with `clean-code` skill)

## Review Process

### Phase 0: Iron Laws (block on any violation)

Before reading the diff, scan for these. Any violation is a blocking finding regardless of how clean the rest of the change looks.

1. **No security vulnerabilities** — command injection, SQL injection, credential exposure, path traversal, unchecked input at system boundaries
2. **No silently swallowed errors** — `err != nil` without wrapping, logging, or surfacing is always wrong; `_ = f()` on a fallible call is wrong
3. **No missing validation at system boundaries** — user input, external API responses, and file reads must be validated before use

If any iron law is violated, flag it as **Blocking** before any other phase. The other phases still run, but the iron-law findings come first in the report. See Phase 3 for detailed patterns and code examples covering iron laws 2 and 3.

### Phase 1: Context

Before examining code:
1. Understand what the change is trying to accomplish
2. Identify the affected packages and their purpose
3. Check test coverage — is the behavior tested?
4. Read the commit message and PR description

### Phase 2: Architectural Analysis (Highest Priority)

Check for structural problems first. These are expensive to fix later.

**Dependency violations:**
- Does domain logic import infrastructure packages?
- Are there circular imports between packages?
- Does a type do things that multiple different stakeholders would request changes to?

**Abstraction problems:**
- Is there a large switch/if-else on a type string that should be an interface?
- Are there God types that know too much about too many things?
- Is there Shotgun Surgery — one logical change requires editing many files?

### Phase 3: Go-Specific Checklist

#### Error Handling

```go
// Required: wrap with context
return fmt.Errorf("parse invoice: %w", err)

// Forbidden: naked return
return err

// Forbidden: log and return (pick one)
log.Error("failed", "err", err)
return err

// Forbidden: silently ignore
_ = doSomething()

// Suspicious: sentinel errors without documentation
var ErrFoo = errors.New("foo") // Is this exported? Is it documented?
```

#### Goroutine Safety

- Is shared state protected by a mutex or accessed only via channels?
- Are goroutines guaranteed to terminate (no goroutine leaks)?
- Is `sync.WaitGroup` used correctly (Add before go, Done via defer)?
- Is there a `go test -race` passing?

```go
// Check: is this safe for concurrent use?
type Cache struct {
    mu    sync.RWMutex // Good: explicit lock
    items map[string]Item
}

// Red flag: map access without lock in concurrent context
type BadCache struct {
    items map[string]Item // Unsafe if accessed from multiple goroutines
}
```

#### Exported API Minimalism

- Only export what callers outside the package actually need
- Internal implementation details should be unexported
- Does the exported API make sense as a standalone contract?

```go
// Good: minimal exported surface
type UserService struct { /* unexported fields */ }
func NewUserService(...) *UserService { ... }
func (s *UserService) GetUser(ctx context.Context, id UserID) (User, error) { ... }

// Bad: over-exported internals
type UserService struct {
    Store  UserStore  // Exported field — callers can replace it, breaks encapsulation
    Logger *slog.Logger // Exported — callers can mutate it
}
```

#### Dependency Safety

Before adding any new dependency:
- Has `govulncheck` been run? (`govulncheck ./...`)
- Is there a justification in the commit message for non-stdlib additions?
- Pre-approved without justification: `testify`, `slog`, `templ`, `sqlc`
- Everything else needs a reason

#### Generic/interface{} Usage

```go
// Bad: any/interface{} when a concrete type or generic works
func Process(data interface{}) error { ... }

// Good: typed
func Process[T Processor](data T) error { ... }

// Good: concrete type when only one type is expected
func ProcessOrder(o *Order) error { ... }
```

#### Context Propagation

- Every function that does I/O (DB, HTTP, file) should accept `ctx context.Context` as the first parameter
- Context should be passed down, not stored in structs
- `context.Background()` should only appear at the top-level (main, test setup)

```go
// Good
func (s *Store) GetUser(ctx context.Context, id UserID) (User, error) { ... }

// Bad: no context — can't cancel, can't trace, can't timeout
func (s *Store) GetUser(id UserID) (User, error) { ... }

// Bad: context stored in struct
type Handler struct {
    ctx context.Context // Don't do this
}
```

#### Logging with slog

```go
// Good: structured, leveled
slog.InfoContext(ctx, "user created", "user_id", u.ID, "email", u.Email)

// Bad: unstructured
log.Printf("user created: %s", u.Email)

// Bad: logging errors that are also returned
slog.Error("failed to save", "err", err)
return fmt.Errorf("save user: %w", err) // caller is double-counting
```

### Phase 4: Code Smell Detection

Apply the catalog from `clean-code/references/code-smells.md`. Key smells to prioritize:

**Architectural impact (fix immediately):**
- Global Data — package-level variables modified at runtime
- Shotgun Surgery — one change touches many unrelated files
- Feature Envy — a method uses another package's internals more than its own
- God Object — a type that does everything

**Design impact (fix before merge if possible):**
- Long Method — functions > ~50 lines in Go (Go can be longer than OOP, but be honest)
- Long Parameter List — > 4-5 parameters; consider a config struct
- Duplicate Code — same logic copy-pasted (wait for Rule of Three)
- Message Chain — `a.B().C().D()` — violates Law of Demeter
- Primitive Obsession — using `string` for email, user ID, currency

**Readability (note in review, fix if cheap):**
- Magic Numbers — bare literals without named constants
- Uncommunicative Names — `data`, `info`, `x`, `tmp`
- Fallacious Comments — comments that don't match the code

### Phase 5: Test Quality

- Are new behaviors covered by tests?
- Are tests table-driven where appropriate?
- Do tests test behavior, not implementation?
- Are there mock-only tests that test nothing real?

For comprehensive test review, load the `test-design` skill.

## Severity Classification

| Severity | Description | Action |
|----------|-------------|--------|
| **Blocking** | Security issue, data corruption risk, broken behavior | Must fix before merge |
| **High** | Architectural violation, goroutine safety, missing context | Fix before merge |
| **Medium** | Design smell, missing tests for changed behavior | Fix in this PR or create issue |
| **Low** | Naming, style, minor duplication | Note in review, optional |

## Output Format

When documenting findings:

```markdown
## Code Review: [package/file]

### Blocking
- `internal/store/user.go:47`: Error swallowed with `_ =` — callers can't know if Save failed

### High
- `internal/handler/order.go:12`: Domain logic in HTTP handler — moves tax calculation out of service layer
- `cmd/server/main.go:89`: Missing context propagation — `GetUser` call has no timeout

### Medium
- `internal/service/invoice.go:34`: Long method (87 lines) — extract `calculateLineItems` and `applyTaxRules`

### Low
- `internal/domain/user.go:5`: Field `Data string` — name reveals nothing about what data
```

## Brain MCP Integration

The brain MCP holds prior review decisions across sessions. Use it to avoid re-litigating the same tradeoffs.

**At review start:**
- Run `brain_query` with the package or feature name + "review" to surface prior review findings, recurring smells, or architectural decisions for this area. This prevents flagging the same smell that was already accepted as a tradeoff.

**After review completes:**
- For findings the author accepts as a tradeoff (with rationale), run `brain_write` to record the decision. Future reviews will not re-flag it.

**Never:**
- Embed brain queries in the review output. They are context for you, not findings for the author.

### Logging

Call `session_log` once at the end of every phase to record the outcome.
Pass-rate is computed downstream by the `/pass-rate` HTTP endpoint, which
treats `pass` as success, `fail` as failure, `skip` as neither.

**At end of each phase:**
- `session_log` with `{skill: "code-review", phase: "<phase-name>", final_status: "pass" | "fail" | "skip", message: "<one-line summary>", duration_ms: <wall-clock>, project_root: "<absolute path>"}`

**Phases for this skill:** phase-0-iron-laws, phase-1-context, phase-2-architectural-analysis, phase-3-go-checklist, phase-4-code-smell-detection, phase-5-test-quality

**Status semantics:**
- `pass` — the phase's intended outcome was reached.
- `fail` — the phase's intended outcome was NOT reached.
- `skip` — phase was skipped intentionally.

**Why this matters:** the routing pod (Plan 6) reads pass-rate to decide whether to route a future call to a local model. If your skill never logs, the routing pod sees no data.

## Common Go Anti-Patterns

| Anti-Pattern | What to Look For |
|-------------|-----------------|
| Goroutine leak | `go func()` without a done channel or WaitGroup |
| Error ignored | `_ = f()` or missing `if err != nil` after a fallible call |
| Nil map write | `m[k] = v` without `m = make(map[...]{})` initialization |
| Race on map | Concurrent map read/write without mutex |
| ctx in struct | `type T struct { ctx context.Context }` |
| Naked panic | `panic("something went wrong")` in non-initialization code |
| fmt.Println in library | Use `slog` or accept a logger; never print to stdout from lib code |
| init() abuse | `init()` with side effects that make testing hard |

## References

- `clean-code/references/code-smells.md` — comprehensive smell catalog
- Load `refactoring` skill when recommending fixes
- Load `test-design` skill for reviewing test quality
- Load `solid` skill for SOLID principle violations

## Mode 2 Routing Note

This skill is invoked identically in Mode 1 (cloud Claude) and Mode 2 (client-local with supervisor routing). The routing layer (Plan 6) does not exist yet; treat this skill as Mode 1 only for now. Findings format and severity classification are unchanged across modes.