go-code-review

# Go Code Review

## Review Workflow

Follow this sequence to avoid false positives and catch version-specific issues:

1. **Check `go.mod`** — Note the Go version. This determines which patterns apply (loop variable capture is only an issue pre-1.22, `slog` is available from 1.21, `errors.Join` from 1.20). Skip version-gated checks that don't apply.
2. **Scan changed files** — Read full functions, not just diffs. Many Go bugs hide in what surrounds the change.
3. **Check each category** — Work through the checklist below, loading references as needed.
4. **Verify before reporting** — Load beagle-go:review-verification-protocol before submitting findings.

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

Load and follow [review-verification-protocol](../review-verification-protocol/SKILL.md) before reporting any issue.