Code Review Culture for Go

Review norms, nit guidelines, and teaching through PRs.

Pull requests are the main classroom for Go idioms on most teams.

A healthy review culture catches bugs early, spreads knowledge, and keeps APIs stable without burning out authors or reviewers.

Summary

Go review culture balances fast merge with high trust.

Reviewers focus on behavior under failure, concurrency safety, exported API promises, and operational hooks.

Cosmetic preferences yield to gofmt and linters.

Teaching happens in comments with rationale and links, not unexplained demands.

Recipe

Quick-reference recipe card - copy-paste ready.

## PR description checklist (author)
 
- [ ] Linked issue or incident ticket
- [ ] go test ./... and golangci-lint run (paste or CI link)
- [ ] -race run if touching goroutines, maps shared across goroutines, or sync primitives
- [ ] Note breaking API changes and migration steps
- [ ] Screenshots or sample log lines for observable behavior changes

When to reach for this:

• Onboarding hires before their first feature PR
• Team retro after review latency or quality complaints
• Turning repeated review comments into linter rules or ADRs
• Calibrating senior/staff expectations in leveling discussions

Working Example

Reviewer examines a handler PR that returns errors incorrectly.

// Before - reviewer blocks: wraps without %w, logs and returns same err
func (s *Server) fetchUser(ctx context.Context, id string) (*User, error) {
    u, err := s.repo.Get(ctx, id)
    if err != nil {
        log.Printf("get user: %v", err)
        return nil, fmt.Errorf("get user: %v", err)
    }
    return u, nil
}

// After - single handling policy: wrap with %w, map to HTTP at handler edge
func (s *Server) fetchUser(ctx context.Context, id string) (*User, error) {
    u, err := s.repo.Get(ctx, id)
    if err != nil {
        return nil, fmt.Errorf("fetch user %s: %w", id, err)
    }
    return u, nil
}
 
func (s *Server) handleUser(w http.ResponseWriter, r *http.Request) {
    u, err := s.fetchUser(r.Context(), r.PathValue("id"))
    if err != nil {
        if errors.Is(err, ErrNotFound) {
            http.NotFound(w, r)
            return
        }
        http.Error(w, "internal error", http.StatusInternalServerError)
        return
    }
    json.NewEncoder(w).Encode(u)
}

What this demonstrates:

• Libraries and service layers wrap with %w for errors.Is at boundaries.
• Logging happens once at the HTTP edge with request correlation, not on every layer.
• Review comment cites team error ADR and links to error-handling docs.
• Author adds table test for ErrNotFound mapping.

Deep Dive

How It Works

• Author proves CI-green and describes risk.
• Reviewer prioritizes: correctness, API stability, concurrency, observability, then readability.
• Nit threshold: if three PRs repeat the same style comment, automate it or document an exception.
• Turnaround: default 24-hour first response on business days; escalate blocked PRs in standup.

Review Priority Table

Go Notes

// Reviewers expect context as first param after receiver
func (c *Client) Do(ctx context.Context, req *Request) (*Response, error)
 
// Flag storing context in structs
type Bad struct {
    ctx context.Context // review: do not store context
}

Teach accept interfaces, return structs by showing a minimal consumer-defined interface in test files.

Nit Guidelines

• Always block: ignored errors, race risks, breaking exports without version plan, secrets in code.
• Usually block: missing context propagation, panics for expected errors in libraries, unbounded goroutines.
• Discuss: performance micro-opts without profiles, naming bikesheds, alternative patterns with trade-offs.
• Automate: formatting, import order, common staticcheck findings via golangci-lint.

Gotchas

• Review tone as gatekeeping - hires stop asking questions. Fix: ask questions ("What happens if ctx is canceled?") and link docs.
• LGTM without reading tests - regressions slip. Fix: require test diff summary in PR template.
• Blocking on non-team idioms - reviewer imports past employer rules. Fix: cite team ADR or open ADR to change rule.
• Endless review rounds on large PRs - fatigue and merge conflicts. Fix: split PRs; use design comment before 500-line dumps.
• Silent approval on concurrency - incidents later. Fix: mandatory second reviewer for sync, channels, or errgroup touches.
• Reviewers never approve - throughput dies. Fix: time-box review slots; delegate when overloaded.

Alternatives

FAQs

Related

• Onboarding Go Specialists - review as ramp layer 2
• Pairing & Mob Sessions for Go Teams - pre-review pairing
• Documentation & godoc Conventions - export comment norms
• Effective Go Rules Checklist - automatable idioms
• Code Review Standards for Go Teams - lint and CI alignment

Stack versions: This page was written for Go 1.26.x (Green Tea GC default, go fix modernizers - verify patch at build), chi (latest - verify at build), gin (latest - verify at build), echo (latest - verify at build), google.golang.org/grpc (latest - verify at build), sigs.k8s.io/controller-runtime (latest - verify at build), kubebuilder (latest - verify at build), tinygo (latest - verify board targets at build), wazero (latest - verify at build), and golangci-lint (latest - verify linter set at build).