Review Go
Correctness and security review for Go. The checklist is the baseline and runs whether or not any
tool is installed; present tools sharpen it. A check that did not run is named in the report: never
omitted, never reported as clean.
Procedure
Follow ../../references/procedure.md. In short: probe, scope, run what exists, walk the checklist
by hand over every file in scope, score per ../../references/rubric.md, emit both artifacts.
Capability probe
command -v go # go vet, go build, go test
command -v staticcheck
command -v gosec
command -v govulncheck
command -v errcheck
| Tool |
Invocation |
If absent |
go vet |
go vet ./... |
install Go: nothing else here works either |
staticcheck |
staticcheck ./... |
go install honnef.co/go/tools/cmd/staticcheck@latest |
gosec |
gosec -quiet ./... |
go install github.com/securego/gosec/v2/cmd/gosec@latest |
govulncheck |
govulncheck ./... |
go install golang.org/x/vuln/cmd/govulncheck@latest |
errcheck |
errcheck ./... |
go install github.com/kisielk/errcheck@latest |
Several missing at once? The suite ships review-tools.sh, which probes and installs the whole
toolchain in one pass. Name it in ## Checks skipped and leave running it to the user: a review
reports, it does not install.
These tools exit non-zero when they find something. That is a result, not a crash: see the error
handling rules in ../../references/procedure.md. A build failure that stops analysis is a crash:
staticcheck cannot report on a package that does not compile, so a compile error means every
tool-based check is skipped, not passed.
Checklist
Walk every row against every file in scope. go vet catches a minority of these; the rest need
reading.
| ID |
Look for |
Why it matters |
Fix direction |
| GO-01 |
A value used after the error that produced it was non-nil: x, err := f(); if err != nil { log.Print(err) } then x.Field, or a return missing from the error branch. |
Go returns zero values alongside errors, so the "handled" path dereferences nil and panics. In a handler that is a remote crash. |
The error branch returns, continues, or assigns a usable fallback. No path reaches the value with the error unhandled. |
| GO-02 |
A one-value type assertion (v := x.(T)) where x is any/interface{} from JSON, a map, a context value, or a plugin boundary. |
A different concrete type panics and takes the process down. Untrusted input reaching one of these is a denial of service. |
Two-value form v, ok := x.(T) with an explicit !ok path, or a type switch with a default. |
| GO-03 |
go func() whose exit condition depends on a channel nobody may write, a send on an unbuffered channel with no reader, context.WithCancel/WithTimeout whose cancel is not deferred, or a goroutine that ignores ctx.Done(). |
Leaked goroutines and timers accumulate per request until the process dies. Leaks are invisible in tests and fatal in production. |
defer cancel() at every WithCancel/WithTimeout. Every goroutine has a select on ctx.Done() or a guaranteed-closed channel. |
| GO-04 |
A database/sql query string built with +, fmt.Sprintf, or strings.Join around a value that came from a request, a file, or a database. |
SQL injection. Identifiers cannot be parameterised, so this is often the real thing rather than a false positive. |
Placeholders (?, $1) with args passed to Query/Exec. Where an identifier must vary, validate it against a fixed allow-list of known column or table names. |
| GO-05 |
defer inside a for body: typically defer f.Close() or defer rows.Close(). |
Deferred calls run at function exit, not iteration exit, so descriptors and locks pile up for the whole loop. Over a large input set this exhausts the fd limit. |
Move the body into a function (named or closure) called per iteration, or close explicitly at the end of the iteration and on every error path. |
| GO-06 |
_ = f() on a function returning error, a bare call whose error is discarded, unchecked defer f.Close() on a writable file, or an ignored rows.Err() after a range over rows. |
Silent failure. A discarded write error means the caller reports success on data that never landed; a missing rows.Err() hides a truncated result set that looks like an empty one. |
Every error is checked, wrapped with context (fmt.Errorf("...: %w", err)), or explicitly ignored with a comment saying why it is safe. |
| GO-07 |
A map, slice, or struct field written from more than one goroutine with no mutex or channel discipline; sync.WaitGroup reused across rounds; a captured loop variable in a pre-1.22 module; lazy init with no sync.Once. |
Data races corrupt memory and crash non-deterministically. Concurrent map write is an unrecoverable fatal error: no recover, no graceful shutdown. |
Guard shared state with sync.Mutex/RWMutex, hand ownership to one goroutine over a channel, or use sync/atomic. Confirm with go test -race. |
Beyond the checklist, also weigh: context not threaded through call chains that do I/O, time.After
in a loop, unbounded io.ReadAll on a request body, and http.Client with no timeout.
Output
Both artifacts, always:
- Human report per the skeleton in
../../references/procedure.md: findings grouped by
severity, then a ## Checks skipped table naming every absent or crashed tool with its reason and
install hint.
- Agent prompt block per
../../references/agent-prompt.md, appended after that table, carrying
every severity. Omit the block entirely when there are no findings.
Derive the block's validation line from the probe. With the full toolchain present:
go build ./... && go vet ./... && staticcheck ./... && go test -race ./.... Drop each absent tool
rather than substituting a different one; go build ./... alone is a legitimate floor.
Common mistakes
- Passing off a
gosec hit as a finding without reading the code. gosec flags patterns, not
reachability. Read the call site, then assign confidence per ../../references/rubric.md.
- Calling a missing tool a clean result. "
govulncheck reported nothing" and "govulncheck is
not installed" are different claims. The second belongs in ## Checks skipped.
- Skipping the manual walk because the tools were quiet. Authorisation gaps, wrong trust
boundaries, and misthreaded
context do not appear in any linter's output.
- Reporting
defer in a loop as Critical. It is a resource leak; severity depends on the
iteration count and process lifetime. Score consequence, not category.
1---2name: review-go3description: Use when reviewing or auditing Go code - .go files, a go.mod module, "review this Go service", "check this handler before I merge", "audit this package" - covering nil dereference after an error, unchecked type assertions, goroutine and context leaks, SQL built by string concatenation, defer inside loops, ignored errors, and data races on shared state.4---56# Review Go78Correctness and security review for Go. The checklist is the baseline and runs whether or not any9tool is installed; present tools sharpen it. A check that did not run is named in the report: never10omitted, never reported as clean.1112## Procedure1314Follow `../../references/procedure.md`. In short: probe, scope, run what exists, walk the checklist15by hand over every file in scope, score per `../../references/rubric.md`, emit both artifacts.1617## Capability probe1819```bash20command -v go # go vet, go build, go test21command -v staticcheck22command -v gosec23command -v govulncheck24command -v errcheck25```2627| Tool | Invocation | If absent |28|---|---|---|29| `go vet` | `go vet ./...` | install Go: nothing else here works either |30| `staticcheck` | `staticcheck ./...` | `go install honnef.co/go/tools/cmd/staticcheck@latest` |31| `gosec` | `gosec -quiet ./...` | `go install github.com/securego/gosec/v2/cmd/gosec@latest` |32| `govulncheck` | `govulncheck ./...` | `go install golang.org/x/vuln/cmd/govulncheck@latest` |33| `errcheck` | `errcheck ./...` | `go install github.com/kisielk/errcheck@latest` |3435Several missing at once? The suite ships `review-tools.sh`, which probes and installs the whole36toolchain in one pass. **Name it in `## Checks skipped` and leave running it to the user**: a review37reports, it does not install.3839These tools exit non-zero when they find something. That is a result, not a crash: see the error40handling rules in `../../references/procedure.md`. A build failure that stops analysis *is* a crash:41`staticcheck` cannot report on a package that does not compile, so a compile error means every42tool-based check is skipped, not passed.4344## Checklist4546Walk every row against every file in scope. `go vet` catches a minority of these; the rest need47reading.4849| ID | Look for | Why it matters | Fix direction |50|---|---|---|---|51| **GO-01** | A value used after the error that produced it was non-nil: `x, err := f(); if err != nil { log.Print(err) }` then `x.Field`, or a `return` missing from the error branch. | Go returns zero values alongside errors, so the "handled" path dereferences nil and panics. In a handler that is a remote crash. | The error branch returns, continues, or assigns a usable fallback. No path reaches the value with the error unhandled. |52| **GO-02** | A one-value type assertion (`v := x.(T)`) where `x` is `any`/`interface{}` from JSON, a map, a context value, or a plugin boundary. | A different concrete type panics and takes the process down. Untrusted input reaching one of these is a denial of service. | Two-value form `v, ok := x.(T)` with an explicit `!ok` path, or a type switch with a `default`. |53| **GO-03** | `go func()` whose exit condition depends on a channel nobody may write, a send on an unbuffered channel with no reader, `context.WithCancel`/`WithTimeout` whose `cancel` is not deferred, or a goroutine that ignores `ctx.Done()`. | Leaked goroutines and timers accumulate per request until the process dies. Leaks are invisible in tests and fatal in production. | `defer cancel()` at every `WithCancel`/`WithTimeout`. Every goroutine has a `select` on `ctx.Done()` or a guaranteed-closed channel. |54| **GO-04** | A `database/sql` query string built with `+`, `fmt.Sprintf`, or `strings.Join` around a value that came from a request, a file, or a database. | SQL injection. Identifiers cannot be parameterised, so this is often the real thing rather than a false positive. | Placeholders (`?`, `$1`) with args passed to `Query`/`Exec`. Where an identifier must vary, validate it against a fixed allow-list of known column or table names. |55| **GO-05** | `defer` inside a `for` body: typically `defer f.Close()` or `defer rows.Close()`. | Deferred calls run at function exit, not iteration exit, so descriptors and locks pile up for the whole loop. Over a large input set this exhausts the fd limit. | Move the body into a function (named or closure) called per iteration, or close explicitly at the end of the iteration and on every error path. |56| **GO-06** | `_ = f()` on a function returning `error`, a bare call whose error is discarded, unchecked `defer f.Close()` on a *writable* file, or an ignored `rows.Err()` after a range over `rows`. | Silent failure. A discarded write error means the caller reports success on data that never landed; a missing `rows.Err()` hides a truncated result set that looks like an empty one. | Every error is checked, wrapped with context (`fmt.Errorf("...: %w", err)`), or explicitly ignored with a comment saying why it is safe. |57| **GO-07** | A map, slice, or struct field written from more than one goroutine with no mutex or channel discipline; `sync.WaitGroup` reused across rounds; a captured loop variable in a pre-1.22 module; lazy init with no `sync.Once`. | Data races corrupt memory and crash non-deterministically. Concurrent map write is an unrecoverable fatal error: no recover, no graceful shutdown. | Guard shared state with `sync.Mutex`/`RWMutex`, hand ownership to one goroutine over a channel, or use `sync/atomic`. Confirm with `go test -race`. |5859Beyond the checklist, also weigh: `context` not threaded through call chains that do I/O, `time.After`60in a loop, unbounded `io.ReadAll` on a request body, and `http.Client` with no timeout.6162## Output6364Both artifacts, always:65661. **Human report** per the skeleton in `../../references/procedure.md`: findings grouped by67 severity, then a `## Checks skipped` table naming every absent or crashed tool with its reason and68 install hint.692. **Agent prompt block** per `../../references/agent-prompt.md`, appended after that table, carrying70 every severity. Omit the block entirely when there are no findings.7172Derive the block's validation line from the probe. With the full toolchain present:73`go build ./... && go vet ./... && staticcheck ./... && go test -race ./...`. Drop each absent tool74rather than substituting a different one; `go build ./...` alone is a legitimate floor.7576## Common mistakes7778- **Passing off a `gosec` hit as a finding without reading the code.** `gosec` flags patterns, not79 reachability. Read the call site, then assign confidence per `../../references/rubric.md`.80- **Calling a missing tool a clean result.** "`govulncheck` reported nothing" and "`govulncheck` is81 not installed" are different claims. The second belongs in `## Checks skipped`.82- **Skipping the manual walk because the tools were quiet.** Authorisation gaps, wrong trust83 boundaries, and misthreaded `context` do not appear in any linter's output.84- **Reporting `defer` in a loop as Critical.** It is a resource leak; severity depends on the85 iteration count and process lifetime. Score consequence, not category.
mrevjd