From 2f39278706b33c8d0351563e7032e4789d81f151 Mon Sep 17 00:00:00 2001 From: Snider Date: Sun, 1 Feb 2026 15:52:32 +0000 Subject: [PATCH] feat(process): add Logger interface for exec wrapper - Define Logger interface with Debug and Error methods - Add NopLogger as default (no-op implementation) - Add SetDefaultLogger/DefaultLogger for package-level config - Add WithLogger method for per-command logger injection - Log commands at DEBUG level before execution - Log failures at ERROR level with error details - Add comprehensive tests for logger functionality Compatible with pkg/log.Logger and other structured loggers. Closes #90 Co-Authored-By: Claude --- pkg/process/exec/exec.go | 54 ++++++++++--- pkg/process/exec/exec_test.go | 148 ++++++++++++++++++++++++++++++++++ pkg/process/exec/logger.go | 30 +++++++ 3 files changed, 220 insertions(+), 12 deletions(-) create mode 100644 pkg/process/exec/exec_test.go create mode 100644 pkg/process/exec/logger.go diff --git a/pkg/process/exec/exec.go b/pkg/process/exec/exec.go index ed8f397b..21978a98 100644 --- a/pkg/process/exec/exec.go +++ b/pkg/process/exec/exec.go @@ -32,11 +32,12 @@ func Command(ctx context.Context, name string, args ...string) *Cmd { // Cmd represents a wrapped command type Cmd struct { - name string - args []string - ctx context.Context - opts Options - cmd *exec.Cmd + name string + args []string + ctx context.Context + opts Options + cmd *exec.Cmd + logger Logger } // WithDir sets the working directory @@ -69,17 +70,23 @@ func (c *Cmd) WithStderr(w io.Writer) *Cmd { return c } +// WithLogger sets a custom logger for this command. +// If not set, the package default logger is used. +func (c *Cmd) WithLogger(l Logger) *Cmd { + c.logger = l + return c +} + // Run executes the command and waits for it to finish. // It automatically logs the command execution at debug level. func (c *Cmd) Run() error { c.prepare() - - // TODO: Use a proper logger interface when available in pkg/process - // For now using cli.Debug which might not be visible unless verbose - // cli.Debug("Executing: %s %s", c.name, strings.Join(c.args, " ")) + c.logDebug("executing command") if err := c.cmd.Run(); err != nil { - return wrapError(err, c.name, c.args) + wrapped := wrapError(err, c.name, c.args) + c.logError("command failed", wrapped) + return wrapped } return nil } @@ -87,9 +94,13 @@ func (c *Cmd) Run() error { // Output runs the command and returns its standard output. func (c *Cmd) Output() ([]byte, error) { c.prepare() + c.logDebug("executing command") + out, err := c.cmd.Output() if err != nil { - return nil, wrapError(err, c.name, c.args) + wrapped := wrapError(err, c.name, c.args) + c.logError("command failed", wrapped) + return nil, wrapped } return out, nil } @@ -97,9 +108,13 @@ func (c *Cmd) Output() ([]byte, error) { // CombinedOutput runs the command and returns its combined standard output and standard error. func (c *Cmd) CombinedOutput() ([]byte, error) { c.prepare() + c.logDebug("executing command") + out, err := c.cmd.CombinedOutput() if err != nil { - return out, wrapError(err, c.name, c.args) + wrapped := wrapError(err, c.name, c.args) + c.logError("command failed", wrapped) + return out, wrapped } return out, nil } @@ -144,3 +159,18 @@ func wrapError(err error, name string, args []string) error { } return fmt.Errorf("failed to execute %q: %w", cmdStr, err) } + +func (c *Cmd) getLogger() Logger { + if c.logger != nil { + return c.logger + } + return defaultLogger +} + +func (c *Cmd) logDebug(msg string) { + c.getLogger().Debug(msg, "cmd", c.name, "args", strings.Join(c.args, " ")) +} + +func (c *Cmd) logError(msg string, err error) { + c.getLogger().Error(msg, "cmd", c.name, "args", strings.Join(c.args, " "), "err", err) +} diff --git a/pkg/process/exec/exec_test.go b/pkg/process/exec/exec_test.go new file mode 100644 index 00000000..f014933d --- /dev/null +++ b/pkg/process/exec/exec_test.go @@ -0,0 +1,148 @@ +package exec_test + +import ( + "context" + "strings" + "testing" + + "github.com/host-uk/core/pkg/process/exec" +) + +// mockLogger captures log calls for testing +type mockLogger struct { + debugCalls []logCall + errorCalls []logCall +} + +type logCall struct { + msg string + keyvals []any +} + +func (m *mockLogger) Debug(msg string, keyvals ...any) { + m.debugCalls = append(m.debugCalls, logCall{msg, keyvals}) +} + +func (m *mockLogger) Error(msg string, keyvals ...any) { + m.errorCalls = append(m.errorCalls, logCall{msg, keyvals}) +} + +func TestCommand_Run_Good_LogsDebug(t *testing.T) { + logger := &mockLogger{} + ctx := context.Background() + + err := exec.Command(ctx, "echo", "hello"). + WithLogger(logger). + Run() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(logger.debugCalls) != 1 { + t.Fatalf("expected 1 debug call, got %d", len(logger.debugCalls)) + } + if logger.debugCalls[0].msg != "executing command" { + t.Errorf("expected msg 'executing command', got %q", logger.debugCalls[0].msg) + } + if len(logger.errorCalls) != 0 { + t.Errorf("expected no error calls, got %d", len(logger.errorCalls)) + } +} + +func TestCommand_Run_Bad_LogsError(t *testing.T) { + logger := &mockLogger{} + ctx := context.Background() + + err := exec.Command(ctx, "false"). + WithLogger(logger). + Run() + if err == nil { + t.Fatal("expected error") + } + + if len(logger.debugCalls) != 1 { + t.Fatalf("expected 1 debug call, got %d", len(logger.debugCalls)) + } + if len(logger.errorCalls) != 1 { + t.Fatalf("expected 1 error call, got %d", len(logger.errorCalls)) + } + if logger.errorCalls[0].msg != "command failed" { + t.Errorf("expected msg 'command failed', got %q", logger.errorCalls[0].msg) + } +} + +func TestCommand_Output_Good(t *testing.T) { + logger := &mockLogger{} + ctx := context.Background() + + out, err := exec.Command(ctx, "echo", "test"). + WithLogger(logger). + Output() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if strings.TrimSpace(string(out)) != "test" { + t.Errorf("expected 'test', got %q", string(out)) + } + if len(logger.debugCalls) != 1 { + t.Errorf("expected 1 debug call, got %d", len(logger.debugCalls)) + } +} + +func TestCommand_CombinedOutput_Good(t *testing.T) { + logger := &mockLogger{} + ctx := context.Background() + + out, err := exec.Command(ctx, "echo", "combined"). + WithLogger(logger). + CombinedOutput() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if strings.TrimSpace(string(out)) != "combined" { + t.Errorf("expected 'combined', got %q", string(out)) + } + if len(logger.debugCalls) != 1 { + t.Errorf("expected 1 debug call, got %d", len(logger.debugCalls)) + } +} + +func TestNopLogger(t *testing.T) { + // Verify NopLogger doesn't panic + var nop exec.NopLogger + nop.Debug("msg", "key", "val") + nop.Error("msg", "key", "val") +} + +func TestSetDefaultLogger(t *testing.T) { + original := exec.DefaultLogger() + defer exec.SetDefaultLogger(original) + + logger := &mockLogger{} + exec.SetDefaultLogger(logger) + + if exec.DefaultLogger() != logger { + t.Error("default logger not set correctly") + } + + // Test nil resets to NopLogger + exec.SetDefaultLogger(nil) + if _, ok := exec.DefaultLogger().(exec.NopLogger); !ok { + t.Error("expected NopLogger when setting nil") + } +} + +func TestCommand_UsesDefaultLogger(t *testing.T) { + original := exec.DefaultLogger() + defer exec.SetDefaultLogger(original) + + logger := &mockLogger{} + exec.SetDefaultLogger(logger) + + ctx := context.Background() + _ = exec.Command(ctx, "echo", "test").Run() + + if len(logger.debugCalls) != 1 { + t.Errorf("expected default logger to receive 1 debug call, got %d", len(logger.debugCalls)) + } +} diff --git a/pkg/process/exec/logger.go b/pkg/process/exec/logger.go new file mode 100644 index 00000000..0892d798 --- /dev/null +++ b/pkg/process/exec/logger.go @@ -0,0 +1,30 @@ +package exec + +// Logger interface for command execution logging. +// Compatible with pkg/log.Logger and other structured loggers. +type Logger interface { + Debug(msg string, keyvals ...any) + Error(msg string, keyvals ...any) +} + +// NopLogger is a no-op logger that discards all messages. +type NopLogger struct{} + +func (NopLogger) Debug(string, ...any) {} +func (NopLogger) Error(string, ...any) {} + +var defaultLogger Logger = NopLogger{} + +// SetDefaultLogger sets the package-level default logger. +// Commands without an explicit logger will use this. +func SetDefaultLogger(l Logger) { + if l == nil { + l = NopLogger{} + } + defaultLogger = l +} + +// DefaultLogger returns the current default logger. +func DefaultLogger() Logger { + return defaultLogger +}