diff --git a/internal/llm/client_tools.go b/internal/llm/client_tools.go index 2f5ac15..5191ea2 100644 --- a/internal/llm/client_tools.go +++ b/internal/llm/client_tools.go @@ -2,11 +2,10 @@ package llm import ( "context" - "fmt" - "log/slog" "runtime" - "strings" "sync" + + "log/slog" ) // callTools executes each tool call in the message concurrently and returns @@ -41,26 +40,31 @@ func (c *Client) callTools(ctx context.Context, msg Message) []Message { slog.Any("error", err), slog.String("name", tc.Function.Name), ) - result = fmt.Sprintf( - `{"ok": false, "error": %q}`, err, - ) - } else { - c.log.InfoContext( - ctx, - "called tool", - slog.String("name", tc.Function.Name), - ) + results[idx] = Message{ + Role: RoleTool, + ContentParts: []ContentPart{{ + Type: ContentTypeText, + Text: err.Error(), + }}, + Name: tc.Function.Name, + ToolCallID: tc.ID, + } + return } - // Content cannot be empty. - if strings.TrimSpace(result) == "" { - result = `{"ok": true, "result": null}` - } + c.log.InfoContext( + ctx, + "called tool", + slog.String("name", tc.Function.Name), + ) + + formatted := result.FormatResult() results[idx] = Message{ Role: RoleTool, ContentParts: []ContentPart{{ - Type: ContentTypeText, Text: result, + Type: ContentTypeText, + Text: formatted, }}, Name: tc.Function.Name, ToolCallID: tc.ID, diff --git a/internal/llm/tool.go b/internal/llm/tool.go index 33a2cd5..b5bd8f5 100644 --- a/internal/llm/tool.go +++ b/internal/llm/tool.go @@ -5,7 +5,9 @@ import ( "context" "encoding/json" "fmt" + "os" "os/exec" + "strings" "text/template" "time" @@ -173,22 +175,68 @@ func (r *Registry) ToAPI() []APITool { return tools } +// ToolResult holds the structured output from a tool execution. +type ToolResult struct { + Stdout string + Stderr string + ProcessState *os.ProcessState +} + +// FormatResult formats the tool result using the structured label-based +// format with stdout, stderr, and exit code sections. +// FormatResult formats the tool result using the structured label-based +// format with stdout, stderr, and exit code sections. +func (r *ToolResult) FormatResult() string { + var buf strings.Builder + + fmt.Fprintln(&buf, "▸ stdout:") + if r.Stdout != "" { + fmt.Fprint(&buf, r.Stdout) + if !strings.HasSuffix(r.Stdout, "\n") { + fmt.Fprint(&buf, "\n") + } + } + + fmt.Fprintln(&buf, "▸ stderr:") + if r.Stderr != "" { + fmt.Fprint(&buf, r.Stderr) + if !strings.HasSuffix(r.Stderr, "\n") { + fmt.Fprint(&buf, "\n") + } + } + + exitCode := r.ProcessState.ExitCode() + fmt.Fprintf(&buf, "┗ exit: %d", exitCode) + // Include signal info when the process was killed by a signal. + if exitCode == -1 { + fmt.Fprintf(&buf, " (%s)", r.ProcessState.String()) + } + fmt.Fprintln(&buf) + + return buf.String() +} + // Execute runs a tool by name with the provided JSON arguments. // It expands argument templates, executes the command as a subprocess, and -// returns stdout on success. The context can be used for cancellation; -// tool-specific timeouts are applied on top of any context deadline. +// returns a structured result with stdout, stderr, and exit code. +// The context can be used for cancellation; tool-specific timeouts are +// applied on top of any context deadline. +// +// Start failures (process never ran) return an error directly. func (r *Registry) Execute( ctx context.Context, name string, args string, -) (string, error) { +) (*ToolResult, error) { tool, ok := r.tools[name] if !ok { - return "", fmt.Errorf("unknown tool: %s", name) + return nil, fmt.Errorf("unknown tool: %q", name) } // Evaluate argument templates. cmdArgs, err := tool.ParseArguments(args) if err != nil { - return "", fmt.Errorf("parse arguments: %w", err) + return nil, fmt.Errorf( + "tool %q: parse arguments: %w", name, err, + ) } // If defined, use the timeout. @@ -207,18 +255,63 @@ func (r *Registry) Execute( ) cmd.Stdout = &stdout cmd.Stderr = &stderr - if err := cmd.Run(); err != nil { - if ctx.Err() == context.DeadlineExceeded && tool.timeout > 0 { - return "", fmt.Errorf( - "tool %s timed out after %v", - name, tool.timeout, - ) + err = cmd.Run() + + // Extract exit code and handle errors. + // After cmd.Run() returns, the possible error cases are: + // + // 1. Context error (ctx.Err() != nil): the context was cancelled or + // timed out. If the process had started, it was killed and + // ProcessState is set. If it had not, Process is nil. + // + // 2. Start failure (cmd.Process == nil): the executable was not found, + // the fork failed, or the context was already done before Start. + // No stdout/stderr to capture. + // + // 3. Process exited with error (err is *exec.ExitError): the process + // ran but exited with a non-zero status or was killed by a signal. + // ProcessState is set and contains the exit code. + // + // In all cases where cmd.Process != nil, ProcessState is guaranteed + // to be set because Start() succeeded. + if err != nil { + switch { + // Context was cancelled or timed out. The process was killed if + // it had started. Distinguish tool timeout from other context + // errors so we can return an appropriate message. + case ctx.Err() != nil: + if ctx.Err() == context.DeadlineExceeded && + tool.timeout > 0 { + // Tool-specific timeout. Return a clear + // message with the configured duration. + return nil, fmt.Errorf( + "tool %q: timed out after %v", + name, tool.timeout, + ) + } + // Context cancelled or parent deadline exceeded. + return nil, fmt.Errorf("tool %q: %w", name, ctx.Err()) + + // Process never started. No ProcessState, no output. + // The error is the raw start failure (e.g., *PathError). + case cmd.Process == nil: + return nil, fmt.Errorf("tool %q: %w", name, err) + + // Process ran and exited with non-zero status or was killed by a signal. + // ProcessState is set; return it along with captured output. + default: + return &ToolResult{ + Stdout: stdout.String(), + Stderr: stderr.String(), + ProcessState: cmd.ProcessState, + }, nil } - return "", fmt.Errorf( - "tool %s: %w\nstderr: %s", - name, err, stderr.String(), - ) } - return stdout.String(), nil + // No error. Process exited with status 0. + return &ToolResult{ + Stdout: stdout.String(), + Stderr: stderr.String(), + ProcessState: cmd.ProcessState, + }, nil } diff --git a/internal/llm/tool_test.go b/internal/llm/tool_test.go index 8b16b21..a5d6a72 100644 --- a/internal/llm/tool_test.go +++ b/internal/llm/tool_test.go @@ -1,6 +1,9 @@ package llm import ( + "context" + "os/exec" + "strings" "testing" "time" @@ -201,3 +204,155 @@ func TestTool_ToAPI(t *testing.T) { t.Error("parameters is nil") } } + +func TestToolResult_FormatResult(t *testing.T) { + // Get real ProcessState values from actual commands. + cmdOK := exec.Command("sh", "-c", "exit 0") + if err := cmdOK.Run(); err != nil { + t.Fatalf("cmdOK: %v", err) + } + cmdFail := exec.Command("sh", "-c", "exit 3") + cmdFail.Run() // expect non-nil error + + tests := []struct { + name string + result ToolResult + wantContains []string + }{ + { + name: "success with stdout", + result: ToolResult{Stdout: "hello\n", Stderr: "", ProcessState: cmdOK.ProcessState}, + wantContains: []string{"▸ stdout:", "hello", "▸ stderr:", "┗ exit: 0"}, + }, + { + name: "failure with stderr", + result: ToolResult{Stdout: "", Stderr: "error occurred\n", ProcessState: cmdFail.ProcessState}, + wantContains: []string{"▸ stdout:", "▸ stderr:", "error occurred", "┗ exit: 3"}, + }, + { + name: "both stdout and stderr", + result: ToolResult{Stdout: "output", Stderr: "warning", ProcessState: cmdOK.ProcessState}, + wantContains: []string{"▸ stdout:", "output", "▸ stderr:", "warning", "┗ exit: 0"}, + }, + { + name: "empty output", + result: ToolResult{Stdout: "", Stderr: "", ProcessState: cmdOK.ProcessState}, + wantContains: []string{"▸ stdout:", "▸ stderr:", "┗ exit: 0"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.result.FormatResult() + for _, want := range tt.wantContains { + if !strings.Contains(got, want) { + t.Errorf("FormatResult() = %q, want to contain %q", got, want) + } + } + }) + } +} + +func TestRegistry_Execute_Success(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{ + {Name: "echo", Description: "echo", Command: "echo", Arguments: []string{"hello"}}, + }) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + result, err := reg.Execute(context.Background(), "echo", "{}") + if err != nil { + t.Fatalf("Execute: %v", err) + } + if result.ProcessState.ExitCode() != 0 { + t.Errorf("ExitCode = %d, want 0", result.ProcessState.ExitCode()) + } + if !strings.Contains(result.Stdout, "hello") { + t.Errorf("Stdout = %q, want to contain hello", result.Stdout) + } +} + +func TestRegistry_Execute_NonZeroExit(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{ + {Name: "fail", Description: "fail", Command: "sh", Arguments: []string{"-c", "exit 42"}}, + }) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + result, err := reg.Execute(context.Background(), "fail", "{}") + if err != nil { + t.Fatalf("Execute: %v", err) + } + if result.ProcessState.ExitCode() != 42 { + t.Errorf("ExitCode = %d, want 42", result.ProcessState.ExitCode()) + } +} + +func TestRegistry_Execute_UnknownCommand(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{ + {Name: "nope", Description: "nope", Command: "nonexistent_binary_xyz", Arguments: []string{}}, + }) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + _, err = reg.Execute(context.Background(), "nope", "{}") + if err == nil { + t.Fatal("Execute: want error, got nil") + } +} + +func TestRegistry_Execute_UnknownTool(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{}) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + _, err = reg.Execute(context.Background(), "missing", "{}") + if err == nil { + t.Fatal("Execute: want error, got nil") + } + if !strings.Contains(err.Error(), "unknown tool") { + t.Errorf("err = %v, want unknown tool error", err) + } +} + +func TestRegistry_Execute_Timeout(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{ + {Name: "sleep", Description: "sleep", Command: "sleep", Arguments: []string{"10"}, Timeout: "50ms"}, + }) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + _, err = reg.Execute(context.Background(), "sleep", "{}") + if err == nil { + t.Fatal("Execute: want timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out") { + t.Errorf("err = %v, want timed out error", err) + } +} + +func TestRegistry_Execute_SignalKilled(t *testing.T) { + reg, err := NewRegistry([]config.ToolConfig{ + {Name: "kill", Description: "kill", Command: "sh", Arguments: []string{"-c", "sleep 100 & kill -9 $!; wait"}}, + }) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + + result, err := reg.Execute(context.Background(), "kill", "{}") + if err != nil { + t.Fatalf("Execute: %v", err) + } + // The process we kill exits with signal, but the shell itself may exit 0. + // On some systems the exit code is 137 (128+9). Check ProcessState for signal info. + if result.ProcessState.ExitCode() == -1 { + if !strings.Contains(result.ProcessState.String(), "signal:") { + t.Errorf("ProcessState.String() = %q, want signal info", result.ProcessState.String()) + } + } +}