Refactor tool output format

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 {
+				results[idx] = Message{
+					Role: RoleTool,
+					ContentParts: []ContentPart{{
+						Type: ContentTypeText,
+						Text: err.Error(),
+					}},
+					Name:       tc.Function.Name,
+					ToolCallID: tc.ID,
+				}
+				return
+			}
+
 			c.log.InfoContext(
 				ctx,
 				"called tool",
 				slog.String("name", tc.Function.Name),
 			)
-			}
 
-			// Content cannot be empty.
-			if strings.TrimSpace(result) == "" {
-				result = `{"ok": true, "result": null}`
-			}
+			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,

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",
+	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,
 				)
 			}
-		return "", fmt.Errorf(
-			"tool %s: %w\nstderr: %s",
-			name, err, stderr.String(),
-		)
+			// 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 stdout.String(), nil
+	// No error. Process exited with status 0.
+	return &ToolResult{
+		Stdout:       stdout.String(),
+		Stderr:       stderr.String(),
+		ProcessState: cmd.ProcessState,
+	}, nil
 }

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())
+		}
+	}
+}