diff --git a/internal/provider/builtin/builtin_more_test.go b/internal/provider/builtin/builtin_more_test.go new file mode 100644 index 00000000..afd82714 --- /dev/null +++ b/internal/provider/builtin/builtin_more_test.go @@ -0,0 +1,88 @@ +package builtin + +import ( + "context" + "errors" + "testing" + + "neo-code/internal/config" + "neo-code/internal/provider" + "neo-code/internal/provider/openai" +) + +func TestRegister(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + reg *provider.Registry + wantErr string + }{ + { + name: "nil registry", + reg: nil, + wantErr: "builtin provider registry is nil", + }, + { + name: "registers openai driver", + reg: provider.NewRegistry(), + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := Register(tt.reg) + if tt.wantErr != "" { + if err == nil || err.Error() != tt.wantErr { + t.Fatalf("expected error %q, got %v", tt.wantErr, err) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !tt.reg.Supports(openai.Name) { + t.Fatalf("expected registry to support %q", openai.Name) + } + }) + } +} + +func TestNewRegistry(t *testing.T) { + t.Parallel() + + reg, err := NewRegistry() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if reg == nil { + t.Fatal("expected registry") + } + if !reg.Supports(openai.Name) { + t.Fatalf("expected builtin registry to support %q", openai.Name) + } +} + +func TestNewRegistryBuildsRegisteredDriver(t *testing.T) { + t.Parallel() + + reg, err := NewRegistry() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + _, err = reg.Build(context.Background(), config.ResolvedProviderConfig{ + ProviderConfig: config.ProviderConfig{ + Driver: openai.Name, + }, + }) + if err == nil { + t.Fatal("expected build to fail without api key or required config") + } + if errors.Is(err, provider.ErrDriverNotFound) { + t.Fatalf("expected registered driver error, got %v", err) + } +} diff --git a/internal/tools/bash/tool.go b/internal/tools/bash/tool.go index f04ea611..d8243122 100644 --- a/internal/tools/bash/tool.go +++ b/internal/tools/bash/tool.go @@ -15,10 +15,9 @@ import ( ) type Tool struct { - root string - shell string - timeout time.Duration - outputLimit int + root string + shell string + timeout time.Duration } type input struct { @@ -28,10 +27,9 @@ type input struct { func New(root string, shell string, timeout time.Duration) *Tool { return &Tool{ - root: root, - shell: shell, - timeout: timeout, - outputLimit: 16 * 1024, + root: root, + shell: shell, + timeout: timeout, } } @@ -63,15 +61,16 @@ func (t *Tool) Schema() map[string]any { func (t *Tool) Execute(ctx context.Context, call tools.ToolCallInput) (tools.ToolResult, error) { var in input if err := json.Unmarshal(call.Arguments, &in); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } if strings.TrimSpace(in.Command) == "" { - return tools.ToolResult{Name: t.Name()}, errors.New("bash: command is empty") + err := errors.New("bash: command is empty") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } workdir, err := resolveWorkdir(call.Workdir, in.Workdir) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } runCtx, cancel := context.WithTimeout(ctx, t.timeout) @@ -83,21 +82,26 @@ func (t *Tool) Execute(ctx context.Context, call tools.ToolCallInput) (tools.Too output, err := cmd.CombinedOutput() content := string(output) - if len(content) > t.outputLimit { - content = content[:t.outputLimit] + "\n...[truncated]" + if err != nil { + result := tools.NewErrorResult( + t.Name(), + tools.NormalizeErrorReason(t.Name(), err), + content, + map[string]any{"workdir": workdir}, + ) + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, err } + result := tools.ToolResult{ Name: t.Name(), Content: content, - IsError: err != nil, Metadata: map[string]any{ "workdir": workdir, }, } - if err != nil && strings.TrimSpace(result.Content) == "" { - result.Content = err.Error() - } - return result, err + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, nil } func (t *Tool) shellArgs(command string) []string { diff --git a/internal/tools/bash/tool_test.go b/internal/tools/bash/tool_test.go index 90fd6297..ad3a0402 100644 --- a/internal/tools/bash/tool_test.go +++ b/internal/tools/bash/tool_test.go @@ -88,6 +88,88 @@ func TestToolExecute(t *testing.T) { } } +func TestToolExecuteErrorFormattingAndTruncation(t *testing.T) { + workspace := t.TempDir() + tool := New(workspace, defaultShell(), 3*time.Second) + + tests := []struct { + name string + arguments []byte + expectErr string + expectContent []string + expectMetadata bool + expectTruncate bool + }{ + { + name: "invalid json arguments", + arguments: []byte(`{invalid`), + expectErr: "invalid character", + expectContent: []string{"tool error", "tool: bash", "reason: invalid arguments"}, + }, + { + name: "command failure returns formatted error", + arguments: mustMarshalArgs(t, map[string]string{"command": failingCommand(), "workdir": ""}), + expectErr: commandFailureFragment(), + expectContent: []string{ + "tool error", + "tool: bash", + "reason:", + commandFailureOutput(), + }, + expectMetadata: true, + }, + { + name: "large output is truncated", + arguments: mustMarshalArgs(t, map[string]string{"command": largeOutputCommand(), "workdir": ""}), + expectContent: []string{ + "...[truncated]", + }, + expectMetadata: true, + expectTruncate: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := tool.Execute(context.Background(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: tt.arguments, + Workdir: workspace, + }) + + if tt.expectErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for _, fragment := range tt.expectContent { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content to contain %q, got %q", fragment, result.Content) + } + } + if tt.expectMetadata && result.Metadata["workdir"] == "" { + t.Fatalf("expected workdir metadata, got %#v", result.Metadata) + } + if truncated, _ := result.Metadata["truncated"].(bool); truncated != tt.expectTruncate { + t.Fatalf("expected truncated=%v, got %#v", tt.expectTruncate, result.Metadata["truncated"]) + } + }) + } +} + +func mustMarshalArgs(t *testing.T, value any) []byte { + t.Helper() + + data, err := json.Marshal(value) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + return data +} + func safeEchoCommand() string { if goruntime.GOOS == "windows" { return "Write-Output 'hello'" @@ -102,6 +184,28 @@ func safePwdCommand() string { return "pwd" } +func failingCommand() string { + if goruntime.GOOS == "windows" { + return "Write-Output 'boom'; exit 1" + } + return "printf 'boom\\n'; exit 1" +} + +func commandFailureFragment() string { + return "exit status 1" +} + +func commandFailureOutput() string { + return "boom" +} + +func largeOutputCommand() string { + if goruntime.GOOS == "windows" { + return "$text = 'x' * 70000; Write-Output $text" + } + return "i=0; while [ $i -lt 70000 ]; do printf x; i=$((i+1)); done" +} + func defaultShell() string { if goruntime.GOOS == "windows" { return "powershell" diff --git a/internal/tools/filesystem/edit.go b/internal/tools/filesystem/edit.go index 7d831ada..f878062a 100644 --- a/internal/tools/filesystem/edit.go +++ b/internal/tools/filesystem/edit.go @@ -57,48 +57,53 @@ func (t *EditTool) Schema() map[string]any { func (t *EditTool) Execute(ctx context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { var args editInput if err := json.Unmarshal(input.Arguments, &args); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } if strings.TrimSpace(args.Path) == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(editToolName + ": path is required") + err := errors.New(editToolName + ": path is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if args.SearchString == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(editToolName + ": search_string is required") + err := errors.New(editToolName + ": search_string is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := ctx.Err(); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } root := effectiveRoot(t.root, input.Workdir) target, err := resolvePath(root, args.Path) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } data, err := os.ReadFile(target) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } content := string(data) matches := strings.Count(content, args.SearchString) switch { case matches == 0: - return tools.ToolResult{Name: t.Name()}, fmt.Errorf("%s: search_string not found in %s", editToolName, toRelativePath(root, target)) + err := fmt.Errorf("%s: search_string not found in %s", editToolName, toRelativePath(root, target)) + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err case matches > 1: - return tools.ToolResult{Name: t.Name()}, fmt.Errorf("%s: search_string matched %d locations in %s; refine it to a unique block", editToolName, matches, toRelativePath(root, target)) + err := fmt.Errorf("%s: search_string matched %d locations in %s; refine it to a unique block", editToolName, matches, toRelativePath(root, target)) + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } updated := strings.Replace(content, args.SearchString, args.ReplaceString, 1) if updated == content { - return tools.ToolResult{Name: t.Name()}, fmt.Errorf("%s: replacement produced no changes", editToolName) + err := fmt.Errorf("%s: replacement produced no changes", editToolName) + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := ctx.Err(); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := os.WriteFile(target, []byte(updated), 0o644); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } return tools.ToolResult{ diff --git a/internal/tools/filesystem/edit_test.go b/internal/tools/filesystem/edit_test.go index dac4507d..d8c499e4 100644 --- a/internal/tools/filesystem/edit_test.go +++ b/internal/tools/filesystem/edit_test.go @@ -142,3 +142,77 @@ func TestEditToolSearchStringNotFound(t *testing.T) { t.Fatalf("expected search_string not found error, got %v", execErr) } } + +func TestEditToolErrorFormattingAndContext(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + mustWriteFile(t, filepath.Join(workspace, "main.go"), "package main\n") + + tool := NewEdit(workspace) + tests := []struct { + name string + ctx func() context.Context + arguments []byte + expectErr string + expectContent []string + }{ + { + name: "invalid json arguments", + ctx: context.Background, + arguments: []byte(`{invalid`), + expectErr: "invalid character", + expectContent: []string{"tool error", "tool: filesystem_edit", "reason: invalid arguments"}, + }, + { + name: "empty search string", + ctx: context.Background, + arguments: mustMarshalFSArgs(t, map[string]string{ + "path": "main.go", + "search_string": "", + "replace_string": "new", + }), + expectErr: "search_string is required", + expectContent: []string{"tool error", "tool: filesystem_edit", "reason: search_string is required"}, + }, + { + name: "canceled context", + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return ctx + }, + arguments: mustMarshalFSArgs(t, map[string]string{ + "path": "main.go", + "search_string": "package", + "replace_string": "module", + }), + expectErr: "context canceled", + expectContent: []string{"tool error", "tool: filesystem_edit", "reason: context canceled"}, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result, err := tool.Execute(tt.ctx(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: tt.arguments, + Workdir: workspace, + }) + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + for _, fragment := range tt.expectContent { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if !result.IsError { + t.Fatalf("expected error result, got %#v", result) + } + }) + } +} diff --git a/internal/tools/filesystem/glob.go b/internal/tools/filesystem/glob.go index 41bac66a..03574e80 100644 --- a/internal/tools/filesystem/glob.go +++ b/internal/tools/filesystem/glob.go @@ -54,27 +54,28 @@ func (t *GlobTool) Schema() map[string]any { func (t *GlobTool) Execute(ctx context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { var args globInput if err := json.Unmarshal(input.Arguments, &args); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } rawPattern := strings.TrimSpace(args.Pattern) if rawPattern == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(globToolName + ": pattern is required") + err := errors.New(globToolName + ": pattern is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } pattern := normalizeSlashPath(rawPattern) if err := ctx.Err(); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } root := effectiveRoot(t.root, input.Workdir) searchRoot, err := resolveSearchDir(root, args.Dir) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } matcher, err := buildGlobMatcher(pattern) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } matches := make([]string, 0, 32) @@ -102,7 +103,7 @@ func (t *GlobTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool return nil }) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } sort.Strings(matches) @@ -117,14 +118,16 @@ func (t *GlobTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool }, nil } - return tools.ToolResult{ + result := tools.ToolResult{ Name: t.Name(), Content: strings.Join(matches, "\n"), Metadata: map[string]any{ "root": searchRoot, "count": len(matches), }, - }, nil + } + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, nil } func buildGlobMatcher(pattern string) (*regexp.Regexp, error) { diff --git a/internal/tools/filesystem/glob_test.go b/internal/tools/filesystem/glob_test.go index c4615ea9..5ac8b30d 100644 --- a/internal/tools/filesystem/glob_test.go +++ b/internal/tools/filesystem/glob_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "path/filepath" + "strconv" "strings" "testing" @@ -106,3 +107,77 @@ func TestBuildGlobMatcherRejectsInvalidUTF8(t *testing.T) { t.Fatalf("expected invalid utf-8 error, got %v", err) } } + +func TestGlobToolErrorFormattingAndTruncation(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + for i := 0; i < 1500; i++ { + mustWriteFile(t, filepath.Join(workspace, "many", strings.Repeat("a", 20), strings.Repeat("b", 20), "file"+strconv.Itoa(i)+".txt"), "x") + } + + tool := NewGlob(workspace) + tests := []struct { + name string + ctx func() context.Context + arguments []byte + expectErr string + expectContent []string + expectTruncate bool + }{ + { + name: "invalid json arguments", + ctx: context.Background, + arguments: []byte(`{invalid`), + expectErr: "invalid character", + expectContent: []string{"tool error", "tool: filesystem_glob", "reason: invalid arguments"}, + }, + { + name: "canceled context", + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return ctx + }, + arguments: mustMarshalFSArgs(t, map[string]string{"pattern": "**/*.txt"}), + expectErr: "context canceled", + expectContent: []string{"tool error", "tool: filesystem_glob", "reason: context canceled"}, + }, + { + name: "long output is truncated", + ctx: context.Background, + arguments: mustMarshalFSArgs(t, map[string]string{"pattern": "**/*.txt"}), + expectContent: []string{"...[truncated]"}, + expectTruncate: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result, err := tool.Execute(tt.ctx(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: tt.arguments, + Workdir: workspace, + }) + + if tt.expectErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for _, fragment := range tt.expectContent { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if truncated, _ := result.Metadata["truncated"].(bool); truncated != tt.expectTruncate { + t.Fatalf("expected truncated=%v, got %#v", tt.expectTruncate, result.Metadata["truncated"]) + } + }) + } +} diff --git a/internal/tools/filesystem/grep.go b/internal/tools/filesystem/grep.go index a152dfe0..cd54adec 100644 --- a/internal/tools/filesystem/grep.go +++ b/internal/tools/filesystem/grep.go @@ -63,26 +63,27 @@ func (t *GrepTool) Schema() map[string]any { func (t *GrepTool) Execute(ctx context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { var args grepInput if err := json.Unmarshal(input.Arguments, &args); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } pattern := strings.TrimSpace(args.Pattern) if pattern == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(grepToolName + ": pattern is required") + err := errors.New(grepToolName + ": pattern is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := ctx.Err(); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } root := effectiveRoot(t.root, input.Workdir) searchRoot, err := resolveSearchDir(root, args.Dir) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } matcher, err := buildGrepMatcher(pattern, args.UseRegex) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } var ( @@ -126,7 +127,7 @@ func (t *GrepTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool return nil }) if err != nil && !errors.Is(err, errGrepResultLimitReached) { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if len(results) == 0 { @@ -141,7 +142,7 @@ func (t *GrepTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool }, nil } - return tools.ToolResult{ + result := tools.ToolResult{ Name: t.Name(), Content: strings.Join(results, "\n"), Metadata: map[string]any{ @@ -149,7 +150,9 @@ func (t *GrepTool) Execute(ctx context.Context, input tools.ToolCallInput) (tool "matched_files": matchedFiles, "matched_lines": len(results), }, - }, nil + } + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + return result, nil } func buildGrepMatcher(pattern string, useRegex bool) (func(string) bool, error) { diff --git a/internal/tools/filesystem/grep_test.go b/internal/tools/filesystem/grep_test.go index 45763037..3eec737f 100644 --- a/internal/tools/filesystem/grep_test.go +++ b/internal/tools/filesystem/grep_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "path/filepath" + "strconv" "strings" "testing" @@ -105,3 +106,77 @@ func TestGrepToolExecute(t *testing.T) { }) } } + +func TestGrepToolErrorFormattingAndTruncation(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + for i := 0; i < 180; i++ { + mustWriteFile(t, filepath.Join(workspace, "bulk", "file"+strconv.Itoa(i)+".txt"), "needle "+strings.Repeat("x", 500)+"\n") + } + + tool := NewGrep(workspace) + tests := []struct { + name string + ctx func() context.Context + arguments []byte + expectErr string + expectContent []string + expectTruncate bool + }{ + { + name: "invalid json arguments", + ctx: context.Background, + arguments: []byte(`{invalid`), + expectErr: "invalid character", + expectContent: []string{"tool error", "tool: filesystem_grep", "reason: invalid arguments"}, + }, + { + name: "canceled context", + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return ctx + }, + arguments: mustMarshalFSArgs(t, map[string]any{"pattern": "needle"}), + expectErr: "context canceled", + expectContent: []string{"tool error", "tool: filesystem_grep", "reason: context canceled"}, + }, + { + name: "long result is truncated", + ctx: context.Background, + arguments: mustMarshalFSArgs(t, map[string]any{"pattern": "needle"}), + expectContent: []string{"...[truncated]"}, + expectTruncate: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + result, err := tool.Execute(tt.ctx(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: tt.arguments, + Workdir: workspace, + }) + + if tt.expectErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for _, fragment := range tt.expectContent { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if truncated, _ := result.Metadata["truncated"].(bool); truncated != tt.expectTruncate { + t.Fatalf("expected truncated=%v, got %#v", tt.expectTruncate, result.Metadata["truncated"]) + } + }) + } +} diff --git a/internal/tools/filesystem/read_file.go b/internal/tools/filesystem/read_file.go index f7902282..612501a0 100644 --- a/internal/tools/filesystem/read_file.go +++ b/internal/tools/filesystem/read_file.go @@ -49,41 +49,46 @@ func (t *ReadFileTool) Schema() map[string]any { func (t *ReadFileTool) Execute(ctx context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { var args readFileInput if err := json.Unmarshal(input.Arguments, &args); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } if strings.TrimSpace(args.Path) == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(readFileToolName + ": path is required") + err := errors.New(readFileToolName + ": path is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } base := effectiveRoot(t.root, input.Workdir) target, err := resolvePath(base, args.Path) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } data, err := os.ReadFile(target) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } - if input.EmitChunk != nil && len(data) > emitChunkSize { - for start := 0; start < len(data); start += emitChunkSize { - end := start + emitChunkSize - if end > len(data) { - end = len(data) - } - input.EmitChunk(data[start:end]) - } - } - - return tools.ToolResult{ + result := tools.ToolResult{ Name: t.Name(), Content: string(data), Metadata: map[string]any{ "path": target, }, - }, nil + } + result = tools.ApplyOutputLimit(result, tools.DefaultOutputLimitBytes) + + if input.EmitChunk != nil { + content := []byte(result.Content) + for start := 0; start < len(content); start += emitChunkSize { + end := start + emitChunkSize + if end > len(content) { + end = len(content) + } + input.EmitChunk(content[start:end]) + } + } + + return result, nil } func resolvePath(root string, requested string) (string, error) { diff --git a/internal/tools/filesystem/read_file_test.go b/internal/tools/filesystem/read_file_test.go index b325ebbf..a124272a 100644 --- a/internal/tools/filesystem/read_file_test.go +++ b/internal/tools/filesystem/read_file_test.go @@ -100,3 +100,74 @@ func TestReadFileToolExecute(t *testing.T) { }) } } + +func TestReadFileToolErrorFormattingAndTruncation(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + largeContent := strings.Repeat("abcdefghij", 7000) + largePath := filepath.Join(workspace, "large.txt") + if err := os.WriteFile(largePath, []byte(largeContent), 0o644); err != nil { + t.Fatalf("write large file: %v", err) + } + + tool := New(workspace) + tests := []struct { + name string + arguments []byte + expectErr string + expectContent []string + expectTruncate bool + }{ + { + name: "invalid json arguments", + arguments: []byte(`{invalid`), + expectErr: "invalid character", + expectContent: []string{"tool error", "tool: filesystem_read_file", "reason: invalid arguments"}, + }, + { + name: "large file is truncated", + arguments: mustMarshalFSArgs(t, map[string]string{"path": "large.txt"}), + expectContent: []string{"...[truncated]"}, + expectTruncate: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + chunks := 0 + result, err := tool.Execute(context.Background(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: tt.arguments, + Workdir: workspace, + EmitChunk: func(chunk []byte) { + if len(chunk) > 0 { + chunks++ + } + }, + }) + + if tt.expectErr != "" { + if err == nil || !strings.Contains(strings.ToLower(err.Error()), strings.ToLower(tt.expectErr)) { + t.Fatalf("expected error containing %q, got %v", tt.expectErr, err) + } + } else if err != nil { + t.Fatalf("unexpected error: %v", err) + } + for _, fragment := range tt.expectContent { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if truncated, _ := result.Metadata["truncated"].(bool); truncated != tt.expectTruncate { + t.Fatalf("expected truncated=%v, got %#v", tt.expectTruncate, result.Metadata["truncated"]) + } + if tt.expectTruncate && chunks == 0 { + t.Fatalf("expected chunk emitter to receive truncated content") + } + }) + } +} diff --git a/internal/tools/filesystem/test_helpers_test.go b/internal/tools/filesystem/test_helpers_test.go index dc96d2ca..d769bb1b 100644 --- a/internal/tools/filesystem/test_helpers_test.go +++ b/internal/tools/filesystem/test_helpers_test.go @@ -1,6 +1,7 @@ package filesystem import ( + "encoding/json" "os" "path/filepath" "testing" @@ -15,3 +16,12 @@ func mustWriteFile(t *testing.T, path string, content string) { t.Fatalf("write %s: %v", path, err) } } + +func mustMarshalFSArgs(t *testing.T, value any) []byte { + t.Helper() + data, err := json.Marshal(value) + if err != nil { + t.Fatalf("marshal args: %v", err) + } + return data +} diff --git a/internal/tools/filesystem/write_file.go b/internal/tools/filesystem/write_file.go index f5220315..bdc9bc3c 100644 --- a/internal/tools/filesystem/write_file.go +++ b/internal/tools/filesystem/write_file.go @@ -52,26 +52,27 @@ func (t *WriteFileTool) Schema() map[string]any { func (t *WriteFileTool) Execute(ctx context.Context, input tools.ToolCallInput) (tools.ToolResult, error) { var args writeFileInput if err := json.Unmarshal(input.Arguments, &args); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), "invalid arguments", err.Error(), nil), err } if strings.TrimSpace(args.Path) == "" { - return tools.ToolResult{Name: t.Name()}, errors.New(writeFileToolName + ": path is required") + err := errors.New(writeFileToolName + ": path is required") + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := ctx.Err(); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } base := effectiveRoot(t.root, input.Workdir) target, err := resolvePath(base, args.Path) if err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } if err := os.WriteFile(target, []byte(args.Content), 0o644); err != nil { - return tools.ToolResult{Name: t.Name()}, err + return tools.NewErrorResult(t.Name(), tools.NormalizeErrorReason(t.Name(), err), "", nil), err } return tools.ToolResult{ diff --git a/internal/tools/filesystem/write_file_test.go b/internal/tools/filesystem/write_file_test.go index 775062a4..91debc3a 100644 --- a/internal/tools/filesystem/write_file_test.go +++ b/internal/tools/filesystem/write_file_test.go @@ -110,3 +110,27 @@ func TestWriteFileToolMetadataAndExecute(t *testing.T) { }) } } + +func TestWriteFileToolInvalidArgumentsFormatting(t *testing.T) { + t.Parallel() + + workspace := t.TempDir() + tool := NewWrite(workspace) + + result, err := tool.Execute(context.Background(), tools.ToolCallInput{ + Name: tool.Name(), + Arguments: []byte(`{invalid`), + Workdir: workspace, + }) + if err == nil || !strings.Contains(err.Error(), "invalid character") { + t.Fatalf("expected invalid json error, got %v", err) + } + for _, fragment := range []string{"tool error", "tool: filesystem_write_file", "reason: invalid arguments"} { + if !strings.Contains(result.Content, fragment) { + t.Fatalf("expected content containing %q, got %q", fragment, result.Content) + } + } + if !result.IsError { + t.Fatalf("expected error result, got %#v", result) + } +} diff --git a/internal/tools/format.go b/internal/tools/format.go new file mode 100644 index 00000000..ae404b7e --- /dev/null +++ b/internal/tools/format.go @@ -0,0 +1,76 @@ +package tools + +import "strings" + +const ( + // DefaultOutputLimitBytes is the default max size of tool output content. + DefaultOutputLimitBytes = 64 * 1024 + truncatedSuffix = "\n...[truncated]" +) + +// ApplyOutputLimit truncates tool output content and adds a truncated metadata flag. +func ApplyOutputLimit(result ToolResult, limit int) ToolResult { + if limit <= 0 { + return result + } + if len(result.Content) <= limit { + return result + } + + result.Content = result.Content[:limit] + truncatedSuffix + if result.Metadata == nil { + result.Metadata = map[string]any{} + } + if existing, ok := result.Metadata["truncated"].(bool); !ok || !existing { + result.Metadata["truncated"] = true + } + return result +} + +// FormatError builds a consistent error payload for tool failures. +func FormatError(toolName string, reason string, details string) string { + toolName = strings.TrimSpace(toolName) + reason = strings.TrimSpace(reason) + details = strings.TrimSpace(details) + + lines := []string{"tool error"} + if toolName != "" { + lines = append(lines, "tool: "+toolName) + } + if reason != "" { + lines = append(lines, "reason: "+reason) + } + if details != "" { + lines = append(lines, "details: "+details) + } + + return strings.Join(lines, "\n") +} + +// NormalizeErrorReason strips the tool name prefix from an error message, if present. +func NormalizeErrorReason(toolName string, err error) string { + if err == nil { + return "" + } + reason := strings.TrimSpace(err.Error()) + if toolName == "" { + return reason + } + + prefix := strings.ToLower(strings.TrimSpace(toolName)) + ":" + lower := strings.ToLower(reason) + if strings.HasPrefix(lower, prefix) { + return strings.TrimSpace(reason[len(prefix):]) + } + return reason +} + +// NewErrorResult returns a standardized error ToolResult. +func NewErrorResult(toolName string, reason string, details string, metadata map[string]any) ToolResult { + return ToolResult{ + Name: toolName, + Content: FormatError(toolName, reason, details), + IsError: true, + Metadata: metadata, + } +} diff --git a/internal/tools/format_test.go b/internal/tools/format_test.go new file mode 100644 index 00000000..06f7428e --- /dev/null +++ b/internal/tools/format_test.go @@ -0,0 +1,188 @@ +package tools + +import ( + "errors" + "strings" + "testing" +) + +func TestApplyOutputLimit(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + result ToolResult + limit int + wantContent string + wantTruncated bool + wantMetadataNil bool + wantPreserveValue any + }{ + { + name: "no limit keeps content", + result: ToolResult{ + Content: "hello", + }, + limit: 0, + wantContent: "hello", + wantTruncated: false, + wantMetadataNil: true, + }, + { + name: "content within limit keeps metadata", + result: ToolResult{ + Content: "hello", + Metadata: map[string]any{"path": "a.txt"}, + }, + limit: 10, + wantContent: "hello", + wantTruncated: false, + wantMetadataNil: false, + wantPreserveValue: "a.txt", + }, + { + name: "content over limit truncates and marks metadata", + result: ToolResult{ + Content: "hello world", + }, + limit: 5, + wantContent: "hello" + truncatedSuffix, + wantTruncated: true, + wantMetadataNil: false, + }, + { + name: "existing truncated true is preserved", + result: ToolResult{ + Content: "hello world", + Metadata: map[string]any{"truncated": true}, + }, + limit: 5, + wantContent: "hello" + truncatedSuffix, + wantTruncated: true, + wantMetadataNil: false, + }, + { + name: "existing truncated false is overwritten", + result: ToolResult{ + Content: "hello world", + Metadata: map[string]any{"truncated": false}, + }, + limit: 5, + wantContent: "hello" + truncatedSuffix, + wantTruncated: true, + wantMetadataNil: false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := ApplyOutputLimit(tt.result, tt.limit) + if got.Content != tt.wantContent { + t.Fatalf("expected content %q, got %q", tt.wantContent, got.Content) + } + + if tt.wantMetadataNil { + if got.Metadata != nil { + t.Fatalf("expected nil metadata, got %#v", got.Metadata) + } + return + } + + if got.Metadata == nil { + t.Fatal("expected metadata to be initialized") + } + if truncated, _ := got.Metadata["truncated"].(bool); truncated != tt.wantTruncated { + t.Fatalf("expected truncated=%v, got %#v", tt.wantTruncated, got.Metadata["truncated"]) + } + if tt.wantPreserveValue != nil && got.Metadata["path"] != tt.wantPreserveValue { + t.Fatalf("expected path metadata %v, got %v", tt.wantPreserveValue, got.Metadata["path"]) + } + }) + } +} + +func TestFormatHelpers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + toolName string + reason string + details string + err error + wantReason string + wantBody []string + }{ + { + name: "format trims fields", + toolName: " bash ", + reason: " failed ", + details: " bad input ", + err: errors.New("bash: failed"), + wantReason: "failed", + wantBody: []string{"tool error", "tool: bash", "reason: failed", "details: bad input"}, + }, + { + name: "normalize without tool prefix keeps message", + toolName: "webfetch", + reason: "unsupported content type", + details: "", + err: errors.New("network unavailable"), + wantReason: "network unavailable", + wantBody: []string{"tool error", "tool: webfetch", "reason: unsupported content type"}, + }, + { + name: "empty fields collapse cleanly", + toolName: "", + reason: "", + details: "", + err: nil, + wantReason: "", + wantBody: []string{"tool error"}, + }, + { + name: "tool name empty keeps raw reason", + toolName: "", + reason: "boom", + details: "", + err: errors.New("bash: failed"), + wantReason: "bash: failed", + wantBody: []string{"tool error", "reason: boom"}, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + if got := NormalizeErrorReason(tt.toolName, tt.err); got != tt.wantReason { + t.Fatalf("expected reason %q, got %q", tt.wantReason, got) + } + + body := FormatError(tt.toolName, tt.reason, tt.details) + for _, fragment := range tt.wantBody { + if !strings.Contains(body, fragment) { + t.Fatalf("expected body to contain %q, got %q", fragment, body) + } + } + + result := NewErrorResult(tt.toolName, tt.reason, tt.details, map[string]any{"k": "v"}) + if !result.IsError { + t.Fatal("expected error result") + } + if result.Name != tt.toolName { + t.Fatalf("expected name %q, got %q", tt.toolName, result.Name) + } + if result.Metadata["k"] != "v" { + t.Fatalf("expected metadata to be preserved, got %#v", result.Metadata) + } + if result.Content != body { + t.Fatalf("expected content %q, got %q", body, result.Content) + } + }) + } +} diff --git a/internal/tools/registry.go b/internal/tools/registry.go index ed1906ce..c0fc57a1 100644 --- a/internal/tools/registry.go +++ b/internal/tools/registry.go @@ -60,10 +60,11 @@ func (r *Registry) ListSchemas() []provider.ToolSpec { func (r *Registry) Execute(ctx context.Context, input ToolCallInput) (ToolResult, error) { tool, err := r.Get(input.Name) if err != nil { + content := FormatError(input.Name, NormalizeErrorReason(input.Name, err), "") return ToolResult{ ToolCallID: input.ID, Name: input.Name, - Content: err.Error(), + Content: content, IsError: true, }, err } @@ -73,7 +74,7 @@ func (r *Registry) Execute(ctx context.Context, input ToolCallInput) (ToolResult if execErr != nil { result.IsError = true if strings.TrimSpace(result.Content) == "" { - result.Content = execErr.Error() + result.Content = FormatError(result.Name, NormalizeErrorReason(result.Name, execErr), "") } return result, execErr } diff --git a/internal/tools/registry_test.go b/internal/tools/registry_test.go index f573a73a..fd12f3b1 100644 --- a/internal/tools/registry_test.go +++ b/internal/tools/registry_test.go @@ -3,6 +3,7 @@ package tools import ( "context" "errors" + "strings" "testing" ) @@ -94,7 +95,7 @@ func TestRegistryExecute(t *testing.T) { Name: "missing_tool", }, expectErr: "tool: not found", - expectContent: "tool: not found", + expectContent: "tool error", expectIsError: true, }, { @@ -104,7 +105,7 @@ func TestRegistryExecute(t *testing.T) { Name: "error_tool", }, expectErr: "boom", - expectContent: "boom", + expectContent: "tool error", expectIsError: true, }, { @@ -135,8 +136,8 @@ func TestRegistryExecute(t *testing.T) { if result.ToolCallID != tt.input.ID { t.Fatalf("expected tool call id %q, got %q", tt.input.ID, result.ToolCallID) } - if result.Content != tt.expectContent { - t.Fatalf("expected content %q, got %q", tt.expectContent, result.Content) + if tt.expectContent != "" && !strings.Contains(result.Content, tt.expectContent) { + t.Fatalf("expected content containing %q, got %q", tt.expectContent, result.Content) } if result.IsError != tt.expectIsError { t.Fatalf("expected IsError=%v, got %v", tt.expectIsError, result.IsError) diff --git a/internal/tools/webfetch/tool.go b/internal/tools/webfetch/tool.go index dfc889bf..efa5474f 100644 --- a/internal/tools/webfetch/tool.go +++ b/internal/tools/webfetch/tool.go @@ -170,7 +170,7 @@ func (t *Tool) handleResponse(targetURL string, resp *http.Response) (tools.Tool content, title, err := t.extractContent(data.ContentType, body) if err != nil { - return t.newErrorResult(data, reasonUnsupportedType, ""), fmt.Errorf("webfetch: extract content: %w", err) + return t.newErrorResult(data, reasonUnsupportedType, err.Error()), fmt.Errorf("webfetch: extract content: %w", err) } data.Title = title @@ -255,9 +255,15 @@ func formatSuccess(data responseData) string { } func formatError(data responseData, reason string, details string) string { - lines := append([]string{"webfetch error"}, formatCommonLines(data)...) - lines = append(lines, "reason: "+reason) - return joinMessage(lines, details) + lines := []string{"tool error", "tool: webfetch"} + if strings.TrimSpace(reason) != "" { + lines = append(lines, "reason: "+strings.TrimSpace(reason)) + } + lines = append(lines, formatCommonLines(data)...) + if strings.TrimSpace(details) != "" { + lines = append(lines, "details: "+strings.TrimSpace(details)) + } + return strings.Join(lines, "\n") } func formatCommonLines(data responseData) []string { diff --git a/internal/tools/webfetch/tool_test.go b/internal/tools/webfetch/tool_test.go index f4c1087b..f49da6a3 100644 --- a/internal/tools/webfetch/tool_test.go +++ b/internal/tools/webfetch/tool_test.go @@ -117,7 +117,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: reasonEmptyContent, expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "content_type: text/html", "reason: " + reasonEmptyContent, }, @@ -153,7 +154,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: reasonUnsupportedType, expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "content_type: image/png", "reason: " + reasonUnsupportedType, }, @@ -167,6 +169,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: reasonUnsupportedType, expectContent: []string{ + "tool error", + "tool: webfetch", "reason: " + reasonUnsupportedType, }, expectStatus: "200 OK", @@ -195,7 +199,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: "unexpected HTTP status 502 Bad Gateway", expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "status: 502 Bad Gateway", "content_type: text/plain", "reason: unexpected HTTP status 502 Bad Gateway", @@ -211,7 +216,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: "url must start with http:// or https://", expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "reason: " + reasonInvalidURL, }, expectIsError: true, @@ -222,7 +228,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: "JSON input", expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "reason: " + reasonInvalidArguments, }, expectIsError: true, @@ -233,7 +240,8 @@ func TestToolExecute(t *testing.T) { toolConfig: defaultConfig, expectErr: "webfetch: fetch", expectContent: []string{ - "webfetch error", + "tool error", + "tool: webfetch", "reason: " + reasonRequestFailed, }, expectIsError: true,