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

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

1. Scope the checklist — Decide which Review Checklist blocks apply (error handling, concurrency, interfaces/types, resources, naming). Load references 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”).

1. Pre-report verification — Load and follow review-verification-protocol.

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)

Output Format

Report findings as:

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

Quick Reference

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:

Before Submitting Findings

Satisfy step 4 in Review Workflow: load review-verification-protocol and complete its pre-report checks for each issue.