Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 84 additions & 6 deletions internal/app/app.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package app

import (
"errors"
"fmt"
"os"
"strings"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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 {
Expand All @@ -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()
Comment on lines +252 to +253
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check temp log writes before reporting saved output

writeTempLog unconditionally returns the temp file path even when WriteString fails, so buildEngineFailureError can tell users “Full engine output saved to …” when the log is actually empty or truncated. This is reproducible on ENOSPC or other write errors and removes the only place stderr is preserved in this flow, making failures harder to debug in exactly the environments where diagnostics are most needed.

Useful? React with 👍 / 👎.

}

// 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)
Comment on lines +266 to +268
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude hint must avoid already pattern-excluded files

buildExcludeCandidates appends filterResult.ExcludedFiles directly, but those files were already excluded by git.Filter and therefore cannot be further reduced by adding --exclude again. When engine execution fails, this produces no-op remediation steps (for example default-excluded lockfiles), which can generate long misleading hints and distract from the actionable files that are actually still in the prompt.

Useful? React with 👍 / 👎.

}
}
return candidates
}

func stageChanges(addAll bool, includeFiles []string) (func(), error) {
tree, err := git.WriteIndexTree()
if err != nil {
Expand Down
110 changes: 110 additions & 0 deletions internal/app/app_test.go
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
16 changes: 15 additions & 1 deletion internal/engine/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
55 changes: 54 additions & 1 deletion internal/engine/cli_test.go
Original file line number Diff line number Diff line change
@@ -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}
Expand Down