Skip to content

feat: add state package implementing Durable Streams State Protocol#50

Merged
jilio merged 4 commits into
mainfrom
jilio/adopt-state-protocol
Dec 30, 2025
Merged

feat: add state package implementing Durable Streams State Protocol#50
jilio merged 4 commits into
mainfrom
jilio/adopt-state-protocol

Conversation

@jilio
Copy link
Copy Markdown
Owner

@jilio jilio commented Dec 30, 2025

Summary

This PR adds an optional state/ package that implements the Durable Streams State Protocol for ebu, enabling database-style state synchronization on top of ebu's event-driven architecture.

What's New

  • Type-safe helpers using Go generics: Insert, Update, Delete, UpdateWithOldValue, DeleteWithOldValue
  • Options pattern for configurable messages: WithTxID, WithTimestamp, WithAutoTimestamp, WithEntityType
  • Materializer that processes state protocol messages and builds typed state from events
  • Store interface with in-memory implementation for materializing state
  • TypedCollection for type-safe access to entity collections
  • Full JSON interoperability with durable-streams ecosystem
  • 100% test coverage with comprehensive unit, integration, and conformance tests

Design

The package is completely optional and doesn't modify core ebu. It integrates seamlessly with ebu's Replay functionality and event bus. Messages implement ebu's TypeNamer interface and can be published directly.

Test Coverage

All 44 tests pass with 100% code coverage. Testing includes unit tests for all types and functions, integration tests with the event bus, JSON conformance tests, concurrent access tests, and error case coverage.

Pre-Merge Verification

  • go fmt ./... - code is properly formatted
  • go test ./... - all tests pass
  • go test -coverprofile=coverage.out ./state && go tool cover -func=coverage.out - 100.0% coverage verified

🤖 Generated with Claude Code

Add an optional `state/` package that provides database-style state synchronization semantics on top of ebu's event bus. This enables interoperability with the durable-streams ecosystem while keeping core ebu unchanged.

Key features:
- Type-safe helpers (Insert, Update, Delete) using Go generics
- Options pattern for message configuration (TxID, timestamps)
- Materializer that builds typed state from events
- Full JSON interoperability with State Protocol spec
- 100% test coverage

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 30, 2025

Pull Request Review: State Package Implementation

Summary

This PR adds a well-designed state/ package that implements the Durable Streams State Protocol. The implementation is solid with excellent test coverage and follows Go best practices. Below is my detailed review.


✅ Strengths

Code Quality

  • Clean architecture: The package is properly separated into focused files (doc.go, message.go, helpers.go, materializer.go, options.go)
  • Type safety: Excellent use of Go generics throughout (Insert[T], Update[T], Store[T], TypedCollection[T])
  • Options pattern: Proper implementation of functional options for both ChangeMessage and Materializer configuration
  • Go idioms: Follows standard Go conventions and patterns
  • Documentation: Comprehensive package documentation with clear examples in doc.go

Design

  • Non-invasive: Properly implements the optional package pattern - no modifications to core ebu
  • Interface compliance: Messages correctly implement ebu's TypeNamer interface (EventTypeName() methods)
  • Concurrency safety: MemoryStore and Materializer use proper mutex locking (sync.RWMutex)
  • Extensibility: Store[T] interface allows custom storage implementations beyond MemoryStore
  • Flexibility: Support for both automatic type names and custom StateTypeName() interface

Testing

  • Comprehensive coverage: 1017 lines of tests covering 43 test cases
  • Coverage claim: PR claims 100% test coverage (should be verified with go test -coverprofile)
  • Test organization: Well-structured with clear sections (Message Types, Helpers, Store, Materializer, Integration, etc.)
  • Edge cases: Tests include error cases (empty keys, unknown types, concurrent access)

🔍 Issues & Concerns

1. Missing Validation in Control Messages (Minor)

Location: state/helpers.go:116-145

Control message constructors (SnapshotStart, SnapshotEnd, Reset) don't validate the offset parameter. Consider:

