go-code-review

What this skill does

# Go Code Review

## Review Workflow

Follow this sequence **in order**. Do not emit findings until every **Pass** below is satisfied.

1. **Baseline `go.mod`** — Open `go.mod` and read the `go` directive.  
   **Pass:** You can state the exact `go X.YY` value (in the review preamble or working notes). Apply version-gated advice only when it matches this baseline (loop capture pre-1.22, `slog`/structured logging from 1.21, `errors.Join` from 1.20).

2. **Read surrounding code** — For each changed `.go` file, read full functions or logical units that contain the edits, not only the diff hunk.  
   **Pass:** At least one full enclosing function (or package-level `init`/var block) containing the change was read per changed file.

3. **Scope the checklist** — Decide which [Review Checklist](#review-checklist) blocks apply (error handling, concurrency, interfaces/types, resources, naming). Load [references](#quick-reference) for those blocks; skip blocks that are irrelevant to the diff.  
   **Pass:** The review (or working notes) lists which checklist blocks you applied, or marks blocks N/A with a one-line reason tied to the diff (e.g. “no concurrency in change”).

4. **Pre-report verification** — Load and follow [review-verification-protocol](../review-verification-protocol/SKILL.md).  
   **Pass:** The protocol’s **Pre-Report Verification Checklist** is satisfied for each finding you will report (actual code read, surrounding context checked, “wrong” vs “different style” distinguished, etc.).

## Hard gates (same sequence, shorter)

| Step | Objective pass condition |
| --- | --- |
| 1 | `go X.YY` from `go.mod` is recorded before version-specific advice. |
| 2 | Full enclosing context read per changed file, not diff-only. |
| 3 | In-scope checklist blocks listed or N/A with diff-tied reason; references opened as needed. |
| 4 | `review-verification-protocol` completed for every reported issue. |

## Output Format

Report findings as:

```text
[FILE:LINE] ISSUE_TITLE
Severity: Critical | Major | Minor | Informational
Description of the issue and why it matters.
```

## Quick Reference

| Issue Type | Reference |
|------------|-----------|
| Missing error checks, wrapping, errors.Join | [references/error-handling.md](references/error-handling.md) |
| Race conditions, channel misuse, goroutine lifecycle | [references/concurrency.md](references/concurrency.md) |
| Interface pollution, naming, generics | [references/interfaces.md](references/interfaces.md) |
| Resource leaks, defer misuse, slog, naming | [references/common-mistakes.md](references/common-mistakes.md) |

## Review Checklist

### Error Handling
- [ ] All errors checked (no `_ = err` without justifying comment)
- [ ] Errors wrapped with context (`fmt.Errorf("...: %w", err)`)
- [ ] `errors.Is`/`errors.As` used instead of string matching
- [ ] `errors.Join` used for aggregating multiple errors (Go 1.20+)
- [ ] Zero values returned alongside errors

### Concurrency
- [ ] No goroutine leaks (context cancellation or shutdown signal exists)
- [ ] Channels closed by sender only, exactly once
- [ ] Shared state protected by mutex or sync types
- [ ] WaitGroups used to wait for goroutine completion
- [ ] Context propagated through call chain
- [ ] Loop variable capture handled (pre-Go 1.22 codebases only)

### Interfaces and Types
- [ ] Interfaces defined by consumers, not producers
- [ ] Interface names follow `-er` convention
- [ ] Interfaces minimal (1-3 methods)
- [ ] Concrete types returned from constructors
- [ ] `any` preferred over `interface{}` (Go 1.18+)
- [ ] Generics used where appropriate instead of `any` or code generation

### Resources and Lifecycle
- [ ] Resources closed with `defer` immediately after creation
- [ ] HTTP response bodies always closed
- [ ] No `defer` in loops without closure wrapping
- [ ] `init()` functions avoided in favor of explicit initialization

### Naming and Style
- [ ] Exported names have doc comments
- [ ] No stuttering names (`user.UserService` → `user.Service`)
- [ ] No naked returns in functions > 5 lines
- [ ] Context passed as first parameter
- [ ] `slog` used over `log` for structured logging (Go 1.21+)

## Severity Calibration

### Critical (Block Merge)
- Unchecked errors on I/O, network, or database operations
- Goroutine leaks (no shutdown path)
- Race conditions on shared state (concurrent map access without sync)
- Unbounded resource accumulation (defer in loop, unclosed connections)

### Major (Should Fix)
- Errors returned without context (bare `return err`)
- Missing WaitGroup for spawned goroutines
- `panic` for recoverable errors
- Context not propagated to downstream calls

### Minor (Consider Fixing)
- `interface{}` instead of `any` in Go 1.18+ codebases
- Missing doc comments on exports
- Stuttering names
- Slice not preallocated when size is known

### Informational (Note Only)
- Suggestions to add generics where code generation exists
- Refactoring ideas for interface design
- Performance optimizations without measured impact

## When to Load References

- Reviewing error return patterns → error-handling.md
- Reviewing goroutines, channels, or sync types → concurrency.md
- Reviewing type definitions, interfaces, or generics → interfaces.md
- General review (resources, naming, init, performance) → common-mistakes.md

## Valid Patterns (Do NOT Flag)

These are acceptable Go patterns — reporting them wastes developer time:

- **`_ = err` with reason comment** — Intentionally ignored errors with explanation
- **Empty interface / `any`** — For truly generic code or interop with untyped APIs
- **Naked returns in short functions** — Acceptable in functions < 5 lines with named returns
- **Channel without close** — When consumer stops via context cancellation, not channel close
- **Mutex protecting struct fields** — Even if accessed only via methods, this is correct encapsulation
- **`//nolint` directives with reason** — Acceptable when accompanied by explanation
- **Defer in loop** — When function scope cleanup is intentional (e.g., processing files in batches)
- **Functional options pattern** — `type Option func(*T)` with `With*` constructors is idiomatic
- **`sync.Pool` for hot paths** — Acceptable for reducing allocation pressure in performance-critical code
- **`context.Background()` in main/tests** — Valid root context for top-level calls
- **`select` with `default`** — Non-blocking channel operation, intentional pattern
- **Short variable names in small scope** — `i`, `err`, `ctx`, `ok` are idiomatic Go

## Context-Sensitive Rules

Only flag these issues when the specific conditions apply:

| Issue | Flag ONLY IF |
|-------|--------------|
| Missing error check | Error return is actionable (can retry, log, or propagate) |
| Goroutine leak | No context cancellation path exists for the goroutine |
| Missing defer | Resource isn't explicitly closed before next acquisition or return |
| Interface pollution | Interface has > 1 method AND only one consumer exists |
| Loop variable capture | `go.mod` specifies Go < 1.22 |
| Missing slog | `go.mod` specifies Go >= 1.21 AND code uses `log` package for structured output |

## Before Submitting Findings

Satisfy **step 4** in [Review Workflow](#review-workflow): load [review-verification-protocol](../review-verification-protocol/SKILL.md) and complete its pre-report checks for each issue.