go-error-review

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

| Reference | Load When |
|-----------|-----------|
| `references/go-error-and-quality.md` | Always (error handling sections) |
| `references/go-api-http-checklist.md` | Code involves net/http, handlers, gin/echo/chi, gRPC |
| `references/go-database-patterns.md` | Code involves database/sql, pgx, sqlx, gorm, ent |
| `references/go-review-anti-examples.md` | Always |

## 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