func SnapshotStart(offset string) (*ControlMessage, error) {
    if offset == "" {
        return nil, fmt.Errorf("state: offset cannot be empty")
    }
    // ...
}

Impact: Low - but could prevent confusing bugs


2. Potential Race Condition in Materializer.lastOffset (Medium)

Location: state/materializer.go:217, 283

lastOffset is read without locking in LastOffset() but written in Apply():

// Line 217 - Write (no lock)
m.lastOffset = event.Offset

// Line 283 - Read (no lock)
func (m *Materializer) LastOffset() eventbus.Offset {
    return m.lastOffset
}

Fix: Add mutex protection:

func (m *Materializer) LastOffset() eventbus.Offset {
    m.mu.RLock()
    defer m.mu.RUnlock()
    return m.lastOffset
}

Impact: Medium - potential data race under concurrent access


3. Error Handling Inconsistency (Minor)

Location: state/materializer.go:190-219

In Apply(), if unmarshaling the event fails, it returns an error. But for control message parsing, errors are silently ignored and it falls through to change message parsing. This could hide malformed control messages.

Suggestion: Add logging or explicit error handling for control message parsing failures.


4. Missing Nil Checks (Low)

Location: state/materializer.go:234-254

The applyChange() method doesn't check if msg is nil. While unlikely in practice, defensive programming would add:

func (m *Materializer) applyChange(msg *ChangeMessage) error {
    if msg == nil {
        return fmt.Errorf("state: nil change message")
    }
    // ...
}

5. JSON Marshaling Errors Could Be More Specific (Minor)

Location: state/helpers.go:95-98, 103-108

When JSON marshaling fails for complex types, the error message could be more helpful:

if err != nil {
    return nil, fmt.Errorf("state: marshal value of type %T: %w", value, err)
}

🔒 Security Considerations

✅ No Major Security Issues Found

  • No SQL injection risk: Uses type-safe operations, not string concatenation
  • No command injection: No shell commands or external process execution
  • JSON safety: Uses standard encoding/json package
  • Concurrency safe: Proper use of mutexes prevents data races

⚠️ Minor Concerns

  1. Unbounded memory growth: MemoryStore has no size limits. Large datasets could cause OOM. Consider documenting this limitation or adding optional size constraints.

  2. No input sanitization: Keys and type names are not validated for special characters. While not a security issue for in-memory use, could be problematic with certain storage backends.


⚡ Performance Considerations

Potential Optimizations

  1. Memory allocations in All() (state/materializer.go:71-78)

    • Creates a full copy of the store on every call
    • For large datasets, this could be expensive
    • Consider: Iterators or pagination for large collections
  2. Lock contention in Materializer (state/materializer.go:234-254)

    • Uses RLock for collection lookup, but holds it very briefly
    • Good pattern, but consider sync.Map for high-concurrency scenarios
  3. JSON unmarshaling on every apply (state/materializer.go:195-211)

    • Unavoidable for type safety, but worth noting
    • Performance is acceptable for most use cases

✅ Good Performance Patterns

  • Uses sync.RWMutex (allows concurrent reads)
  • Proper use of json.RawMessage to defer unmarshaling
  • Minimal allocations in hot paths

📋 Pre-Merge Checklist

Per CLAUDE.md requirements, please verify:

  • Run go fmt ./... on the state package
  • Run go test ./... and confirm all tests pass
  • Verify 100% coverage: go test -coverprofile=coverage.out ./state && go tool cover -func=coverage.out
  • Ensure no linter warnings: golint ./state/... (if applicable)

🎯 Recommendations

Must Fix (Before Merge)

  1. Fix race condition in Materializer.LastOffset() - Add mutex protection

Should Fix (High Priority)

  1. Add validation to control message constructors
  2. Improve error handling for control message parsing

Nice to Have (Low Priority)

  1. Add nil checks in public methods
  2. Enhance error messages with type information
  3. Document memory limitations of MemoryStore
  4. Consider adding size limits or pagination for large stores

📊 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

This is a well-crafted addition to the ebu ecosystem. The code quality is high, the design is clean, and the test coverage is excellent. The issues identified are mostly minor, with one medium-priority race condition that should be addressed before merging.

