go-code-review

Go Code Review

A repeatable, opinionated review pass for Go code. Read the diff file by file, walk each topic in order, flag findings with file:line references, then group by severity.

Core Rules

1. Mechanical checks first. Never start a human review until gofmt, go vet, and golangci-lint are clean. They free your attention for what tools cannot catch.
2. One file at a time, topic by topic. Walk the diff in order and apply each topic checklist below. Switching topics mid-file loses the thread.
3. Every finding cites a rule. file:line plus the rule name (go-naming: initialisms) — never a bare opinion.
4. Severity is non-negotiable. Must-Fix (correctness/security/data-loss/broken contract) blocks merge; Should-Fix (significant design or style issue); Nit (small preference, flag once).
5. Drop what you cannot defend. After flagging, re-read and remove any finding you would not stand behind in a thread.
6. Praise non-trivial improvements. A review without acknowledgement teaches only avoidance.

Review Procedure

1. Run mechanical checks: gofmt -d ./..., go vet ./..., golangci-lint run ./..., go test ./... -race -short.
2. Read the diff one file at a time. For each file, walk the topic checklists below in order.
3. Flag every issue with file:line and the rule name that justifies it.
4. After all files are reviewed, re-read flagged items and drop any you cannot justify.
5. Group findings by Must Fix / Should Fix / Nit using the rubric below.

Use references/review-template.md when writing up the review for consistent severity grouping and tone.

Automated Checks

gofmt -l ./... && go vet ./... && golangci-lint run ./... && go test ./... -race -short

Fix anything the tools find before continuing. See go-linting for setup.

Formatting

• gofmt/goimports clean; long lines break by semantics, not column count

Documentation

• Exported symbols documented (starts with name, ends with .); package comment adjacent to package clause; non-trivial unexported have intent comments; named returns only when they clarify

See go-documentation.

Error Handling

• No discarded errors (_ = f()) without a written justification
• Error strings are lowercase with no trailing punctuation
• Errors wrap with %w when the caller may want to inspect; %v only when hiding is deliberate
• No in-band magic values (-1, "", nil) for failure — multi-return with an error or ok bool
• Error path comes first; the success path stays unindented
• Each error is handled exactly once (log or return, not both)

See go-error-handling.

Naming

• MixedCaps / mixedCaps only — no underscores or SCREAMING_SNAKE
• Initialisms are uniformly cased: URL, ID, HTTP, XMLHTTPRequest, serveHTTP
• Short names for short scopes (i, r, ctx); longer names for wider scopes
• Receiver names are one or two letters, consistent across methods; no this/self/me
• Packages don't stutter (http.Server, not http.HTTPServer)
• Package names avoid util, helpers, common, misc
• No identifiers shadow builtins (len, error, cap, new, make, copy, any)

See go-naming and go-packages.

Declarations

• Related var/const/type are in grouped blocks; unrelated kept separate
• var for intentional zero values; := for computed locals
• Variables scoped as narrowly as is readable (if-init where it fits)
• Struct literals use field names; zero-value fields are omitted
• Enums start at iota + 1 (or zero is explicitly meaningful)
• any instead of interface{} in new code

See go-declarations.

Control Flow

• No else after a returning/breaking/continuing if; no := shadowing of outer ctx/err; map iteration is order-agnostic; labeled break/continue for switch-in-loop

See go-control-flow.

Functions

• File order: type → constructor → exported → unexported → utilities; wrapped signatures one-per-line; no pointer-to-interface; bool/int params renamed via type or commented; printf-style helpers end in f

See go-functions.

Interfaces

• Defined in the consumer package; not "just for mocking" on the implementor; consistent receivers per type; compile-time var _ I = (*T)(nil) on exported implementations

See go-interfaces.

Concurrency

• Goroutine lifetimes clear (bounded by ctx.Done() or documented); APIs synchronous by default; context.Context first param, never struct field; lock order documented; sender closes channels

Example to flag (pkg/worker/worker.go:42):

go s.process(req)   // ✗ no ctx, no done signal — leak on shutdown

vs. acceptable:

s.wg.Add(1)
go func() { defer s.wg.Done(); s.process(ctx, req) }()

See go-concurrency.

Data Structures

• var t []T for nil slices, []T{} only when empty non-nil is required (JSON output); copies of structs containing sync.Mutex flagged; slice/map at API boundaries copied or borrowing documented

See go-data-structures.

Security & Logging

• crypto/rand for secret material (never math/rand); no library panic for ordinary failures
• log/slog (not log/fmt.Println); static message + structured attrs; secrets and PII never logged

See go-defensive and go-logging.

Imports

• Grouped: stdlib → external → local; no rename unless collision; no blank import outside main/tests; no dot imports

See go-packages.

Generics

• Justified by ≥2 real call sites; constraint is the loosest that compiles; no generics-just-for-an-interface

See go-generics.

Testing

• Tests cover the new behavior at the right level; failure messages include what/inputs/got/want
• httptest.NewServer over hand-rolled mocks; TestMain only when truly necessary; Example* for non-trivial APIs

Read references/integrative-example.md to see the rules applied together on a small HTTP server.

Read references/severity-rubric.md when deciding whether to label a finding must-fix, should-fix, or nit.

Quick Severity Rubric

Severity	Examples
Must Fix	Race, security bug, swallowed error, broken API, data loss
Should Fix	Wrong layer for an interface, panic in a library, leaky goroutine
Nit	Name preference, comment phrasing, ordering within a block

Anti-Patterns

These are reviewer anti-patterns — bad habits to avoid when writing the review itself:

Anti-pattern	Do this instead
"I would have written this differently"	drop it, or cite a concrete rule
Listing every nit you noticed	flag once, note "× N similar"
No severity labels; comment without file:line	label Must / Should / Nit; anchor with path:line
Reviewing the author, not the code	write about the change, not the person
Re-doing the work in the review	point to the rule and let them write it
Approving without reading tests	tests are part of the diff

Verification Checklist

• Every finding has file:line + a rule citation; no bare opinions
• No duplicates (note "× N similar" if widespread); severity matches the rubric
• Tests were read, not just counted
• Praise included for non-trivial improvements; tone is about the change, never the author

References

• review-template.md — markdown shape for posting a review
• severity-rubric.md — extended must/should/nit guidance
• integrative-example.md — sample HTTP server walked through