---

name: golang-reviewer description: Review Go codebases for quality, security, and idiomatic patterns. Use when asked to review Go code, run quality gates, audit Go security, fix linting errors, improve test coverage, or iterate on code quality. Combines static analysis, security review, idiomatic patterns, and testing best practices. Triggers on "go review", "golang lint", "go security", "go coverage", "quality loop", "go best practices".

Go Code Reviewer

Systematic Go code review combining automated tooling with manual inspection.

Quick Reference

| Task | Command | |------|---------| | Full lint | golangci-lint run ./... | | Basic checks | go vet ./... && staticcheck ./... | | Tests + race | go test -race -coverprofile=coverage.out ./... | | Coverage | go tool cover -func=coverage.out | | Security scan | semgrep --config p/golang . | | Tidy deps | go mod tidy && go mod verify |

---

When to Use / When NOT to Use

✅ Use this skill when:

• Reviewing Go code for quality issues
• Running quality gates before merge/release
• Security auditing Go applications
• Improving test coverage
• Running review → fix → test loops
• Preparing Go code for production

❌ Do NOT use when:

• Non-Go codebases (use language-specific reviewer)
• Quick syntax check only (just run go vet)
• Performance profiling (use pprof directly)
• Debugging runtime issues (use dlv debugger)

---

Decision Tree

Review Request
    │
    ├─ Full review? → Run Steps 1-5, iterate until clean
    │
    ├─ Lint only? → Step 1: Static Analysis
    │
    ├─ Security only? → Step 4: Security Checklist
    │
    ├─ Coverage only? → Step 2: Tests & Coverage
    │
    └─ Quick fix? → Run lint, fix errors, commit

---

Step 1: Static Analysis

# Primary (runs 50+ linters)
golangci-lint run ./... 2>&1 | head -100

# Fallback if golangci-lint unavailable
go vet ./...
staticcheck ./...

| Linter | Catches | Severity | |--------|---------|----------| | errcheck | Unchecked errors | HIGH | | gosec | Security issues | CRITICAL | | govet | Suspicious constructs | HIGH | | ineffassign | Ineffectual assignments | MEDIUM | | staticcheck | Bugs, simplifications | HIGH | | unused | Dead code | LOW |

Rule: Fix all errors before proceeding. Warnings can be triaged.

---

Step 2: Tests & Coverage

# Run with race detection
go test -race -coverprofile=coverage.out ./...

# Coverage summary
go tool cover -func=coverage.out | tail -20

# HTML report (optional)
go tool cover -html=coverage.out -o coverage.html

Coverage Targets:

| Package Type | Target | Notes | |--------------|--------|-------| | Core business logic | ≥60% | Critical paths, error handling | | Utilities/helpers | ≥40% | Basic happy path | | CLI entry points | 0% OK | main() rarely testable | | Generated code | Skip | Exclude from coverage |

---

Step 3: Idiomatic Go Patterns

Naming

| Element | Convention | Example | |---------|------------|---------| | Packages | short, lowercase, no underscores | config, auth | | Interfaces | -er suffix for single method | Reader, Handler | | Getters | No Get prefix | user.Name() not user.GetName() | | Acronyms | All caps | HTTPClient, userID | | Exported | PascalCase with doc comment | // Client manages... |

// ❌ BAD
type IUserService interface{}  // No I prefix
func (u *User) GetName() string {}  // No Get prefix
type HttpClient struct{}  // Inconsistent caps

// ✅ GOOD
type UserService interface{}
func (u *User) Name() string {}
type HTTPClient struct{}

Error Handling

// ❌ BAD: Swallowed error
if err != nil {
    // empty
}

// ❌ BAD: No context
return err

// ❌ BAD: Sentinel comparison with ==
if err == sql.ErrNoRows {}

// ✅ GOOD: Wrapped with context
if err != nil {
    return fmt.Errorf("connect to database: %w", err)
}

// ✅ GOOD: errors.Is for sentinel
if errors.Is(err, sql.ErrNoRows) {}

Struct Initialization

// ❌ BAD: Positional fields
user := User{"john", 30, true}

// ✅ GOOD: Named fields
user := User{
    Name:   "john",
    Age:    30,
    Active: true,
}

Defer

// ❌ BAD: Defer in loop
for _, f := range files {
    file, _ := os.Open(f)
    defer file.Close()  // Defers stack up!
}

// ✅ GOOD: Explicit close or wrap in function
for _, f := range files {
    if err := processFile(f); err != nil {
        return err
    }
}

---

Step 3b: Clean Code Principles

Based on Bob Martin's Clean Code — adapted for Go.

Functions

