Review checklist

This is the checklist to run down — by eye, on any diff, yours or an AI’s — after the tools in Verification Tools have run and come back clean. It fits on one printed page on purpose: a checklist nobody actually opens during a review is worthless. Every line links to the lesson that explains why, so this page stays a list, not a re-run of the course.

Errors

• Does every error return get checked before the value next to it is used?
• Is any error handled twice — logged and returned up the stack? See Error Values.
• Does a := inside an if/for accidentally shadow an outer err? See Variables & Types.
• Is a concrete error type ever assigned to an error-typed variable and returned as-is? See A nil pointer inside an interface is not nil.
• Does every errors.As call pass a pointer (&target), not the target itself? See Wrapping Errors.
• Is recover() called directly inside the deferred function — never through a helper one call away? See Panic and Recover.
• Does any package outside main call log.Fatal or os.Exit, skipping its callers’ defers? See Panic and Recover.

Concurrency

• Does every goroutine have a visible exit condition — a closed channel, a cancelled ctx, or a value it is waiting for? See Goroutines.
• Is wg.Add called before the goroutine starts, from the same goroutine that calls wg.Wait? See Goroutines.
• Does every goroutine that can panic recover from inside itself? See Panic and Recover.
• Is any type containing a sync.Mutex (or similar) ever used or passed by value? See The sync package.
• If a loop launches goroutines or defers closures, does the go.mod go directive actually guarantee per-iteration variables (1.22+), or is the loop var captured explicitly? See Functions.
• Does a bare break inside select inside for actually reach the loop it looks like it targets? See Control Flow.
• Does background work reuse a request-scoped context.Context it should have detached with context.WithoutCancel, or get stored in a struct field? See Context.

Resources

• Is every defer that closes/unlocks/releases something scoped to one acquisition, not accumulating across a loop? See io and bufio.
• Is a resource used (resp.Body, a file, a lock) only after the error from acquiring it has been checked? See The client side.
• Does every http.Client used outside a quick script set Timeout? See The client side.

API contract

• Is resp.StatusCode checked before the response body is trusted as data? See The client side.
• Does omitempty sit on a time.Time or struct field expecting it to omit the zero value? See encoding/json.
• Does every field the JSON contract needs actually get exported (and tagged)? See encoding/json.
• Is a nil slice returned where the contract requires [], or vice versa? See encoding/json.

Types

• Does an append on a sub-slice risk overwriting a parent slice’s backing array? See Slices.
• Is a map ever written to before it has been initialized with make or a literal? See Maps.
• Does any call site still assume maps.Keys/maps.Values return a slice instead of an iter.Seq? See Iterators.

Tooling & style

• Is every time.Sleep/duration literal explicit about its unit (5 * time.Second, never a bare 5)? See time.
• Does a new interface have more than one real implementation — or is any/a concrete type simpler? Is interface{} used where any reads better? Is a receiver named after the type, not this/self? See Interfaces and Methods.

🟦 A checklist narrows the search, it does not replace it

Every line above maps to one of the 24 patterns in the AI Bug Catalog — half of which no tool in this course catches automatically. Run this list on every diff regardless of how clean the linters came back.

Which group does 'does every http.Client set Timeout' belong to, and why that group rather than 'API contract'?

API contract — it is about what a caller can expect from a response Resources — a Client with no Timeout can hold a connection (a resource) open indefinitely Types — http.Client is a struct type Tooling & style — it is a matter of code style, not correctness

Why does the checklist tell you to check 'is recover() called directly inside the deferred function' rather than just 'does the code call recover() somewhere'?

recover() only stops a panic when called directly by a deferred function — one level of indirection through a helper makes it silently ineffective recover() is deprecated in modern Go recover() must be called before defer, not after There is no difference — both phrasings check the same thing

The checklist says to check the go.mod go directive for loop-variable capture bugs. Why does the directive matter here?

It doesn't — this bug is identical on every Go version Go 1.22 changed for-loop variables to be scoped per iteration, so the same capturing code is a bug pre-1.22 and correct at 1.22+ The go directive only affects which stdlib version is available Older go directives disable goroutines entirely

Why is 'does a new interface have more than one real implementation' on this checklist at all, given no linter can check it?

It shouldn't be — checklists should only contain things a tool can verify A checklist is exactly for judgment calls no tool makes; that is the gap human review exists to fill golangci-lint can check this if you enable enough linters Interfaces with one implementation are always a compile error

Submit

Mark as complete