Agent Skills: Go Code Review

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

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

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

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