| Rule | Guideline | |------|-----------| | Small | 5-20 lines ideal, max 40 | | Do One Thing | If you can extract another function, do it | | One Abstraction Level | Don't mix high-level logic with low-level details | | Few Arguments | 0-2 ideal, 3 max; use structs for more | | No Side Effects | Don't mutate inputs unexpectedly | | Command-Query Separation | Functions either DO something or RETURN something, not both |

// ❌ BAD: Does too much, multiple abstraction levels
func ProcessOrder(order Order) error {
    // Validate
    if order.ID == "" { return errors.New("missing id") }
    if order.Total < 0 { return errors.New("invalid total") }
    
    // Save to DB
    db, _ := sql.Open("postgres", connStr)
    _, err := db.Exec("INSERT INTO orders...")
    
    // Send email
    smtp.SendMail(host, auth, from, to, msg)
    return nil
}

// ✅ GOOD: Single responsibility, one abstraction level
func ProcessOrder(order Order) error {
    if err := validateOrder(order); err != nil {
        return fmt.Errorf("invalid order: %w", err)
    }
    if err := saveOrder(order); err != nil {
        return fmt.Errorf("save failed: %w", err)
    }
    return notifyCustomer(order)
}

SOLID Principles

| Principle | Go Application | |-----------|----------------| | Single Responsibility | One struct/package = one reason to change | | Open-Closed | Extend via interfaces, not modification | | Liskov Substitution | Interface implementations must be interchangeable | | Interface Segregation | Small, focused interfaces (1-3 methods) | | Dependency Inversion | Depend on interfaces, not concretions |

// ❌ BAD: Concrete dependency
type OrderService struct {
    db *sql.DB  // Concrete type
}

// ✅ GOOD: Interface dependency (Dependency Inversion)
type OrderRepository interface {
    Save(Order) error
    Find(id string) (Order, error)
}

type OrderService struct {
    repo OrderRepository  // Interface
}

The Boy Scout Rule

"Leave the code cleaner than you found it."

When touching a file:

• [ ] Rename unclear variables
• [ ] Extract magic numbers to constants
• [ ] Remove dead code
• [ ] Add missing error context
• [ ] Fix obvious lint warnings

Naming

| ❌ Bad | ✅ Good | Why | |--------|---------|-----| | d | daysSinceCreation | Reveal intent | | ProcessData | ValidateUserInput | Be specific | | doStuff | calculateTax | Describe the action | | tempList | activeUsers | Name the content | | flag | isAdmin | Booleans as questions |

Comments

| Use Comments For | Avoid Comments For | |------------------|-------------------| | Legal/license headers | Explaining what code does | | TODO with ticket reference | Commented-out code | | Why (non-obvious decisions) | Redundant narration | | Public API documentation | Markers like // end if |

// ❌ BAD: Narrating the obvious
// Increment counter by 1
counter++

// ✅ GOOD: Explaining why
// Use exponential backoff to avoid thundering herd on recovery
delay := baseDelay * time.Duration(1<<attempt)

Code Smells Checklist

• [ ] Long functions — Extract smaller functions
• [ ] Deep nesting — Use guard clauses, early returns
• [ ] Primitive obsession — Create domain types
• [ ] Feature envy — Move logic to the struct that owns the data
• [ ] God struct — Split into focused types
• [ ] Duplicate code — DRY, but don't over-abstract

// ❌ BAD: Deep nesting
func process(user User) error {
    if user.Active {
        if user.Verified {
            if user.Balance > 0 {
                // actual logic here
            }
        }
    }
    return nil
}

// ✅ GOOD: Guard clauses (early returns)
func process(user User) error {
    if !user.Active {
        return ErrInactiveUser
    }
    if !user.Verified {
        return ErrUnverifiedUser
    }
    if user.Balance <= 0 {
        return ErrInsufficientBalance
    }
    // actual logic here
    return nil
}

Package Structure

project/
├── cmd/                    # Entry points (main packages)
│   └── server/main.go
├── internal/               # Private packages
│   ├── domain/             # Business entities
│   ├── service/            # Business logic
│   ├── repository/         # Data access
│   └── handler/            # HTTP/gRPC handlers
├── pkg/                    # Public packages (if any)
└── go.mod

| Layer | Depends On | Never Depends On | |-------|------------|------------------| | Handler | Service | Repository directly | | Service | Repository (interface) | Handler | | Repository | Domain | Service | | Domain | Nothing | Anything |

---

Step 4: Security Checklist

Secrets & Credentials

• [ ] No hardcoded API keys, passwords, tokens
• [ ] Secrets from env or secret manager
• [ ] Config files have 0600 permissions

// ❌ NEVER
const apiKey = "sk-live-xxxxx"