The package successfully achieves its goal of providing optional state synchronization without modifying core ebu. The use of Go generics provides excellent type safety, and the options pattern makes it flexible and extensible.

Recommendation: Approve with minor revisions (fix the race condition in LastOffset())


Great work on this implementation! 🚀

Fixes a potential race condition where lastOffset was being written in
Apply() and read in LastOffset() without synchronization. Now both
reads and writes are protected by the RWMutex.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 30, 2025

Code Review: State Package Implementation

I've completed a comprehensive review of this PR implementing the Durable Streams State Protocol. Overall, this is excellent work with high-quality implementation. Here are my findings:


✅ Strengths

Code Quality & Design

  • Clean architecture: The package is truly optional and non-invasive to core ebu
  • Excellent use of Go generics: Type-safe helpers (Insert, Update, Delete) provide great DX
  • Options pattern: Well-implemented for both ChangeMessage and Materializer configuration
  • Interface segregation: Store interface allows pluggable backends beyond MemoryStore
  • Comprehensive documentation: Package docs, function comments, and examples are excellent
  • TypeNamer pattern: Mirrors ebu's design for consistency, allows stable type names

Testing

  • 43 tests with claimed 100% coverage (needs verification via CI)
  • Good test organization: Unit tests, integration tests, JSON conformance, concurrent access
  • Thorough edge case coverage: Empty keys, marshal errors, unknown types, etc.

Thread Safety

  • Proper mutex usage: MemoryStore uses RWMutex correctly for concurrent access
  • Materializer synchronization: Protects collections map and lastOffset appropriately

🔍 Potential Issues & Suggestions

1. Potential Nil Pointer Issue in EntityType (state/message.go:90)

func EntityType(entity any) string {
    if namer, ok := entity.(TypeNamer); ok {
        return namer.StateTypeName()
    }
    return reflect.TypeOf(entity).String()  // ⚠️ Panics if entity is nil
}

Issue: If entity is nil, reflect.TypeOf(entity) returns nil, and calling .String() will panic.

Recommendation: Add nil check:

func EntityType(entity any) string {
    if namer, ok := entity.(TypeNamer); ok {
        return namer.StateTypeName()
    }
    t := reflect.TypeOf(entity)
    if t == nil {
        return "nil"  // or return an error
    }
    return t.String()
}

2. Missing Validation in Helper Functions

The helper functions validate empty keys but don't validate other constraints:

  • No validation that Operation is a valid enum value
  • No validation that Control is a valid enum value
  • No validation of timestamp format when parsing (though this might be acceptable)

Recommendation: Consider adding validation if strict protocol compliance is important.

3. MemoryStore.All() Returns Shallow Copy (state/materializer.go:71-78)

func (s *MemoryStore[T]) All() map[string]T {
    s.mu.RLock()
    defer s.mu.RUnlock()
    result := make(map[string]T, len(s.data))
    for k, v := range s.data {
        result[k] = v  // ⚠️ Shallow copy
    }
    return result
}

Issue: If T is a pointer or contains pointers, callers can mutate internal state.

Recommendation: Document this behavior clearly, or consider returning map[string]T as a read-only interface if deep immutability is required. For most use cases, this is acceptable but should be documented.

4. TypedCollection.Get() Inconsistency

// Get retrieves an entity by its key (without the type prefix).
func (c *TypedCollection[T]) Get(key string) (T, bool) {
    return c.store.Get(CompositeKey(c.entityType, key))
}

Observation: Get() takes a simple key but All() returns composite keys. This asymmetry could be confusing.

Recommendation: Document this clearly (which you have), or consider adding a GetByCompositeKey() method for consistency.

5. Materializer.Apply() Double Unmarshal

