anyproto/.claude/skills/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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

// 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:

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

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

### [Severity] Issue Title

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

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

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

Suggested Fix:

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