diff --git a/README.md b/README.md index 3b3b3ce..e4b5ba3 100644 --- a/README.md +++ b/README.md @@ -219,6 +219,7 @@ provider implementation entirely (via a Go build constraint), so selecting `apfel`/`--apfel` on Linux or Windows fails immediately with a clear error instead of trying (and failing) to exec a nonexistent `apfel` binary. +- Failed commands also include `apfel`'s stderr output in the error message. - Does not support `--list-models` (it has no concept of a model catalog — it just talks to whatever on-device model `apfel` provides); using `--list-models --apfel`/`--list-models --provider apfel` is an error. @@ -237,6 +238,7 @@ error. - `--model` selects the `fm` model when supplied; `fm` currently provides the `system` model. +- Failed commands also include `fm`'s stderr output in the error message. - Does not support `--list-models`; using `--list-models --fm`/`--list-models --provider fm` is an error. diff --git a/go.mod b/go.mod index c598899..cc3b918 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,3 @@ module github.com/evilmarty/lazycommit -go 1.27.0 +go 1.27.3 diff --git a/provider/apfel.go b/provider/apfel.go index 4a71842..79799f1 100644 --- a/provider/apfel.go +++ b/provider/apfel.go @@ -9,7 +9,6 @@ package provider import ( "context" - "fmt" "strconv" "strings" @@ -51,7 +50,7 @@ func (p *ApfelProvider) Generate(_ context.Context, prompt string) (string, erro out, err := p.runner()("apfel", args) if err != nil { - return "", fmt.Errorf("local generation with apfel failed: %w", err) + return "", localGenerationError("apfel", err) } return strings.TrimSpace(string(out)), nil diff --git a/provider/apfel_test.go b/provider/apfel_test.go index 5f00ffb..e661de4 100644 --- a/provider/apfel_test.go +++ b/provider/apfel_test.go @@ -5,6 +5,8 @@ package provider import ( "context" "errors" + "fmt" + "os/exec" "strings" "testing" ) @@ -56,13 +58,57 @@ func TestApfelProviderDefaultMaxTokens(t *testing.T) { } func TestApfelProviderRunnerError(t *testing.T) { + runErr := errors.New("apfel not found") p := &ApfelProvider{ Runner: func(name string, args []string) ([]byte, error) { - return nil, errors.New("apfel not found") + return nil, runErr }, } - if _, err := p.Generate(context.Background(), "p"); err == nil { - t.Fatal("expected error") + if _, err := p.Generate(context.Background(), "p"); !errors.Is(err, runErr) { + t.Fatalf("expected wrapped runner error, got %v", err) + } +} + +func TestApfelProviderExitError(t *testing.T) { + for _, tt := range []struct { + name string + stderr string + wrapped bool + }{ + {name: "stderr", stderr: " Model unavailable\n"}, + {name: "wrapped stderr", stderr: "Model unavailable", wrapped: true}, + {name: "empty stderr"}, + {name: "whitespace stderr", stderr: " \n"}, + } { + t.Run(tt.name, func(t *testing.T) { + _, runErr := exec.Command("sh", "-c", `printf '%s' "$1" >&2; exit 1`, + "sh", tt.stderr).Output() + var exitErr *exec.ExitError + if !errors.As(runErr, &exitErr) { + t.Fatalf("expected exit error, got %v", runErr) + } + if tt.wrapped { + runErr = fmt.Errorf("command failed: %w", runErr) + } + p := &ApfelProvider{ + Runner: func(string, []string) ([]byte, error) { + return []byte("partial output"), runErr + }, + } + + got, err := p.Generate(context.Background(), "p") + if got != "" || !errors.Is(err, runErr) { + t.Fatalf("Generate() = %q, %v; want empty output and wrapped runner error", got, err) + } + want := "local generation with apfel failed" + if stderr := strings.TrimSpace(tt.stderr); stderr != "" { + want += ": " + stderr + } + want += ": " + runErr.Error() + if err.Error() != want { + t.Errorf("error = %q, want %q", err.Error(), want) + } + }) } } diff --git a/provider/fm.go b/provider/fm.go index b25e9fd..11f0857 100644 --- a/provider/fm.go +++ b/provider/fm.go @@ -4,7 +4,6 @@ package provider import ( "context" - "fmt" "strings" "github.com/evilmarty/lazycommit/internal/cmdrunner" @@ -36,7 +35,7 @@ func (p *FoundationModelProvider) Generate(_ context.Context, prompt string) (st out, err := p.runner()("fm", args) if err != nil { - return "", fmt.Errorf("local generation with fm failed: %w", err) + return "", localGenerationError("fm", err) } return strings.TrimSpace(string(out)), nil diff --git a/provider/fm_test.go b/provider/fm_test.go index 2f7aa9a..87dce01 100644 --- a/provider/fm_test.go +++ b/provider/fm_test.go @@ -5,7 +5,10 @@ package provider import ( "context" "errors" + "fmt" + "os/exec" "reflect" + "strings" "testing" ) @@ -54,14 +57,63 @@ func TestFoundationModelProviderGenerateWithoutModel(t *testing.T) { } func TestFoundationModelProviderRunnerError(t *testing.T) { + runErr := errors.New("fm unavailable") p := &FoundationModelProvider{ Runner: func(name string, args []string) ([]byte, error) { - return nil, errors.New("fm unavailable") + return nil, runErr }, } - if _, err := p.Generate(context.Background(), "p"); err == nil { - t.Fatal("expected error") + if _, err := p.Generate(context.Background(), "p"); !errors.Is(err, runErr) { + t.Fatalf("expected wrapped runner error, got %v", err) + } +} + +func TestFoundationModelProviderExitStatus(t *testing.T) { + for _, tt := range []struct { + name string + status int + wrapped bool + stderr string + }{ + {name: "license required", status: 69}, + {name: "wrapped license error", status: 69, wrapped: true}, + {name: "license stderr", status: 69, stderr: "Please run sudo fm license"}, + {name: "wrapped stderr", status: 69, wrapped: true, stderr: "Please run sudo fm license"}, + {name: "other failure", status: 1}, + {name: "other failure stderr", status: 1, stderr: "Model unavailable"}, + {name: "whitespace stderr", status: 1, stderr: " \n"}, + } { + t.Run(tt.name, func(t *testing.T) { + _, runErr := exec.Command("sh", "-c", `printf '%s' "$1" >&2; exit "$2"`, + "sh", tt.stderr, fmt.Sprint(tt.status)).Output() + var exitErr *exec.ExitError + if !errors.As(runErr, &exitErr) || exitErr.ExitCode() != tt.status { + t.Fatalf("expected exit status %d, got %v", tt.status, runErr) + } + if tt.wrapped { + runErr = fmt.Errorf("command failed: %w", runErr) + } + p := &FoundationModelProvider{ + Runner: func(string, []string) ([]byte, error) { + return []byte("partial output"), runErr + }, + } + + got, err := p.Generate(context.Background(), "p") + if got != "" || !errors.Is(err, runErr) { + t.Fatalf("Generate() = %q, %v; want empty output and wrapped runner error", got, err) + } + if stderr := strings.TrimSpace(tt.stderr); stderr != "" { + if !strings.Contains(err.Error(), ": "+stderr+": ") { + t.Errorf("missing stderr output: %v", err) + } + } else { + if want := "local generation with fm failed: " + runErr.Error(); err.Error() != want { + t.Errorf("error = %q, want %q", err.Error(), want) + } + } + }) } } diff --git a/provider/local_darwin.go b/provider/local_darwin.go new file mode 100644 index 0000000..28adeff --- /dev/null +++ b/provider/local_darwin.go @@ -0,0 +1,21 @@ +//go:build darwin + +package provider + +import ( + "errors" + "fmt" + "os/exec" + "strings" +) + +func localGenerationError(name string, err error) error { + message := "local generation with " + name + " failed" + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + if stderr := strings.TrimSpace(string(exitErr.Stderr)); stderr != "" { + message += ": " + stderr + } + } + return fmt.Errorf("%s: %w", message, err) +}