func (m *Materializer) Apply(event *eventbus.StoredEvent) error {
    var raw struct {
        Headers json.RawMessage `json:"headers"`
    }
    if err := json.Unmarshal(event.Data, &raw); err != nil {
        return fmt.Errorf("state: unmarshal event: %w", err)
    }
    
    var ctrlHeaders ControlHeaders
    if json.Unmarshal(raw.Headers, &ctrlHeaders) == nil && ctrlHeaders.Control != "" {
        // ...
    }
    
    var changeMsg ChangeMessage
    if err := json.Unmarshal(event.Data, &changeMsg); err != nil {  // ⚠️ Second full unmarshal
        return fmt.Errorf("state: unmarshal change message: %w", err)
    }
    // ...
}

Issue: Performance - unmarshals event.Data twice for change messages.

Recommendation: Consider unmarshaling once into a union type or using json.RawMessage more efficiently. For small messages this is fine, but could be optimized.

6. Missing Bounds Checking

  • No limits on key length, value size, or transaction ID length
  • No protection against memory exhaustion in MemoryStore

Recommendation: Consider adding configurable limits if this will be used in production with untrusted input.


🔒 Security Considerations

No Critical Issues Found

  • ✅ No SQL injection (no SQL)
  • ✅ No command injection
  • ✅ No path traversal
  • ✅ Proper error wrapping (uses %w)
  • ✅ Thread-safe implementation

Minor Considerations

  1. DoS via unbounded memory growth: MemoryStore has no size limits. Document or add optional max size.
  2. JSON unmarshaling: Relies on Go's json package which is safe, but malformed JSON could cause panics. Current error handling is appropriate.

⚡ Performance Considerations

Good

  • ✅ Efficient use of RWMutex (read locks for reads, write locks for writes)
  • ✅ Map pre-sizing in All() method
  • ✅ Minimal allocations in hot paths

Could Improve

  1. Double unmarshal in Apply() (mentioned above)
  2. All() creates full copy: Consider iterator pattern for large collections
  3. No index/cache for composite keys: If lookups by type become common, consider indexing

Benchmark Recommendation

Consider adding benchmarks for:

  • Insert/Update/Delete operations
  • Concurrent access patterns
  • Large collection materialization

📋 Pre-Merge Checklist

Per CLAUDE.md, before merging verify:

  • Run go fmt ./... (code appears formatted but verify)
  • Run go test ./... and ensure all tests pass
  • Run go test -coverprofile=coverage.out ./... && go tool cover -func=coverage.out to verify 100% coverage claim

🎯 Verdict

This is high-quality code that follows Go best practices and the repository's standards. The implementation is:

  • Well-designed and maintainable
  • Properly documented
  • Thread-safe
  • Non-invasive to core ebu

Recommendations Priority

  1. High Priority: Fix the nil pointer issue in EntityType() (state/message.go:90)
  2. Medium Priority: Document MemoryStore.All() shallow copy behavior
  3. Low Priority: Consider performance optimizations (double unmarshal, bounds checking)

The nil pointer issue should be addressed before merge. Other items are minor and can be addressed in follow-up PRs if desired.

Great work on this implementation! 🚀

jilio and others added 2 commits December 30, 2025 22:13
Add nil check in EntityType() to return "nil" string instead of
panicking when reflect.TypeOf(nil).String() is called.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Document the new optional state package that implements the Durable
Streams State Protocol for database-style state synchronization.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown

claude Bot commented Dec 30, 2025

Code Review for State Package Implementation

Summary

This is a well-architected implementation of the Durable Streams State Protocol for ebu. The code demonstrates strong design principles, comprehensive testing, and thoughtful integration with the existing ebu architecture.

Strengths

Code Quality ✅

  • Clean Architecture: The package follows Go best practices with clear separation of concerns across message types, helpers, options, and materializer
  • Type Safety: Excellent use of Go generics for type-safe operations (Insert[T], Update[T], etc.)
  • Options Pattern: Consistent use of functional options for both ChangeMessage and Materializer configuration
  • Documentation: Comprehensive package documentation with clear examples and usage patterns
  • Error Handling: Proper error wrapping with context using fmt.Errorf with %w

Test Coverage ✅

  • Comprehensive Testing: 43 tests covering unit, integration, JSON conformance, concurrent access, and error cases
  • Edge Cases: Good coverage of error conditions (empty keys, unmarshalable types, invalid JSON)
  • Concurrency: Dedicated tests for concurrent access to MemoryStore and Materializer
  • Integration: Tests demonstrate real-world usage with ebu's EventBus and Replay functionality

