From 2739a090b8301d7bfebfb9a9581fb7ca9406fcc2 Mon Sep 17 00:00:00 2001
From: Snider <snider@host.uk.com>
Date: Wed, 4 Feb 2026 12:23:14 +0000
Subject: [PATCH] fix(core): address review feedback from Gemini and Copilot

- Move locked check inside mutex in registerService to fix TOCTOU race
- Add mutex guards to enableLock and applyLock methods
- Replace fmt.Errorf with errors.Join in action() for correct error
  aggregation (consistent with queryAll and lifecycle methods)
- Add TestMessageBus_Action_Bad for error aggregation coverage

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
---
 pkg/framework/core/message_bus.go      |  3 +--
 pkg/framework/core/message_bus_test.go | 17 +++++++++++++++++
 pkg/framework/core/service_manager.go  | 10 +++++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/pkg/framework/core/message_bus.go b/pkg/framework/core/message_bus.go
index c01b3cc..457ced2 100644
--- a/pkg/framework/core/message_bus.go
+++ b/pkg/framework/core/message_bus.go
@@ -2,7 +2,6 @@ package core
 
 import (
 	"errors"
-	"fmt"
 	"sync"
 )
 
@@ -35,7 +34,7 @@ func (b *messageBus) action(msg Message) error {
 	var agg error
 	for _, h := range handlers {
 		if err := h(b.core, msg); err != nil {
-			agg = fmt.Errorf("%w; %v", agg, err)
+			agg = errors.Join(agg, err)
 		}
 	}
 	return agg
diff --git a/pkg/framework/core/message_bus_test.go b/pkg/framework/core/message_bus_test.go
index d661dc7..e69ac95 100644
--- a/pkg/framework/core/message_bus_test.go
+++ b/pkg/framework/core/message_bus_test.go
@@ -1,6 +1,7 @@
 package core
 
 import (
+	"errors"
 	"sync"
 	"testing"
 
@@ -25,6 +26,22 @@ func TestMessageBus_Action_Good(t *testing.T) {
 	assert.Len(t, received, 2)
 }
 
+func TestMessageBus_Action_Bad(t *testing.T) {
+	c, _ := New()
+
+	err1 := errors.New("handler1 failed")
+	err2 := errors.New("handler2 failed")
+
+	c.bus.registerAction(func(_ *Core, msg Message) error { return err1 })
+	c.bus.registerAction(func(_ *Core, msg Message) error { return nil })
+	c.bus.registerAction(func(_ *Core, msg Message) error { return err2 })
+
+	err := c.bus.action("test")
+	assert.Error(t, err)
+	assert.ErrorIs(t, err, err1)
+	assert.ErrorIs(t, err, err2)
+}
+
 func TestMessageBus_RegisterAction_Good(t *testing.T) {
 	c, _ := New()
 
diff --git a/pkg/framework/core/service_manager.go b/pkg/framework/core/service_manager.go
index b2996c4..80c208f 100644
--- a/pkg/framework/core/service_manager.go
+++ b/pkg/framework/core/service_manager.go
@@ -26,14 +26,14 @@ func newServiceManager() *serviceManager {
 // registerService adds a named service to the registry.
 // It also appends to startables/stoppables if the service implements those interfaces.
 func (m *serviceManager) registerService(name string, svc any) error {
-	if m.locked {
-		return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name)
-	}
 	if name == "" {
 		return fmt.Errorf("core: service name cannot be empty")
 	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
+	if m.locked {
+		return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name)
+	}
 	if _, exists := m.services[name]; exists {
 		return fmt.Errorf("core: service %q already registered", name)
 	}
@@ -62,12 +62,16 @@ func (m *serviceManager) service(name string) any {
 
 // enableLock marks that the lock should be applied after initialisation.
 func (m *serviceManager) enableLock() {
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	m.lockEnabled = true
 }
 
 // applyLock activates the service lock if it was enabled.
 // Called once during New() after all options have been processed.
 func (m *serviceManager) applyLock() {
+	m.mu.Lock()
+	defer m.mu.Unlock()
 	if m.lockEnabled {
 		m.locked = true
 	}