/go-code-review

name: go-code-review
description: Reviews Go code for idiomatic patterns, error handling, concurrency safety, and common mistakes. Use when reviewing .go files, checking error handling, goroutine usage, or interface design. Covers generics (Go 1.18+), errors.Join and slog (Go 1.21+), and Go 1.22 loop variable semantics.

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:

[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