Go Reviewer

Go Reviewer Skill

Purpose

Reviews Go code for idiomatic patterns, error handling, concurrency, and best practices.

When to Use

• Go project code review
• Error handling review
• Goroutine/channel review
• Interface design review
• Go testing patterns

Project Detection

• go.mod in project root
• .go files
• cmd/, internal/, pkg/ structure
• *_test.go test files

Workflow

Step 1: Analyze Project

**Go Version**: 1.21+
**Module**: github.com/org/project
**Structure**: Standard Go layout
**Testing**: go test / testify
**Linter**: golangci-lint

Step 2: Select Review Areas

AskUserQuestion:

"Which areas to review?"
Options:
- Full Go review (recommended)
- Error handling patterns
- Concurrency and goroutines
- Interface design
- Testing and benchmarks
multiSelect: true

Detection Rules

Error Handling

// BAD: Ignored error
data, _ := ioutil.ReadFile("config.json")

// GOOD: Handle error
data, err := os.ReadFile("config.json")
if err != nil {
    return fmt.Errorf("reading config: %w", err)
}

// BAD: No context
if err != nil {
    return err
}

// GOOD: Add context
if err != nil {
    return fmt.Errorf("failed to process user %d: %w", userID, err)
}

// BAD: Panic for recoverable error
func GetUser(id int) *User {
    user, err := db.FindUser(id)
    if err != nil {
        panic(err)  // Don't panic!
    }
    return user
}

// GOOD: Return error
func GetUser(id int) (*User, error) {
    user, err := db.FindUser(id)
    if err != nil {
        return nil, fmt.Errorf("getting user %d: %w", id, err)
    }
    return user, nil
}

Concurrency

// BAD: Data race
type Counter struct {
    count int
}

func (c *Counter) Increment() {
    c.count++  // Race condition!
}

// GOOD: Mutex protection
type Counter struct {
    mu    sync.Mutex
    count int
}

func (c *Counter) Increment() {
    c.mu.Lock()
    defer c.mu.Unlock()
    c.count++
}

// BAD: Goroutine leak
func process(items []Item) {
    for _, item := range items {
        go processItem(item)  // No way to wait or cancel!
    }
}

// GOOD: WaitGroup and context
func process(ctx context.Context, items []Item) error {
    g, ctx := errgroup.WithContext(ctx)

    for _, item := range items {
        item := item  // Capture loop variable
        g.Go(func() error {
            return processItem(ctx, item)
        })
    }

    return g.Wait()
}

// BAD: No timeout
resp, err := client.Do(req)

// GOOD: With context timeout
ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
defer cancel()

req = req.WithContext(ctx)
resp, err := client.Do(req)

Interface Design

// BAD: Large interface
type UserService interface {
    GetUser(id int) (*User, error)
    CreateUser(u *User) error
    UpdateUser(u *User) error
    DeleteUser(id int) error
    ListUsers() ([]*User, error)
    SearchUsers(query string) ([]*User, error)
    // ... 20 more methods
}

// GOOD: Small, focused interfaces
type UserGetter interface {
    GetUser(ctx context.Context, id int) (*User, error)
}

type UserCreator interface {
    CreateUser(ctx context.Context, u *User) error
}

// Consumer defines the interface it needs
type UserHandler struct {
    getter UserGetter  // Only what it needs
}

// BAD: Concrete type dependency
func ProcessFile(f *os.File) error {
    // Hard to test
}

// GOOD: Interface dependency
func ProcessFile(r io.Reader) error {
    // Easy to test with strings.Reader, bytes.Buffer, etc.
}

Code Organization

// GOOD: Standard Go project layout
project/
├── cmd/
│   └── server/
│       └── main.go
├── internal/           # Private packages
│   ├── service/
│   │   └── user.go
│   └── repository/
│       └── user.go
├── pkg/               # Public packages
│   └── client/
│       └── client.go
├── go.mod
└── go.sum

Testing

// GOOD: Table-driven test
func TestParseSize(t *testing.T) {
    tests := []struct {
        name    string
        input   string
        want    int64
        wantErr bool
    }{
        {"bytes", "100", 100, false},
        {"kilobytes", "1KB", 1024, false},
        {"megabytes", "1MB", 1048576, false},
        {"invalid", "abc", 0, true},
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got, err := ParseSize(tt.input)
            if (err != nil) != tt.wantErr {
                t.Errorf("ParseSize() error = %v, wantErr %v", err, tt.wantErr)
                return
            }
            if got != tt.want {
                t.Errorf("ParseSize() = %v, want %v", got, tt.want)
            }
        })
    }
}

// GOOD: Benchmark
func BenchmarkParseSize(b *testing.B) {
    for i := 0; i < b.N; i++ {
        ParseSize("1MB")
    }
}

Response Template

## Go Code Review Results

**Project**: [name]
**Go**: 1.22 | **Linter**: golangci-lint

### Error Handling
| Status | File | Issue |
|--------|------|-------|
| CRITICAL | service.go:45 | Ignored error from db.Query |

### Concurrency
| Status | File | Issue |
|--------|------|-------|
| HIGH | worker.go:23 | Potential goroutine leak |

### Interface Design
| Status | File | Issue |
|--------|------|-------|
| MEDIUM | handler.go:12 | Concrete type in function signature |

### Testing
| Status | File | Issue |
|--------|------|-------|
| MEDIUM | service_test.go | No table-driven tests |

### Recommended Actions
1. [ ] Handle all returned errors
2. [ ] Add context cancellation to goroutines
3. [ ] Define interfaces at consumer side
4. [ ] Convert tests to table-driven format

Best Practices

1. Errors: Always handle, wrap with context
2. Concurrency: Use context, errgroup, proper sync
3. Interfaces: Small, defined at consumer
4. Testing: Table-driven, interfaces for mocking
5. Linting: Use golangci-lint

Integration

• go-api-reviewer: API framework patterns
• security-scanner: Go security audit
• perf-analyzer: Go performance profiling