Design Decisions ✅

  • Optional Package: Smart decision to keep this as an optional add-on without modifying core ebu
  • Interface Segregation: Store[T] interface allows for multiple storage implementations
  • Thread Safety: Proper use of sync.RWMutex for concurrent access patterns
  • TypeNamer Pattern: Consistent with ebu's existing TypeNamer interface for stable event naming

Issues & Recommendations

🟡 Minor Issues

1. Potential Race Condition in Materializer.applyControl (materializer.go:263-271)

Location: state/materializer.go:263-271

case ControlReset:
    m.mu.Lock()
    for _, c := range m.collections {
        c.clear()
    }
    m.mu.Unlock()
    if m.cfg.onReset != nil {
        m.cfg.onReset()  // Called after unlock
    }

Issue: The onReset callback is invoked after releasing the lock. If the callback tries to query the materializer state or if new events are applied concurrently, there could be inconsistency.

Recommendation: Consider documenting this behavior or holding the lock during callback execution (though this could cause deadlocks if callbacks try to acquire the lock).

2. Missing Validation in newChangeMessage (helpers.go:60-112)

Location: state/helpers.go:60-112

Issue: The function validates empty keys but doesn't validate:

  • Empty or invalid entity type names
  • Potential issues with operations (e.g., Insert with nil value pointer)

Example:

msg, _ := Insert("key", User{}, WithEntityType(""))  // Empty type name accepted

Recommendation: Add validation for entity type names:

if entityType == "" || entityType == "nil" {
    return nil, fmt.Errorf("state: invalid entity type")
}

3. Incomplete Type Discrimination in Apply (materializer.go:190-223)

Location: state/materializer.go:190-223

Issue: The message type detection relies on checking if Control field is non-empty, which could be fragile if the JSON structure changes.

Current approach:

if json.Unmarshal(raw.Headers, &ctrlHeaders) == nil && ctrlHeaders.Control != "" {
    // Treat as control message
}

Recommendation: Consider using the event's Type field from StoredEvent if available, or add an explicit type discriminator field to messages.

🔵 Enhancement Opportunities

4. Store Interface Lacks Iterator Pattern (materializer.go:12-25)

Location: state/materializer.go:12-25

Issue: The Store interface's All() method returns a full copy of all entities, which could be memory-intensive for large datasets.

Recommendation: Consider adding an iterator or callback-based traversal:

type Store[T any] interface {
    // ... existing methods
    ForEach(func(key string, value T) bool) error
}

5. Missing Telemetry/Observability Hooks (materializer.go:156-296)

Issue: The Materializer lacks integration with observability systems. Given that ebu has Observability support, this package could benefit from metrics and tracing.

Recommendation: Consider adding:

  • Metrics for apply operations (success/failure rates)
  • Trace spans for materialization operations
  • Callbacks for monitoring state changes

6. No Validation for Invalid Operations (materializer.go:134-150)

Issue: The applyChange method treats Insert and Update identically, which could lead to:

  • Inserting over existing entities without warning
  • Updating non-existent entities without error

Recommendation: Add optional strict mode validation:

case OperationInsert:
    if _, exists := a.collection.store.Get(key); exists && strictInsert {
        return fmt.Errorf("state: entity already exists: %s", key)
    }
case OperationUpdate:
    if _, exists := a.collection.store.Get(key); !exists && strictUpdate {
        return fmt.Errorf("state: entity does not exist: %s", key)
    }

🟢 Performance Considerations

7. Memory Allocation in All() (materializer.go:70-78)

Location: state/materializer.go:70-78

Observation: All() creates a full copy of the data map on every call. This is safe but potentially expensive.

Status: Acceptable for current use case, but document the memory implications for large datasets.

8. JSON Marshaling on Every Insert/Update (helpers.go:93-109)

Location: state/helpers.go:93-109

Observation: Values are marshaled to JSON immediately upon message creation, even if the message might not be published.

