skills/code-review/SKILL.md

name, description

name	description
code-review	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

// 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?

// 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?

// 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

// 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)

// 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

// 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:

## 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.