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:

[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
Race conditions, channel misuse, goroutine lifecycle	references/concurrency.md
Interface pollution, naming, generics	references/interfaces.md
Resource leaks, defer misuse, slog, naming	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 before reporting any issue.