Go Error Review¶

Purpose¶

Audit Go code for error handling correctness, nil safety, and failure-path integrity. Core question for every function call: "What happens when it fails?"

This skill merges error handling + API request/response correctness + database operation correctness because they share one review lens: "does this code handle failure correctly?"

This skill does NOT cover: security vulnerabilities, concurrency safety, performance, code style, test quality, or business logic — those belong to sibling vertical skills.

When To Use¶

• Code contains error return values
• Code uses panic / recover
• Code involves sql.Rows, transactions, connection pools
• Code involves HTTP request/response body handling
• Code operates on []*T pointer slices

When NOT To Use¶

• Security vulnerabilities → go-security-review
• Concurrency safety → go-concurrency-review
• Performance optimization → go-performance-review
• Code style → go-quality-review
• Business logic → go-logic-review

Mandatory Gates¶

1) Execution Integrity Gate¶

Never claim tests ran unless they actually did. If not run: state reason + exact command.

2) Go Version Gate¶

Read go.mod. Key features: - errors.Is / errors.As (Go 1.13+) - errors.Join (Go 1.20+) - fmt.Errorf with multiple %w (Go 1.20+)

3) Anti-Example Suppression Gate¶

MUST quote specific code evidence. Category match alone insufficient.

Embedded anti-examples: - "Missing error handling on json.Marshal" — when marshaling known-safe struct with only primitive fields (string, int, bool, no interface fields). json.Marshal on such structs always returns nil error. - "Missing error wrapping" — when caller already wraps; adding another layer creates redundant context like "create user: insert user: insert row: ...". Cite the caller's wrapping code. - "Speculative nil dereference" — when caller is internal and always passes non-nil. Cite the caller code proving it never passes nil. - "Should use errors.Is instead of ==" — direct == against sentinel from the same package is acceptable. Cross-package comparison must use errors.Is. - "defer f.Close() ignoring error" — acceptable for read-only file opens. Flag only for write operations where Close flushes buffered data.

4) Generated Code Exclusion Gate¶

Exclude: *.pb.go, *_gen.go, mock_*.go. Note excluded files in Execution Status.

Workflow¶

1. Define scope — files/diff under review. Apply Generated Code Exclusion Gate.
2. Gather evidence — read changed files, identify error-handling patterns: if err != nil, _ =, panic(, sql.Rows, tx., resp.Body, []*T.
3. Load references — always load go-error-and-quality.md (error sections); load go-api-http-checklist.md when net/http code present; load go-database-patterns.md when database code present.
4. Evaluate ALL 12 checklist items — for each function call, ask "what happens on failure?"
5. Apply suppression → format output.

Grep-Gated Execution Protocol¶

This skill uses mechanical grep pre-scanning to guarantee zero missed checklist items. The model's attention is reserved for semantic judgment on grep hits and semantic-only items.

Execution Order¶

1. Identify target files (from dispatch prompt, or write raw snippet to $TMPDIR/review_snippet.go)
2. Run grep for ALL 12 checklist items against target files
3. HIT → run semantic analysis to confirm or reject (true positive vs false positive)
4. MISS → auto-mark NOT FOUND, skip semantic analysis for that item
5. For compound patterns (item 12): run both grep patterns, apply AND logic
6. Report only FOUND items (grep-confirmed + semantic-confirmed)

Grep Audit Line¶

Include in Execution Status: Grep pre-scan: X/12 items hit, Z confirmed as findings

Compound Pattern Protocol¶

Some items require two grep patterns. Run both: - Item 12 (Log-and-return): TRIGGER when log\.\|slog\. HIT AND return.*err HIT in same file

Error Checklist (12 Items)¶

Error Handling (High)¶

API/HTTP Correctness (High)¶

Database Correctness (High)¶

Severity Rubric¶

High — Resource leak, crash, silent failure, data inconsistency.

Medium — Suboptimal error handling that makes debugging harder but no immediate failure.

Evidence Rules¶

• For each finding: explain what happens on the failure path
• For resource leaks: show the code path where Close/Rollback is missed
• For body leaks: show the error-path branch that skips Close
• Merge rule: same issue at ≥3 locations → one finding with location list

Output Format¶

Findings¶

[High|Medium] Short Title¶

• ID: ERR-NNN
• Location: path:line
• Impact: What happens when this code path fails
• Evidence: The missing error check / resource close / wrapping
• Recommendation: Specific fix
• Action: must-fix | follow-up

Suppressed Items¶

[Suppressed] Short Title¶

• Reason: Anti-example matched + evidence cited

Execution Status¶

• Go version: X.Y
• Grep pre-scan: X/12 items hit, Z confirmed as findings
• go test: PASS | FAIL | Not run (reason + command)
• Excluded (generated): list or None
• References loaded: list

Summary¶

1-2 lines with finding count.

Example Output¶

### Findings

#### [High] Response Body Leak in HTTP Client
- **ID:** ERR-001
- **Location:** `internal/client/api.go:45`
- **Impact:** Connection pool exhaustion — resp.Body not closed on error path
- **Evidence:** `resp, err := client.Do(req)` at L42; if `resp.StatusCode != 200` at L44, function returns error at L46 without closing resp.Body. Body only closed on happy path at L52.
- **Recommendation:** Move defer immediately after nil-error check:
  ```go
  resp, err := client.Do(req)
  if err != nil { return err }
  defer resp.Body.Close()
  ```
- **Action:** must-fix

#### [High] Missing rows.Err() After Iteration
- **ID:** ERR-002
- **Location:** `internal/repo/order.go:78`
- **Impact:** Silent data truncation — if iteration breaks due to network error, partial results returned without error
- **Evidence:** `for rows.Next() { ... }` loop at L73-80 exits without checking `rows.Err()`
- **Recommendation:** Add after loop: `if err := rows.Err(); err != nil { return nil, fmt.Errorf("iterating orders: %w", err) }`
- **Action:** must-fix

### Suppressed Items
#### [Suppressed] json.Marshal Error Ignored
- **Reason:** `json.Marshal(config)` at config.go:30 — `config` is `AppConfig` struct with only primitive fields. Anti-example: "known-safe struct with no interface fields"

### Execution Status
- Go version: 1.21
- Grep pre-scan: 5/12 items hit, 2 confirmed as findings
- go test: PASS
- Excluded (generated): None
- References loaded: go-error-and-quality.md, go-api-http-checklist.md, go-database-patterns.md

### Summary
2 High findings (response body leak, missing rows.Err). No Medium findings.

No-Finding Case¶

If no issues found: state No error handling findings identified. Still output Execution Status.

Load References Selectively¶

Review Discipline¶

• Error handling, nil safety, failure-path integrity only — not security, concurrency, performance, style, tests, or logic
• For every function call: "what happens when it fails?"
• Execute ALL 12 checklist items without skipping
• Server handler r.Body: do NOT require manual Close (framework handles it)
• Client resp.Body: MUST be closed on all paths