[Issue #37] Replace interface{} with properly defined interfaces #42

HugoNijhuis · 2026-01-10T14:33:15Z

HugoNijhuis commented

2026-01-10 14:33:15 +00:00

Closes #37

Summary

Add VirtualMachine interface with GetID(), GetActorID(), GetState() for type-safe VM access
Add RuntimeModel and RuntimeMessage interfaces for event storming model and actor message contracts
Add VMProvider interface for decoupled VM access in cluster registry
Update VMRegistry.GetActiveVMs() to return map[string]VirtualMachine instead of map[string]interface{}
Update Runtime interface to use proper types instead of interface{}
Refactor DistributedVMRegistry to use dependency injection via VMProvider

Test plan

All existing tests pass
Build succeeds without errors
Manual verification of cluster operations

Generated with Claude Code

Closes #37 ## Summary - Add `VirtualMachine` interface with `GetID()`, `GetActorID()`, `GetState()` for type-safe VM access - Add `RuntimeModel` and `RuntimeMessage` interfaces for event storming model and actor message contracts - Add `VMProvider` interface for decoupled VM access in cluster registry - Update `VMRegistry.GetActiveVMs()` to return `map[string]VirtualMachine` instead of `map[string]interface{}` - Update `Runtime` interface to use proper types instead of `interface{}` - Refactor `DistributedVMRegistry` to use dependency injection via `VMProvider` ## Test plan - [x] All existing tests pass - [x] Build succeeds without errors - [ ] Manual verification of cluster operations Generated with Claude Code

HugoNijhuis added 1 commit 2026-01-10 14:33:15 +00:00

Replace interface{} with properly defined interfaces

CI / build (pull_request) Successful in 16s

Details

8c02b63dc7

- Add VirtualMachine interface with GetID(), GetActorID(), GetState()
- Add VMState type with idle/running/paused/stopped states
- Add RuntimeModel interface for event storming model contracts
- Add RuntimeMessage interface for actor message contracts
- Add VMProvider interface for decoupled VM access
- Update VMRegistry.GetActiveVMs() to return map[string]VirtualMachine
- Update Runtime interface to use RuntimeModel and RuntimeMessage
- Update DistributedVMRegistry to use VMProvider instead of interface{}
- Add SetVMProvider method to DistributedVM for dependency injection

This improves type safety, makes contracts explicit, and enables better
IDE support while avoiding import cycles through interface segregation.

Closes #37

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

HugoNijhuis commented

2026-01-10 14:39:47 +00:00

Code Review: PR #42 - Replace interface{} with properly defined interfaces

Overall Assessment

Good refactoring that improves type safety. The changes follow Go best practices for interface design.

Issues to Address

Type Assertion Bug in handleClusterMessage (distributed.go:171-180)

The type assertions will fail at runtime because JSON unmarshals into map[string]interface{}, not concrete types implementing RuntimeModel/RuntimeMessage. The assertion will never succeed.

Fix: Add concrete structs for JSON unmarshaling, or re-marshal and unmarshal the payload.

Verdict

The design is sound. However, the type assertion issue is a functional bug.

Recommendation: Needs Work

## Code Review: PR #42 - Replace interface{} with properly defined interfaces ### Overall Assessment Good refactoring that improves type safety. The changes follow Go best practices for interface design. ### Issues to Address **Type Assertion Bug in handleClusterMessage (distributed.go:171-180)** The type assertions will fail at runtime because JSON unmarshals into map[string]interface{}, not concrete types implementing RuntimeModel/RuntimeMessage. The assertion will never succeed. Fix: Add concrete structs for JSON unmarshaling, or re-marshal and unmarshal the payload. ### Verdict The design is sound. However, the type assertion issue is a functional bug. **Recommendation: Needs Work**

HugoNijhuis added 1 commit 2026-01-10 15:41:46 +00:00

Fix type assertion bug in handleClusterMessage

CI / build (pull_request) Successful in 16s

Details

a33ef47a39

JSON unmarshal produces map[string]interface{}, not concrete types.
Added ModelPayload and MessagePayload concrete types that implement
RuntimeModel and RuntimeMessage interfaces respectively.

The handleClusterMessage now re-marshals and unmarshals the payload
to convert from map[string]interface{} to the proper concrete type.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

HugoNijhuis commented

2026-01-10 15:41:52 +00:00

Fix Applied

Addressed the type assertion bug in handleClusterMessage:

Added ModelPayload and MessagePayload concrete types that implement RuntimeModel and RuntimeMessage interfaces
Updated handleClusterMessage to re-marshal/unmarshal the payload to convert from map[string]interface{} to proper concrete types
All tests pass

Ready for re-review.

## Fix Applied Addressed the type assertion bug in handleClusterMessage: - Added `ModelPayload` and `MessagePayload` concrete types that implement `RuntimeModel` and `RuntimeMessage` interfaces - Updated `handleClusterMessage` to re-marshal/unmarshal the payload to convert from `map[string]interface{}` to proper concrete types - All tests pass Ready for re-review.

HugoNijhuis force-pushed issue-37-replace-interface from a33ef47a39 to b759c7fb97

2026-01-10 15:45:57 +00:00

Compare

HugoNijhuis commented

2026-01-10 15:46:42 +00:00

Re-Review: Approved

The type assertion bug has been fixed correctly:

Added ModelPayload and MessagePayload concrete types implementing the interfaces
handleClusterMessage now properly re-marshals/unmarshals payloads
Branch rebased onto latest main, conflicts resolved

All tests pass, build succeeds. Ready to merge.

## Re-Review: Approved The type assertion bug has been fixed correctly: - Added ModelPayload and MessagePayload concrete types implementing the interfaces - handleClusterMessage now properly re-marshals/unmarshals payloads - Branch rebased onto latest main, conflicts resolved All tests pass, build succeeds. Ready to merge.

HugoNijhuis merged commit 8df36cac7a into main

2026-01-10 17:46:12 +00:00

HugoNijhuis referenced this issue from a commit

2026-01-10 17:46:12 +00:00

Merge pull request '[Issue #37] Replace interface{} with properly defined interfaces' (#42) from issue-37-replace-interface into main

HugoNijhuis commented

2026-01-10 23:02:50 +00:00

Code Review: PR #42 - Replace interface{} with properly defined interfaces

Overall Assessment

Good refactoring that improves type safety by replacing interface{} with properly defined interfaces. The changes follow Go best practices for interface design.

What Works Well

Well-documented interfaces with clear purpose and method contracts
VMProvider pattern enables proper dependency injection
Interfaces are minimal (only the methods needed)
Removed emoji characters from log messages (cleaner for production)
Fixed missing newline at end of files

Issues to Address

1. Type Assertion Bug in handleClusterMessage (distributed.go:171-180)

The type assertions will always fail at runtime:

if model, ok := clusterMsg.Payload.(RuntimeModel); ok {
    dvm.localRuntime.LoadModel(model)
}

When JSON unmarshals into interface{}, it produces map[string]interface{}, not a concrete type implementing RuntimeModel. The assertion will never succeed.

Suggested fix: Either:

Add a concrete struct implementing RuntimeModel that JSON can unmarshal into, then re-unmarshal the payload
Or use the pattern: payloadBytes, _ := json.Marshal(clusterMsg.Payload); json.Unmarshal(payloadBytes, &concreteModel)

2. Silent Failures (Minor)

Failed type assertions in handleClusterMessage are silently ignored. Consider logging when payloads cannot be processed:

if model, ok := clusterMsg.Payload.(RuntimeModel); ok {
    dvm.localRuntime.LoadModel(model)
} else {
    // Log that we received a load_model message but couldn't process it
}

Verdict

The design is sound and the interfaces are well-structured. However, the type assertion issue in handleClusterMessage represents a functional bug that should be addressed before merging.

Recommendation: Needs Work - Please fix the type assertion issue, then this is ready to merge.

## Code Review: PR #42 - Replace interface{} with properly defined interfaces ### Overall Assessment Good refactoring that improves type safety by replacing `interface{}` with properly defined interfaces. The changes follow Go best practices for interface design. ### What Works Well - Well-documented interfaces with clear purpose and method contracts - VMProvider pattern enables proper dependency injection - Interfaces are minimal (only the methods needed) - Removed emoji characters from log messages (cleaner for production) - Fixed missing newline at end of files ### Issues to Address **1. Type Assertion Bug in handleClusterMessage (distributed.go:171-180)** The type assertions will always fail at runtime: ```go if model, ok := clusterMsg.Payload.(RuntimeModel); ok { dvm.localRuntime.LoadModel(model) } ``` When JSON unmarshals into `interface{}`, it produces `map[string]interface{}`, not a concrete type implementing `RuntimeModel`. The assertion will never succeed. **Suggested fix:** Either: - Add a concrete struct implementing RuntimeModel that JSON can unmarshal into, then re-unmarshal the payload - Or use the pattern: `payloadBytes, _ := json.Marshal(clusterMsg.Payload); json.Unmarshal(payloadBytes, &concreteModel)` **2. Silent Failures (Minor)** Failed type assertions in `handleClusterMessage` are silently ignored. Consider logging when payloads cannot be processed: ```go if model, ok := clusterMsg.Payload.(RuntimeModel); ok { dvm.localRuntime.LoadModel(model) } else { // Log that we received a load_model message but couldn't process it } ``` ### Verdict The design is sound and the interfaces are well-structured. However, the type assertion issue in `handleClusterMessage` represents a functional bug that should be addressed before merging. **Recommendation: Needs Work** - Please fix the type assertion issue, then this is ready to merge.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: flowmade-one/aether#42