How to review Go pull requests: the 9 defect classes
Go pull requests are short and explicit, so a reviewer's eye slides over them. The bugs are often in what is missing: an error that is never checked, a timeout that is never set, a lock that is never taken. This guide walks the nine defect classes DiffDojo trains, in the order a reviewer should weigh them, with what each one looks like in Go code.
1. Security
Look for request values reaching an interpreter: SQL built with fmt.Sprintf instead of placeholders, a file name from a request joined with filepath.Join without checking that the result stays inside the base directory, or a redirect target. Then check object-level authorization: a handler that loads a row by id must also check that it belongs to the caller.
2. Requirement fidelity
Read the ticket before the code. The sort order, which records a query includes, whether a range includes its end and the field a total is computed from are where Go PRs most often do something reasonable that is not what was asked.
3. Edge cases & error handling
Ask what happens on empty, missing and huge input, and on every error. Typical misses: an err that is ignored, a nil pointer dereferenced on a path the tests never take, and outbound calls with no deadline at all, such as http.Get on the default client, which has no timeout.
4. Hallucinated APIs & dependencies
Models invent plausible functions and options: a standard library function that does not exist, a method from another major version of a module, an option a client never had. The compiler catches a call that does not exist, so look harder at what compiles and is still wrong, such as a struct tag or a configuration key that nothing reads.
5. State & data flow
Watch for state shared between goroutines: a map written from several goroutines without a mutex (a data race that can crash the program), a loop variable captured by a goroutine in a module older than Go 1.22, or a package-level variable used as per-request state. Ask how each piece of shared state is synchronized.
6. Performance
Look for work that grows with the input: a query per row inside a loop (N+1), a goroutine started for every item with no limit, or a slice scanned inside a loop over a large input. With small test data none of it is noticeable, so ask what the code does at the production size the ticket implies.
7. Data correctness
Money and time are the usual suspects. float64 for currency, integer division that truncates where the ticket expects rounding, and time.Time values compared with == (which also compares the location and the monotonic reading, so use Equal) all produce code that runs and returns the wrong answer.
8. Deceptive tests
A green test proves only what it asserts. Check for table-driven tests whose cases all take the same branch, a test that checks only that no error came back, and failures written with t.Log instead of t.Error, which never fail the test.
9. Over-engineering
Not a bug, but a review comment: an interface declared for a single implementation, a generic helper used with one type, or a framework for what a few lines of the standard library do. Ask for the simple version.
The checklist
- Does any request value reach SQL, a file path or a redirect?
- Does the change do exactly what the ticket says, field by field?
- Is every error checked, and does every outbound call have a timeout or a context?
- Does every unfamiliar function, method and option actually exist?
- Is any state shared between goroutines without synchronization?
- Is there a query or unbounded concurrency inside a loop?
- Are money and time handled without floats and with time.Equal?
- Would the tests fail if the code were wrong?
- Is there a simpler way to write this?
As of October 2026, the DiffDojo library has 23 live Go PRs to practise on, each graded on whether you catch the planted bug or approve the clean one.
Practise
Review a Go PR with the answer hidden.
Leave inline comments and a verdict, then see what you caught. Free, no signup.
Open the Go library →