Status: This is likely intentional for early error detection and is acceptable.

Security Considerations ✅

  • No SQL Injection: All data is handled as JSON, no raw SQL
  • No Path Traversal: Keys are treated as opaque strings
  • No Code Injection: No dynamic code execution
  • Proper Error Messages: Error messages don't leak sensitive information

Pre-Merge Checklist

Before merging, please verify (as per CLAUDE.md):

  • Run go fmt ./state/... to format all Go code
  • Run go test ./state/... to ensure all tests pass
  • Run go test -coverprofile=coverage.out ./state/... && go tool cover -func=coverage.out to verify 100% coverage claim

Recommendations Summary

Must Fix Before Merge

None - the code is production-ready as-is.

Should Consider

  1. Add validation for empty/nil entity type names
  2. Document the lock release behavior in applyControl callbacks
  3. Consider adding type discrimination to message structs

Nice to Have

  1. Add iterator pattern for large datasets
  2. Integrate observability hooks
  3. Add strict mode for Insert/Update validation

Overall Assessment

LGTM with minor suggestions

This is high-quality code that demonstrates:

  • Strong understanding of Go idioms and best practices
  • Thoughtful API design with clear ergonomics
  • Comprehensive testing approach
  • Good integration with existing ebu patterns

The minor issues identified are largely enhancement opportunities rather than blocking concerns. The implementation is solid, well-tested, and ready for production use.

Recommendation: Approve and merge. The suggested improvements can be addressed in future iterations if needed.

@claude
Copy link
Copy Markdown

claude Bot commented Dec 30, 2025

Code Review: State Package Implementation

Overview

This PR adds an optional state/ package implementing the Durable Streams State Protocol. The implementation is well-designed, follows Go best practices, and integrates seamlessly with the existing ebu architecture. The code quality is excellent with comprehensive test coverage.

✅ Strengths

Architecture & Design

  • Clean separation of concerns: The package is truly optional and doesn't modify core ebu
  • Excellent use of generics: Type-safe helpers (Insert, Update, Delete) provide great DX
  • Options pattern: Consistent with ebu's patterns (WithTxID, WithTimestamp, etc.)
  • Interface-based design: Store interface allows multiple backend implementations
  • Thread-safe: MemoryStore properly uses sync.RWMutex for concurrent access

Code Quality

  • Well-documented: Comprehensive godoc with examples in doc.go
  • Consistent error handling: All errors properly wrapped with context
  • JSON interoperability: Proper use of json.RawMessage for flexibility
  • Type safety: Smart use of generics throughout

Integration

  • Seamless ebu integration: ChangeMessage/ControlMessage implement TypeNamer
  • Replay integration: Materializer.Replay() provides convenient wrapper
  • Event protocol compliance: Follows Durable Streams State Protocol spec

🔍 Code Quality Observations

message.go (lines 87-95)

The EntityType() function has good fallback logic. Consider documenting that reflect.TypeOf().String() returns package-qualified names which may not be stable across package refactoring (hence the recommendation to implement TypeNamer).

helpers.go (lines 60-75)

The zero value instantiation pattern var zero T is elegant for determining entity types. However, note that for pointer types, this will use the pointer type name, not the underlying type. This is likely intentional but worth documenting.

materializer.go (lines 190-223)

The Apply() method's type detection logic is clever - checking for control headers first. However, there's a potential edge case:

Issue: If json.Unmarshal(event.Data, &raw) succeeds but the Data contains neither a valid ControlMessage nor ChangeMessage, the error from line 212 will be returned, but it may not accurately reflect the actual problem.

Suggestion: Consider adding a more descriptive error message that indicates the event data doesn't match either expected message type.

materializer.go (lines 237-258)

Good error handling with the strictSchema option. The dual-mode behavior (strict vs lenient) is well-designed for different use cases.

🔐 Security Considerations

No Critical Issues Found

The code handles untrusted JSON input safely:

  • Uses json.Unmarshal with proper error checking
  • No reflection-based type instantiation from user input
  • No SQL injection risks (no database queries)
  • No command injection risks

