diff --git a/internal/app/app.go b/internal/app/app.go index 5004e39..124b53b 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -1,6 +1,7 @@ package app import ( + "errors" "fmt" "os" "strings" @@ -45,7 +46,7 @@ func Run(context, contextFile, promptName, promptFile, engineName string, amend, } } - diff, err := commitDiff(amend, cfg, excludeFiles) + diff, filterResult, err := commitDiff(amend, cfg, excludeFiles) if err != nil { return err } @@ -80,7 +81,7 @@ func Run(context, contextFile, promptName, promptFile, engineName string, amend, output, err := eng.Generate(promptText) if err != nil { - return err + return buildEngineFailureError(err, filterResult, excludeFiles) } message := sanitizeMessage(output) if message == "" { @@ -141,7 +142,7 @@ func sanitizeMessage(message string) string { return clean } -func commitDiff(amend bool, cfg config.Config, excludeFiles []string) (string, error) { +func commitDiff(amend bool, cfg config.Config, excludeFiles []string) (string, git.Result, error) { var diff string var err error if amend { @@ -150,7 +151,7 @@ func commitDiff(amend bool, cfg config.Config, excludeFiles []string) (string, e diff, err = git.StagedDiff() } if err != nil { - return "", err + return "", git.Result{}, err } // Determine exclude patterns @@ -174,9 +175,9 @@ func commitDiff(amend bool, cfg config.Config, excludeFiles []string) (string, e result := git.Filter(diff, opts) if result.Truncated || len(result.ExcludedFiles) > 0 { - return result.Diff + formatFilterNotice(result), nil + return result.Diff + formatFilterNotice(result), result, nil } - return result.Diff, nil + return result.Diff, result, nil } func formatFilterNotice(result git.Result) string { @@ -193,6 +194,83 @@ func formatFilterNotice(result git.Result) string { return "\n\n[Filter notice: " + strings.Join(parts, "; ") + "]" } +// buildEngineFailureError converts an engine error into an actionable user +// message. If err is an *engine.EngineError, it saves the full stderr to a +// temp log file and appends an --exclude hint when the filter result contains +// truncated or pattern-excluded files. Non-EngineError values are returned +// unchanged. +func buildEngineFailureError(err error, filterResult git.Result, userExcluded []string) error { + var engineErr *engine.EngineError + if !errors.As(err, &engineErr) { + return err + } + + var msg strings.Builder + msg.WriteString(engineErr.Error()) + + logPath := writeTempLog(engineErr.Stderr) + if logPath != "" { + msg.WriteString("\nFull engine output saved to: ") + msg.WriteString(logPath) + } + + candidates := buildExcludeCandidates(filterResult, userExcluded) + if len(candidates) > 0 { + msg.WriteString("\nHint: the following files were truncated or excluded and may have caused\na context window overflow. Re-run with --exclude to skip them:") + const binary = "git ai-commit" + padding := strings.Repeat(" ", 2+len(binary)+1) + for i, f := range candidates { + if i == 0 { + msg.WriteString("\n ") + msg.WriteString(binary) + msg.WriteString(" --exclude ") + msg.WriteString(f) + } else { + msg.WriteString(" \\\n") + msg.WriteString(padding) + msg.WriteString("--exclude ") + msg.WriteString(f) + } + } + } + + return errors.New(msg.String()) +} + +// writeTempLog writes content to a new temp file and returns its path. +// Returns empty string if the file cannot be created or written. +func writeTempLog(stderr string) string { + content := stderr + if strings.TrimSpace(content) == "" { + content = "(empty)\n" + } + f, err := os.CreateTemp("", "git-ai-commit-stderr-*.log") + if err != nil { + return "" + } + defer f.Close() + _, _ = f.WriteString(content) + return f.Name() +} + +// buildExcludeCandidates returns the list of files to suggest in the +// --exclude hint: truncated files plus pattern-excluded files that the user +// has not already explicitly excluded on the current invocation. +func buildExcludeCandidates(filterResult git.Result, userExcluded []string) []string { + excluded := make(map[string]bool, len(userExcluded)) + for _, f := range userExcluded { + excluded[f] = true + } + candidates := make([]string, 0, len(filterResult.TruncatedFiles)+len(filterResult.ExcludedFiles)) + candidates = append(candidates, filterResult.TruncatedFiles...) + for _, f := range filterResult.ExcludedFiles { + if !excluded[f] { + candidates = append(candidates, f) + } + } + return candidates +} + func stageChanges(addAll bool, includeFiles []string) (func(), error) { tree, err := git.WriteIndexTree() if err != nil { diff --git a/internal/app/app_test.go b/internal/app/app_test.go index ee5d35a..eee3a7c 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -1,11 +1,121 @@ package app import ( + "errors" + "strings" "testing" "git-ai-commit/internal/config" + "git-ai-commit/internal/engine" + "git-ai-commit/internal/git" ) +func TestBuildEngineFailureErrorNonEngineError(t *testing.T) { + plain := errors.New("some other error") + result := buildEngineFailureError(plain, git.Result{}, nil) + if result != plain { + t.Fatalf("expected original error to be returned unchanged, got %v", result) + } +} + +func TestBuildEngineFailureErrorNoHint(t *testing.T) { + inner := errors.New("exit status 1") + engErr := &engine.EngineError{Err: inner, Stderr: "some stderr output"} + result := buildEngineFailureError(engErr, git.Result{}, nil) + msg := result.Error() + + if !strings.HasPrefix(msg, "engine command failed: exit status 1") { + t.Fatalf("message should start with engine error, got: %q", msg) + } + if !strings.Contains(msg, "Full engine output saved to:") { + t.Fatalf("message should contain log path, got: %q", msg) + } + if strings.Contains(msg, "Hint:") { + t.Fatalf("message should not contain hint when no truncated/excluded files, got: %q", msg) + } +} + +func TestBuildEngineFailureErrorWithTruncatedFiles(t *testing.T) { + inner := errors.New("exit status 1") + engErr := &engine.EngineError{Err: inner, Stderr: ""} + filterResult := git.Result{ + TruncatedFiles: []string{"large.txt"}, + } + result := buildEngineFailureError(engErr, filterResult, nil) + msg := result.Error() + + if !strings.Contains(msg, "Hint:") { + t.Fatalf("message should contain hint for truncated files, got: %q", msg) + } + if !strings.Contains(msg, "--exclude large.txt") { + t.Fatalf("message should contain --exclude large.txt, got: %q", msg) + } +} + +func TestBuildEngineFailureErrorWithExcludedFiles(t *testing.T) { + inner := errors.New("exit status 1") + engErr := &engine.EngineError{Err: inner, Stderr: ""} + filterResult := git.Result{ + ExcludedFiles: []string{"go.sum", "package-lock.json"}, + } + result := buildEngineFailureError(engErr, filterResult, nil) + msg := result.Error() + + if !strings.Contains(msg, "--exclude go.sum") { + t.Fatalf("message should contain --exclude go.sum, got: %q", msg) + } + if !strings.Contains(msg, "--exclude package-lock.json") { + t.Fatalf("message should contain --exclude package-lock.json, got: %q", msg) + } +} + +func TestBuildEngineFailureErrorSkipsUserExcluded(t *testing.T) { + inner := errors.New("exit status 1") + engErr := &engine.EngineError{Err: inner, Stderr: ""} + filterResult := git.Result{ + TruncatedFiles: []string{"large.txt"}, + ExcludedFiles: []string{"already-excluded.txt"}, + } + result := buildEngineFailureError(engErr, filterResult, []string{"already-excluded.txt"}) + msg := result.Error() + + if strings.Contains(msg, "--exclude already-excluded.txt") { + t.Fatalf("message should not re-list user-excluded file, got: %q", msg) + } + if !strings.Contains(msg, "--exclude large.txt") { + t.Fatalf("message should still contain truncated file hint, got: %q", msg) + } +} + +func TestBuildEngineFailureErrorEmptyStderr(t *testing.T) { + inner := errors.New("exit status 1") + engErr := &engine.EngineError{Err: inner, Stderr: ""} + result := buildEngineFailureError(engErr, git.Result{}, nil) + msg := result.Error() + + // Even with empty stderr, a log file should be created + if !strings.Contains(msg, "Full engine output saved to:") { + t.Fatalf("message should contain log path even for empty stderr, got: %q", msg) + } +} + +func TestBuildExcludeCandidates(t *testing.T) { + filterResult := git.Result{ + TruncatedFiles: []string{"big.go"}, + ExcludedFiles: []string{"go.sum", "user-excluded.txt"}, + } + candidates := buildExcludeCandidates(filterResult, []string{"user-excluded.txt"}) + if len(candidates) != 2 { + t.Fatalf("expected 2 candidates, got %d: %v", len(candidates), candidates) + } + if candidates[0] != "big.go" { + t.Fatalf("first candidate should be big.go, got %q", candidates[0]) + } + if candidates[1] != "go.sum" { + t.Fatalf("second candidate should be go.sum, got %q", candidates[1]) + } +} + func TestSelectEngineCodexDefault(t *testing.T) { cfg := config.Default() cfg.DefaultEngine = "codex" diff --git a/internal/engine/cli.go b/internal/engine/cli.go index 3706a96..9570515 100644 --- a/internal/engine/cli.go +++ b/internal/engine/cli.go @@ -8,6 +8,20 @@ import ( "strings" ) +// EngineError is returned by Generate when the engine subprocess exits with a +// non-zero status. Stderr holds the full standard error output of the engine +// process, which may be empty. +type EngineError struct { + Err error // the underlying exec error (e.g. *exec.ExitError) + Stderr string // full stderr content, possibly empty +} + +func (e *EngineError) Error() string { + return fmt.Sprintf("engine command failed: %v", e.Err) +} + +func (e *EngineError) Unwrap() error { return e.Err } + type CLI struct { Command string Args []string @@ -33,7 +47,7 @@ func (c CLI) Generate(prompt string) (string, error) { cmd.Stdout = &stdout cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - return "", fmt.Errorf("engine command failed: %v: %s", err, strings.TrimSpace(stderr.String())) + return "", &EngineError{Err: err, Stderr: stderr.String()} } return stdout.String(), nil } diff --git a/internal/engine/cli_test.go b/internal/engine/cli_test.go index cd5448f..9386e60 100644 --- a/internal/engine/cli_test.go +++ b/internal/engine/cli_test.go @@ -1,6 +1,59 @@ package engine -import "testing" +import ( + "errors" + "strings" + "testing" +) + +func TestEngineErrorMessage(t *testing.T) { + inner := errors.New("exit status 1") + e := &EngineError{Err: inner, Stderr: "some error output"} + got := e.Error() + if got != "engine command failed: exit status 1" { + t.Fatalf("Error() = %q", got) + } +} + +func TestEngineErrorMessageEmptyStderr(t *testing.T) { + inner := errors.New("exit status 1") + e := &EngineError{Err: inner, Stderr: ""} + got := e.Error() + if got != "engine command failed: exit status 1" { + t.Fatalf("Error() = %q", got) + } +} + +func TestEngineErrorUnwrap(t *testing.T) { + inner := errors.New("exit status 1") + e := &EngineError{Err: inner, Stderr: ""} + if !errors.Is(e, inner) { + t.Fatal("errors.Is should match the wrapped error") + } +} + +func TestEngineErrorDoesNotIncludeStderr(t *testing.T) { + inner := errors.New("exit status 1") + e := &EngineError{Err: inner, Stderr: "secret output that should not appear"} + if strings.Contains(e.Error(), "secret output") { + t.Fatal("Error() must not include stderr content") + } +} + +func TestCLIGenerateReturnsEngineErrorOnFailure(t *testing.T) { + cli := CLI{Command: "/bin/sh", Args: []string{"-c", "echo 'err output' >&2; exit 1"}} + _, err := cli.Generate("ignored") + if err == nil { + t.Fatal("expected error, got nil") + } + var engineErr *EngineError + if !errors.As(err, &engineErr) { + t.Fatalf("expected *EngineError, got %T: %v", err, err) + } + if !strings.Contains(engineErr.Stderr, "err output") { + t.Fatalf("Stderr = %q, want it to contain 'err output'", engineErr.Stderr) + } +} func TestCLIGenerateUsesStdin(t *testing.T) { cli := CLI{Command: "/bin/cat", Args: nil}