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

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

Load and follow review-verification-protocol before reporting any issue.

Converted and distributed by TomeVault — claim your Tome and manage your conversions.