Minor observation: The package stores arbitrary JSON blobs in Value/OldValue fields. This is fine, but consumers should be aware that malicious JSON could consume memory if entities are large. This is an acceptable trade-off for the flexibility provided.

⚡ Performance Considerations

Good Performance Characteristics

  • Efficient locking: Uses RWMutex appropriately (read locks for reads, write locks for mutations)
  • Minimal allocations: Direct unmarshaling into target types
  • No unnecessary copying: json.RawMessage avoids double-marshaling

Potential Optimizations (Not Required)

  1. materializer.go:71-78 - The All() method creates a full copy of the map. This is safe but could be expensive for large datasets. Consider documenting this behavior or adding a streaming iteration API in the future.

  2. materializer.go:264-267 - The Reset operation clears all collections while holding a write lock. For large datasets, this could block reads. Consider if this is acceptable for your use case (likely is).

🧪 Test Coverage

The PR claims 100% test coverage with 43 tests. The test file (state_test.go) shows:

  • ✅ Comprehensive unit tests for all public functions
  • ✅ Integration tests with EventBus
  • ✅ Concurrent access tests
  • ✅ Error case coverage
  • ✅ JSON conformance tests

Recommendation: Before merging, verify coverage with:

go test -coverprofile=coverage.out ./state/...
go tool cover -func=coverage.out

📋 Best Practices Compliance

Per CLAUDE.md requirements:

Go idioms: Excellent use of interfaces, generics, and options pattern
Options pattern: Consistent throughout (ChangeOption, MaterializerOption)
Simple & focused: Each component has a single responsibility
⚠️ Formatting: Should run go fmt ./... before merge (per CLAUDE.md)
⚠️ Tests: Should run go test ./... before merge (per CLAUDE.md)
⚠️ Coverage: Should verify 100% coverage claim (per CLAUDE.md)

🐛 Potential Issues

Minor: Empty key validation (helpers.go:61-63)

The validation checks for empty keys but doesn't validate the key format. Consider if keys containing "/" could cause issues since CompositeKey uses "/" as a separator.

Example edge case:

msg, _ := Insert("user/admin", User{Name: "Alice"})
// Composite key becomes: "state.User/user/admin"
// This could be ambiguous when parsing

Recommendation: Either:

  1. Document that keys should not contain "/"
  2. Add validation to reject keys containing "/"
  3. URL-encode keys containing special characters

Minor: Materializer offset tracking (materializer.go:203-205, 219-221)

The lastOffset is updated inside the Apply() method, but if Apply() is called with events out of order, the lastOffset could be misleading. Consider documenting that events should be replayed in order.

📝 Documentation Quality

Excellent Documentation

  • ✅ Package-level doc with usage examples
  • ✅ All exported functions/types documented
  • ✅ Code examples in comments
  • ✅ Clear explanation of State Protocol concepts

README.md Addition

The README addition (lines 387-428) is clear and provides good quick-start examples. Well done.

🎯 Recommendations

Before Merge (Required per CLAUDE.md):

  1. Run go fmt ./...
  2. Run go test ./... and verify all tests pass
  3. Run coverage check and confirm 100% coverage

Consider for Future (Optional):

  1. Add validation for "/" in keys or document the limitation
  2. Consider adding a streaming API for large collections (e.g., ForEach method)
  3. Document the ordering requirements for Materializer.Apply()
  4. Consider adding metrics/observability hooks (e.g., change counter, error counter)

🏆 Overall Assessment

This is high-quality code that follows Go best practices and integrates well with ebu's architecture. The implementation is clean, well-tested, and production-ready.

Recommendation: ✅ Approve with minor suggestions

The code is ready to merge after running the standard pre-merge checks (fmt, test, coverage) as specified in CLAUDE.md. The minor issues noted above are not blockers but could be addressed in follow-up PRs if desired.

Great work on this implementation! 🎉

@jilio jilio merged commit 554880e into main Dec 30, 2025
3 checks passed
@jilio jilio deleted the jilio/adopt-state-protocol branch December 30, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant