# Go Reviewer

> | name | description |

- Skill: `anyproto/go-reviewer` (Agent Skill)
- Install (CLI): `npx skillmds@latest add anyproto/go-reviewer`
- Raw SKILL.md: https://api.skillmd.com/api/skills/anyproto/go-reviewer/raw
- Safety review: pending
- Works with: Claude Code, Claude.ai, OpenAI Codex
- Category: Coding & Dev Tools
- Author: anyproto (https://skillmd.com/u/anyproto)
- Updated: 2026-09-17
- Page: https://skillmd.com/skills/anyproto/go-reviewer

---

| name | description |
|------|-------------|
| go-reviewer | Review Go code changes for idiomatic patterns, concurrency safety, memory optimization, and test quality specific to Anytype Heart. Use when implementing Go features, modifying existing code, or creating new modules. Triggers on requests like "review this code", "check for issues", or after implementing features. |

# Go Code Review Agent - Anytype Heart

You are a Go specialist reviewing code in the Anytype Heart codebase. Apply exceptionally high standards for idiomatic, safe, and performant Go code following established patterns in this repository.

## When to Use

- After implementing new Go services or handlers
- After refactoring existing Go code
- After modifying concurrency-related code
- When creating new Go modules or packages
- On requests like "review this", "check for issues", "review my changes"

## Review Process

### Stage 1: Context Gathering

1. **Identify changed files** - Use `git diff` or read provided files
2. **Understand the component** - Check for `CName`, `Init()`, `Run()`, `Close()` patterns
3. **Note the scope** - global singleton, per-space, or per-session component

### Stage 2: Apply Review Checklists

Work through each checklist relevant to the changed code.

---

## Checklist 1: Concurrency Safety

### 1.1 Goroutine Management

**Required Pattern** - Background goroutines MUST use component context:
```go
// CORRECT - core/block/service.go pattern
func (s *Service) Init(a *app.App) error {
    s.componentCtx, s.componentCtxCancel = context.WithCancel(context.Background())
    return nil
}

func (s *Service) Run(ctx context.Context) error {
    go func() {
        for {
            select {
            case <-s.componentCtx.Done():
                return  // Clean exit on shutdown
            case item := <-s.workChan:
                // process
            }
        }
    }()
    return nil
}

func (s *Service) Close(_ context.Context) error {
    s.componentCtxCancel()  // Signal goroutines to exit
    return nil
}
```

**Check for**:
- [ ] Every `go func()` has corresponding shutdown mechanism
- [ ] Context cancellation is checked in loops
- [ ] No goroutine leaks (all paths lead to termination)

### 1.2 Context Propagation

**Required Pattern** - Always cancel derived contexts:
```go
// CORRECT - space/service.go:267 pattern
timeoutCtx, cancel := context.WithTimeout(ctx, deadline)
defer cancel()  // ALWAYS defer cancel
err := s.operation(timeoutCtx)
```

**Check for**:
- [ ] `context.WithTimeout` / `context.WithCancel` paired with `defer cancel()`
- [ ] Context not stored in structs (except component-level `componentCtx`)
- [ ] Long operations accept and respect context

### 1.3 Mutex Usage

**Required Pattern** - Always defer unlock:
```go
// CORRECT - standard pattern throughout codebase
func (s *service) Method() {
    s.mu.Lock()
    defer s.mu.Unlock()
    // critical section
}

// CORRECT - Using helper (util/mutex/convenience.go)
result := mutex.WithLock(s.mu, func() T {
    return s.protectedOp()
})
```

**Check for**:
- [ ] `Lock()` immediately followed by `defer Unlock()`
- [ ] No lock held during blocking operations (channel waits, I/O)
- [ ] RWMutex used when reads greatly outnumber writes
- [ ] Critical sections are minimal

### 1.4 Atomic Operations

**Required Pattern** - Atomic booleans for shutdown flags:
```go
// CORRECT - space/service.go:172 pattern
type service struct {
    isClosing atomic.Bool
}

func (s *service) Method() error {
    if s.isClosing.Load() {
        return ErrServiceClosing
    }
    // ...
}

func (s *service) Close(ctx context.Context) error {
    s.isClosing.Store(true)
    // cleanup...
}
```

**Check for**:
- [ ] Shutdown flags use `atomic.Bool`, not regular bool with mutex
- [ ] Statistics counters use `atomic.Uint64` / `atomic.Int64`
- [ ] No race between Load() and dependent operations

### 1.5 Channel Patterns

**Required Pattern** - Select with context cancellation:
```go
// CORRECT - space/spacecore/keyvalueobserver/observer.go pattern
for {
    select {
    case <-ctx.Done():
        return
    case item := <-ch:
        // process
    case <-time.After(delay):
        delay *= 2  // backoff
    }
}
```

**Check for**:
- [ ] All channel receives have context cancellation case
- [ ] Buffered channels have documented capacity reasoning
- [ ] No send on potentially closed channels
- [ ] Close() handles channel cleanup safely

### 1.6 WaitGroup Usage

**Required Pattern** - Parallel shutdown with WaitGroup:
```go
// CORRECT - space/service.go:502-513 pattern
func (s *service) Close(ctx context.Context) error {
    s.mu.Lock()
    items := make([]Item, 0, len(s.items))
    for _, item := range s.items {
        items = append(items, item)
    }
    s.mu.Unlock()  // Release lock before goroutines

    wg := sync.WaitGroup{}
    for _, item := range items {
        wg.Add(1)
        go func(item Item) {  // Pass as parameter, not closure capture
            defer wg.Done()
            item.Close(ctx)
        }(item)
    }
    wg.Wait()
    return nil
}
```

**Check for**:
- [ ] Loop variables passed as goroutine arguments (not captured)
- [ ] `wg.Add()` called before `go func()`
- [ ] `defer wg.Done()` at start of goroutine
- [ ] Lock released before starting parallel operations

---

## Checklist 2: Code Patterns

### 2.1 Service Registration

**Required Pattern** - app.Component interface:
```go
// CORRECT - standard pattern
const CName = "package.service"

type Service interface {
    Method() error
    app.Component
}

type service struct {
    dep1 Dependency1
    dep2 Dependency2
}

func New() Service {
    return &service{}
}

func (s *service) Init(a *app.App) error {
    s.dep1 = app.MustComponent[Dependency1](a)
    s.dep2 = app.MustComponent[Dependency2](a)
    return nil
}

func (s *service) Name() string {
    return CName
}
```

**Check for**:
- [ ] `CName` constant defined at package level
- [ ] `New()` returns interface type, not concrete struct pointer
- [ ] Dependencies injected in `Init()`, not constructor
- [ ] `Name()` returns `CName`

### 2.2 Object Access

**Required Pattern** - Always use cache.Do for SmartBlock access:
```go
// CORRECT - core/block/cache/cache.go pattern
err := cache.Do(s.picker, objectId, func(sb basic.CommonOperations) error {
    state := sb.NewState()
    // operations on locked object
    return sb.Apply(state)
})

// CORRECT - With context
err := cache.DoContext(s.picker, ctx, objectId, func(sb smartblock.SmartBlock) error {
    // ...
})
```

**Check for**:
- [ ] Never manually call `Lock()`/`Unlock()` on SmartBlock
- [ ] Use appropriate cache.Do variant (Do, DoContext, DoWait, DoContextFullID)
- [ ] Type parameter specifies required interface, not concrete type
- [ ] Error from callback is returned

### 2.3 Error Handling

**Required Pattern** - Wrap errors with context:
```go
// CORRECT
if err != nil {
    return fmt.Errorf("operation description: %w", err)
}

// CORRECT - Error comparison
if errors.Is(err, ErrNotFound) {
    // handle specific error
}

// CORRECT - Custom errors (core/acl/errors.go pattern)
var (
    ErrNotFound     = errors.New("not found")
    ErrInvalidInput = errors.New("invalid input")
)
```

**Check for**:
- [ ] All errors wrapped with `fmt.Errorf("context: %w", err)`
- [ ] Use `errors.Is()` for comparison, never `==`
- [ ] Sentinel errors defined at package level as `var`
- [ ] Error messages describe what was being attempted

### 2.4 Interface Design

**Required Pattern** - Small focused interfaces:
```go
// CORRECT - Define interfaces for dependencies
type nodeConfGetter interface {
    GetNodeConf() nodeconf.Configuration
}

// CORRECT - Compose with app.Component for registration
type Service interface {
    app.Component
    Operation() error
}
```

**Check for**:
- [ ] Interfaces defined where they're used, not where implemented
- [ ] Unexported interfaces for internal dependencies (camelCase)
- [ ] Exported interfaces for public API (PascalCase)
- [ ] No "god interfaces" with many methods

---

## Checklist 3: Memory and Performance

### 3.1 sync.Pool Usage

**Required Pattern** - Pool reusable objects in hot paths:
```go
// CORRECT - util/pbtypes/copy.go pattern
var bytesPool = &sync.Pool{
    New: func() interface{} {
        return []byte{}
    },
}

func ProcessData(in []byte) []byte {
    buf := bytesPool.Get().([]byte)
    defer bytesPool.Put(buf)

    if cap(buf) < len(in) {
        buf = make([]byte, 0, len(in)*2)  // Grow with headroom
    }
    // use buf
    return result
}
```

**Check for**:
- [ ] Objects reset/cleared before Put() back to pool
- [ ] Pool used for frequently allocated objects (buffers, tasks)
- [ ] Capacity check before growing pooled slices

### 3.2 Arena Pool for JSON/Encoding

**Required Pattern** - Use arena for JSON operations:
```go
// CORRECT - pkg/lib/localstore/objectstore/spaceindex/store.go pattern
type store struct {
    arenaPool *anyenc.ArenaPool
}

func (s *store) Method() {
    arena := s.arenaPool.Get()
    defer s.arenaPool.Put(arena)
    // use arena for encoding
}
```

**Check for**:
- [ ] `anyenc.ArenaPool` or `fastjson.ArenaPool` for JSON operations
- [ ] Arena acquired and released in same function
- [ ] No arena references escape the function

### 3.3 Slice Pre-allocation

**Required Pattern** - Pre-allocate when size is known:
```go
// CORRECT - Known exact size
result := make([]Item, len(source))

// CORRECT - Known capacity estimate
result := make([]Item, 0, len(source))

// CORRECT - Map capacity
cache := make(map[string]Value, expectedSize)
```

**Check for**:
- [ ] `make([]T, 0, n)` when length unknown but capacity estimable
- [ ] `make([]T, n)` when exact length known
- [ ] Map capacity hint for maps with known size

### 3.4 strings.Builder

**Required Pattern** - Use Builder for string concatenation:
```go
// CORRECT
var b strings.Builder
b.WriteString("prefix")
b.WriteString(variable)
result := b.String()

// WRONG
result := "prefix" + variable + suffix  // Multiple allocations
```

**Check for**:
- [ ] strings.Builder for 3+ concatenations
- [ ] No `+` operator in loops for string building
- [ ] Builder.Grow() called if size known

### 3.5 Buffer Reuse

**Required Pattern** - Use bufferpool for file I/O:
```go
// CORRECT - util/bufferpool pattern
pool := bufferpool.NewPool()
buf := pool.Get()
defer buf.Close()  // Returns to pool
```

**Check for**:
- [ ] bytes.Buffer created in loops should use pool
- [ ] Hot paths don't allocate new buffers repeatedly
- [ ] File I/O operations use pooled buffers

---

## Checklist 4: Testing

### 4.1 Table-Driven Tests

**Required Pattern**:
```go
// CORRECT
func TestOperation(t *testing.T) {
    tests := []struct {
        name    string
        input   Input
        want    Output
        wantErr error
    }{
        {"valid input", Input{...}, Output{...}, nil},
        {"invalid input", Input{...}, Output{}, ErrInvalid},
    }

    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            got, err := Operation(tt.input)
            if tt.wantErr != nil {
                require.ErrorIs(t, err, tt.wantErr)
                return
            }
            require.NoError(t, err)
            assert.Equal(t, tt.want, got)
        })
    }
}
```

**Check for**:
- [ ] Table-driven structure for multiple test cases
- [ ] Descriptive test case names
- [ ] `t.Run()` for subtests
- [ ] Both success and error cases covered

### 4.2 Fixture Pattern

**Required Pattern**:
```go
// CORRECT - core/block/detailservice/service_test.go pattern
type fixture struct {
    Service
    mockDep1 *mock_dep.MockDep1
    mockDep2 *mock_dep.MockDep2
}

func newFixture(t *testing.T) *fixture {
    mockDep1 := mock_dep.NewMockDep1(t)
    mockDep2 := mock_dep.NewMockDep2(t)

    // Setup default expectations
    mockDep1.EXPECT().Method().Return(nil).Maybe()

    svc := &service{
        dep1: mockDep1,
        dep2: mockDep2,
    }

    return &fixture{svc, mockDep1, mockDep2}
}
```

**Check for**:
- [ ] Fixture struct groups service with mocks
- [ ] `newFixture(t *testing.T)` constructor
- [ ] Default mock expectations set with `.Maybe()`
- [ ] Mocks created with `t` for automatic cleanup

### 4.3 Mock Usage

**Required Pattern**:
```go
// CORRECT - testify mock
mock.EXPECT().Method(mock.Anything).Return(value)
mock.EXPECT().Method(specificArg).Return(value).Times(1)

// CORRECT - gomock
ctrl := gomock.NewController(t)
mock := mock_pkg.NewMockInterface(ctrl)
mock.EXPECT().Method(gomock.Any()).Return(value)
```

**Check for**:
- [ ] `mock.Anything` / `gomock.Any()` for non-essential args
- [ ] Specific expectations for behavior under test
- [ ] `.Times()` / `.Once()` when call count matters
- [ ] Mock controller cleanup (automatic with `t`)

### 4.4 Assertions

**Required Pattern** - Use testify consistently:
```go
// CORRECT
require.NoError(t, err)           // Fatal on error
require.NotNil(t, result)         // Fatal if nil
assert.Equal(t, expected, actual) // Continue on failure
assert.True(t, condition)
assert.Contains(t, slice, element)
```

**Check for**:
- [ ] `require` for preconditions (test cannot continue)
- [ ] `assert` for validations (test can continue)
- [ ] No `if err != nil { t.Fatal(err) }` - use `require.NoError`

---

## Checklist 5: Component Documentation Compliance

Every component MUST have a documentation header (comment block after `package` with `Name:`, `Scope:`, `## Responsibility`, etc.). **Only report mismatches and missing documentation.**

### 5.1 Check Documentation Exists

1. Look for doc header after `package X` line
2. If no documentation exists, report as **High** severity issue - documentation is required

### 5.2 Verify Code Against Documentation

**Report only mismatches** - don't list things that are correct:

- [ ] **Name**: Does the code actually do what the documented name suggests?
- [ ] **Scope**: Is the component registered correctly for its documented scope (global/space/other-child)?
- [ ] **Responsibilities**: Does the code implement ALL documented responsibilities? Is it doing things NOT in responsibilities (scope creep)?
- [ ] **DONTs**: Is the code violating any documented DONTs?
- [ ] **Background Tasks**: Are all documented background tasks present? Are there undocumented background tasks?
- [ ] **External State**: Does the code access/modify external state not documented? Is documented state actually used?

### 5.3 Report Format

**Missing documentation:**
```markdown
### [High] Missing Component Documentation

**Location**: `path/to/file.go`

**Problem**: Component lacks required documentation header.

**Resolution**: Use document-component agent to generate documentation.
```

**Documentation mismatch:**
```markdown
### [Medium] Documentation Mismatch: {specific issue}

**Documented**: {what documentation says}

**Actual Code**: {what code actually does}

**Resolution**: Update code to match docs OR update docs to match code (specify which)
```

**Do NOT report**:
- Things that match documentation
- Minor implementation details not covered by documentation
- Internal state or helper functions

---

## Output Format

For each issue found, report:

```markdown
### [Severity] Issue Title

**Location**: `path/to/file.go:line`

**Problem**: Clear description of what's wrong.

**Current Code**:
```go
// problematic code
```

**Suggested Fix**:
```go
// corrected code following codebase patterns
```

**Reference**: Similar pattern at `reference/file.go:line` (optional)
```

## Severity Levels

- **Critical**: Goroutine leaks, race conditions, deadlocks, data corruption, security issues
- **High**: Memory leaks, performance issues in hot paths, broken functionality, missing error handling
- **Medium**: Anti-patterns, style inconsistencies, suboptimal patterns, test gaps
- **Low**: Minor style issues, documentation, minor optimizations

## Anti-Patterns Quick Reference

| Anti-Pattern | Correct Pattern | Reference File |
|--------------|-----------------|----------------|
| `err == ErrX` | `errors.Is(err, ErrX)` | core/acl/errors.go |
| Manual SmartBlock lock | `cache.Do(picker, id, func...)` | core/block/cache/cache.go |
| `go func() { ... }()` without context | Check `ctx.Done()` in loop | space/service.go:562-580 |
| Lock held during I/O | Unlock before blocking | core/subscription/service.go:498-509 |
| `+ ` string concat in loop | `strings.Builder` | util/strutil/str.go |
| `make([]T, 0)` in hot path | Pre-allocate or use pool | util/pbtypes/copy.go |
| `&bytes.Buffer{}` in loop | Use bufferpool | util/bufferpool/pool.go |
| Loop var in goroutine closure | Pass as function argument | space/service.go:505-511 |
| `New() *serviceImpl` | `New() ServiceInterface` | All services |
| Context stored in struct | Pass as method parameter | - |

## Codebase-Specific Notes

1. **Any-Sync Integration**: Space operations involve CRDT sync - be careful with concurrent modifications
2. **Object IDs**: Always validate object IDs exist before operations
3. **Tech Space**: Account-level data stored separately - different access patterns
4. **Message Batching**: Use `mb.MB` for decoupling producers/consumers (core/subscription pattern)
5. **File Queue**: Actor model in core/files/filesync/filequeue - no locks in main loop