// ✅ ALWAYS
apiKey := os.Getenv("API_KEY")
if apiKey == "" {
    return errors.New("API_KEY required")
}

Input Validation

• [ ] User input validated before use
• [ ] File paths sanitized (no traversal)
• [ ] SQL uses parameterized queries

// ❌ NEVER: Path traversal
path := filepath.Join(base, userInput)

// ✅ ALWAYS: Sanitize
clean := filepath.Clean(userInput)
if strings.HasPrefix(clean, "..") {
    return errors.New("invalid path")
}
path := filepath.Join(base, filepath.Base(clean))

// ❌ NEVER: SQL injection
query := fmt.Sprintf("SELECT * FROM users WHERE id = '%s'", id)

// ✅ ALWAYS: Parameterized
row := db.QueryRow("SELECT * FROM users WHERE id = $1", id)

HTTP Security

• [ ] Clients have timeouts
• [ ] TLS not disabled in production
• [ ] Headers sanitized in logs

// ❌ NEVER: No timeout
client := &http.Client{}

// ✅ ALWAYS: Explicit timeout
client := &http.Client{
    Timeout: 30 * time.Second,
}

// ❌ NEVER: Disabled TLS
tr := &http.Transport{
    TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}

// ✅ ALWAYS: Proper TLS (prod)
tr := &http.Transport{
    TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS12},
}

Concurrency

• [ ] Shared state protected (mutex or channels)
• [ ] No goroutine leaks (context cancellation)
• [ ] WaitGroup: Add before Go, Done deferred

// ❌ BAD: Goroutine leak
go func() {
    for item := range ch {
        process(item)
    }
}()

// ✅ GOOD: Context cancellation
go func() {
    for {
        select {
        case item := <-ch:
            process(item)
        case <-ctx.Done():
            return
        }
    }
}()

// ✅ GOOD: WaitGroup pattern
var wg sync.WaitGroup
wg.Add(1)
go func() {
    defer wg.Done()
    work()
}()
wg.Wait()

Semgrep Scan (Optional)

# Go-specific security rules
semgrep --config p/golang .

# OWASP rules
semgrep --config p/owasp-top-ten .

---

Step 5: Dependencies

# Check for outdated deps
go list -m -u all 2>&1 | grep '\['

# Tidy and verify
go mod tidy
go mod verify

# Check for vulnerabilities
govulncheck ./...

---

Findings Classification

| Severity | Description | Action | |----------|-------------|--------| | CRITICAL | Security vuln, data race, crash | Fix immediately | | HIGH | Lint error, test fail, unchecked error | Fix before merge | | MEDIUM | Coverage gap, missing docs | Fix or document why not | | LOW | Style preference, micro-optimization | Optional | | FALSE POSITIVE | Tool wrong or N/A | Document and skip |

---

Review Loop

1. Run full review (Steps 1-5)
2. Fix CRITICAL and HIGH findings
3. Commit: "fix: description (file:line)"
4. Re-run static analysis + tests
5. If new issues → repeat from 2
6. Document remaining MEDIUM/LOW
7. Exit when: tests pass, lint clean, only minor findings

Report: REVIEW_COMPLETE with summary

---

Output Format

## Iteration N

### Fixed
- [HIGH] Fixed unchecked error in db.go:45
- [HIGH] Added timeout to HTTP client

### Remaining  
- [MEDIUM] browser pkg coverage 31% (needs playwright)
- [LOW] Consider renaming httpClient → client

### Status
| Check | Result |
|-------|--------|
| golangci-lint | ✅ 0 errors |
| go test -race | ✅ PASS |
| Coverage | 58.7% |

CONTINUE / REVIEW_COMPLETE

---

Common Pitfalls

| ❌ Don't | ✅ Do | |----------|-------| | Ignore errcheck findings | Fix or explicitly ignore with _ = | | Disable linters wholesale | Add targeted //nolint:linter with reason | | Chase 100% coverage | Focus on critical paths + error handling | | Skip -race in tests | Always run race detector | | Return bare errors | Wrap with fmt.Errorf("context: %w", err) | | Use interface{} freely | Use generics or specific types |

---

Minimal golangci-lint Config

# .golangci.yml
version: "2"
linters:
  enable:
    - errcheck
    - gosec
    - govet
    - ineffassign
    - staticcheck
    - unused
run:
  timeout: 5m
issues:
  exclude-use-default: false
output:
  formats:
    - format: line-number

---

References

Go-Specific:

• Effective Go
• Go Code Review Comments
• Uber Go Style Guide

Clean Code & Architecture:

• Clean Code — Robert C. Martin
• Clean Architecture — Robert C. Martin
• SOLID Principles

Tools:

• golangci-lint
• gosec
• govulncheck