claudetool: replace timeout parameter with slow_ok boolean
Empirically, the agent doesn't set timeouts long enough,
and doesn't retry on failure.
Give it only one decision to make: Is this maybe a slow command?
If, horror of horrors, your project can't accomplish tasks within the
default timeouts, there's a new command line flag to adjust them.
Co-Authored-By: sketch <hello@sketch.dev>
Change-ID: sc26e3516f28c22d4k
diff --git a/claudetool/bash.go b/claudetool/bash.go
index a62892f..59b8e3c 100644
--- a/claudetool/bash.go
+++ b/claudetool/bash.go
@@ -23,44 +23,77 @@
// PermissionCallback is a function type for checking if a command is allowed to run
type PermissionCallback func(command string) error
-// BashTool is a struct for executing shell commands with bash -c and optional timeout
+// BashTool specifies a llm.Tool for executing shell commands.
type BashTool struct {
// CheckPermission is called before running any command, if set
CheckPermission PermissionCallback
// EnableJITInstall enables just-in-time tool installation for missing commands
EnableJITInstall bool
+ // Timeouts holds the configurable timeout values (uses defaults if nil)
+ Timeouts *Timeouts
}
const (
EnableBashToolJITInstall = true
NoBashToolJITInstall = false
+
+ DefaultFastTimeout = 30 * time.Second
+ DefaultSlowTimeout = 15 * time.Minute
+ DefaultBackgroundTimeout = 24 * time.Hour
)
-// NewBashTool creates a new Bash tool with optional permission callback
-func NewBashTool(checkPermission PermissionCallback, enableJITInstall bool) *llm.Tool {
- tool := &BashTool{
- CheckPermission: checkPermission,
- EnableJITInstall: enableJITInstall,
- }
+// Timeouts holds the configurable timeout values for bash commands.
+type Timeouts struct {
+ Fast time.Duration // regular commands (e.g., ls, echo, simple scripts)
+ Slow time.Duration // commands that may reasonably take longer (e.g., downloads, builds, tests)
+ Background time.Duration // background commands (e.g., servers, long-running processes)
+}
+// Fast returns t's fast timeout, or DefaultFastTimeout if t is nil.
+func (t *Timeouts) fast() time.Duration {
+ if t == nil {
+ return DefaultFastTimeout
+ }
+ return t.Fast
+}
+
+// Slow returns t's slow timeout, or DefaultSlowTimeout if t is nil.
+func (t *Timeouts) slow() time.Duration {
+ if t == nil {
+ return DefaultSlowTimeout
+ }
+ return t.Slow
+}
+
+// Background returns t's background timeout, or DefaultBackgroundTimeout if t is nil.
+func (t *Timeouts) background() time.Duration {
+ if t == nil {
+ return DefaultBackgroundTimeout
+ }
+ return t.Background
+}
+
+// Tool returns an llm.Tool based on b.
+func (b *BashTool) Tool() *llm.Tool {
return &llm.Tool{
Name: bashName,
Description: strings.TrimSpace(bashDescription),
InputSchema: llm.MustSchema(bashInputSchema),
- Run: tool.Run,
+ Run: b.Run,
}
}
-// The Bash tool executes shell commands with bash -c and optional timeout
-var Bash = NewBashTool(nil, NoBashToolJITInstall)
-
const (
bashName = "bash"
bashDescription = `
-Executes a shell command using bash -c with an optional timeout, returning combined stdout and stderr.
-When run with background flag, the process may keep running after the tool call returns, and
-the agent can inspect the output by reading the output files. Use the background task when, for example,
-starting a server to test something. Be sure to kill the process group when done.
+Executes shell commands via bash -c, returning combined stdout/stderr.
+
+With background=true, returns immediately while process continues running
+with output redirected to files. Kill process group when done.
+Use background for servers/demos that need to stay running.
+
+MUST set slow_ok=true for potentially slow commands: builds, downloads,
+installs, tests, or any other substantive operation.
`
// If you modify this, update the termui template for prettier rendering.
bashInputSchema = `
@@ -70,15 +103,15 @@
"properties": {
"command": {
"type": "string",
- "description": "Shell script to execute"
+ "description": "Shell to execute"
},
- "timeout": {
- "type": "string",
- "description": "Timeout as a Go duration string, defaults to 10s if background is false; 10m if background is true"
+ "slow_ok": {
+ "type": "boolean",
+ "description": "Use extended timeout"
},
"background": {
"type": "boolean",
- "description": "If true, executes the command in the background without waiting for completion"
+ "description": "Execute in background"
}
}
}
@@ -87,7 +120,7 @@
type bashInput struct {
Command string `json:"command"`
- Timeout string `json:"timeout,omitempty"`
+ SlowOK bool `json:"slow_ok,omitempty"`
Background bool `json:"background,omitempty"`
}
@@ -97,19 +130,14 @@
StderrFile string `json:"stderr_file"`
}
-func (i *bashInput) timeout() time.Duration {
- if i.Timeout != "" {
- dur, err := time.ParseDuration(i.Timeout)
- if err == nil {
- return dur
- }
- }
-
- // Otherwise, use different defaults based on background mode
- if i.Background {
- return 10 * time.Minute
- } else {
- return 10 * time.Second
+func (i *bashInput) timeout(t *Timeouts) time.Duration {
+ switch {
+ case i.Background:
+ return t.background()
+ case i.SlowOK:
+ return t.slow()
+ default:
+ return t.fast()
}
}
@@ -140,9 +168,11 @@
}
}
+ timeout := req.timeout(b.Timeouts)
+
// If Background is set to true, use executeBackgroundBash
if req.Background {
- result, err := executeBackgroundBash(ctx, req)
+ result, err := executeBackgroundBash(ctx, req, timeout)
if err != nil {
return nil, err
}
@@ -156,7 +186,7 @@
}
// For foreground commands, use executeBash
- out, execErr := executeBash(ctx, req)
+ out, execErr := executeBash(ctx, req, timeout)
if execErr != nil {
return nil, execErr
}
@@ -165,8 +195,8 @@
const maxBashOutputLength = 131072
-func executeBash(ctx context.Context, req bashInput) (string, error) {
- execCtx, cancel := context.WithTimeout(ctx, req.timeout())
+func executeBash(ctx context.Context, req bashInput, timeout time.Duration) (string, error) {
+ execCtx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
// Can't do the simple thing and call CombinedOutput because of the need to kill the process group.
@@ -218,7 +248,7 @@
if len(partialOutput) > maxBashOutputLength {
partialOutput = partialOutput[:maxBashOutputLength] + "\n[output truncated due to size]\n"
}
- return "", fmt.Errorf("command timed out after %s\nCommand output (until it timed out):\n%s", req.timeout(), outstr)
+ return "", fmt.Errorf("command timed out after %s\nCommand output (until it timed out):\n%s", timeout, outstr)
}
if err != nil {
return "", fmt.Errorf("command failed: %w\n%s", err, outstr)
@@ -246,7 +276,7 @@
}
// executeBackgroundBash executes a command in the background and returns the pid and output file locations
-func executeBackgroundBash(ctx context.Context, req bashInput) (*BackgroundResult, error) {
+func executeBackgroundBash(ctx context.Context, req bashInput, timeout time.Duration) (*BackgroundResult, error) {
// Create temporary directory for output files
tmpDir, err := os.MkdirTemp("", "sketch-bg-")
if err != nil {
@@ -296,7 +326,6 @@
// Set up timeout handling if a timeout was specified
pid := cmd.Process.Pid
- timeout := req.timeout()
if timeout > 0 {
// Launch a goroutine that will kill the process after the timeout
go func() {
@@ -377,7 +406,7 @@
}
subConvo := info.Convo.SubConvo()
subConvo.Hidden = true
- subBash := NewBashTool(nil, NoBashToolJITInstall)
+ subBash := &BashTool{EnableJITInstall: NoBashToolJITInstall}
done := false
doneTool := &llm.Tool{
@@ -428,7 +457,7 @@
}
subConvo.Tools = []*llm.Tool{
- subBash,
+ subBash.Tool(),
doneTool,
}
diff --git a/claudetool/bash_test.go b/claudetool/bash_test.go
index 6904447..98410d5 100644
--- a/claudetool/bash_test.go
+++ b/claudetool/bash_test.go
@@ -11,12 +11,60 @@
"time"
)
+func TestBashSlowOk(t *testing.T) {
+ // Test that slow_ok flag is properly handled
+ t.Run("SlowOk Flag", func(t *testing.T) {
+ input := json.RawMessage(`{"command":"echo 'slow test'","slow_ok":true}`)
+
+ bashTool := (&BashTool{}).Tool()
+ result, err := bashTool.Run(context.Background(), input)
+ if err != nil {
+ t.Fatalf("Unexpected error: %v", err)
+ }
+
+ expected := "slow test\n"
+ if len(result) == 0 || result[0].Text != expected {
+ t.Errorf("Expected %q, got %q", expected, result[0].Text)
+ }
+ })
+
+ // Test that slow_ok with background works
+ t.Run("SlowOk with Background", func(t *testing.T) {
+ input := json.RawMessage(`{"command":"echo 'slow background test'","slow_ok":true,"background":true}`)
+
+ bashTool := (&BashTool{}).Tool()
+ result, err := bashTool.Run(context.Background(), input)
+ if err != nil {
+ t.Fatalf("Unexpected error: %v", err)
+ }
+
+ // Should return background result JSON
+ var bgResult BackgroundResult
+ resultStr := result[0].Text
+ if err := json.Unmarshal([]byte(resultStr), &bgResult); err != nil {
+ t.Fatalf("Failed to unmarshal background result: %v", err)
+ }
+
+ if bgResult.PID <= 0 {
+ t.Errorf("Invalid PID returned: %d", bgResult.PID)
+ }
+
+ // Clean up
+ os.Remove(bgResult.StdoutFile)
+ os.Remove(bgResult.StderrFile)
+ os.Remove(filepath.Dir(bgResult.StdoutFile))
+ })
+}
+
func TestBashTool(t *testing.T) {
+ var bashTool BashTool
+ tool := bashTool.Tool()
+
// Test basic functionality
t.Run("Basic Command", func(t *testing.T) {
input := json.RawMessage(`{"command":"echo 'Hello, world!'"}`)
- result, err := Bash.Run(context.Background(), input)
+ result, err := tool.Run(context.Background(), input)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -31,7 +79,7 @@
t.Run("Command With Arguments", func(t *testing.T) {
input := json.RawMessage(`{"command":"echo -n foo && echo -n bar"}`)
- result, err := Bash.Run(context.Background(), input)
+ result, err := tool.Run(context.Background(), input)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -42,21 +90,21 @@
}
})
- // Test with timeout parameter
- t.Run("With Timeout", func(t *testing.T) {
+ // Test with slow_ok parameter
+ t.Run("With SlowOK", func(t *testing.T) {
inputObj := struct {
Command string `json:"command"`
- Timeout string `json:"timeout"`
+ SlowOK bool `json:"slow_ok"`
}{
Command: "sleep 0.1 && echo 'Completed'",
- Timeout: "5s",
+ SlowOK: true,
}
inputJSON, err := json.Marshal(inputObj)
if err != nil {
t.Fatalf("Failed to marshal input: %v", err)
}
- result, err := Bash.Run(context.Background(), inputJSON)
+ result, err := tool.Run(context.Background(), inputJSON)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -67,21 +115,22 @@
}
})
- // Test command timeout
+ // Test command timeout with custom timeout config
t.Run("Command Timeout", func(t *testing.T) {
- inputObj := struct {
- Command string `json:"command"`
- Timeout string `json:"timeout"`
- }{
- Command: "sleep 0.5 && echo 'Should not see this'",
- Timeout: "100ms",
+ // Use a custom BashTool with very short timeout
+ customTimeouts := &Timeouts{
+ Fast: 100 * time.Millisecond,
+ Slow: 100 * time.Millisecond,
+ Background: 100 * time.Millisecond,
}
- inputJSON, err := json.Marshal(inputObj)
- if err != nil {
- t.Fatalf("Failed to marshal input: %v", err)
+ customBash := &BashTool{
+ Timeouts: customTimeouts,
}
+ tool := customBash.Tool()
- _, err = Bash.Run(context.Background(), inputJSON)
+ input := json.RawMessage(`{"command":"sleep 0.5 && echo 'Should not see this'"}`)
+
+ _, err := tool.Run(context.Background(), input)
if err == nil {
t.Errorf("Expected timeout error, got none")
} else if !strings.Contains(err.Error(), "timed out") {
@@ -93,7 +142,7 @@
t.Run("Failed Command", func(t *testing.T) {
input := json.RawMessage(`{"command":"exit 1"}`)
- _, err := Bash.Run(context.Background(), input)
+ _, err := tool.Run(context.Background(), input)
if err == nil {
t.Errorf("Expected error for failed command, got none")
}
@@ -103,7 +152,7 @@
t.Run("Invalid JSON Input", func(t *testing.T) {
input := json.RawMessage(`{"command":123}`) // Invalid JSON (command must be string)
- _, err := Bash.Run(context.Background(), input)
+ _, err := tool.Run(context.Background(), input)
if err == nil {
t.Errorf("Expected error for invalid input, got none")
}
@@ -117,10 +166,9 @@
t.Run("Successful Command", func(t *testing.T) {
req := bashInput{
Command: "echo 'Success'",
- Timeout: "5s",
}
- output, err := executeBash(ctx, req)
+ output, err := executeBash(ctx, req, 5*time.Second)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -135,10 +183,9 @@
t.Run("SKETCH Environment Variable", func(t *testing.T) {
req := bashInput{
Command: "echo $SKETCH",
- Timeout: "5s",
}
- output, err := executeBash(ctx, req)
+ output, err := executeBash(ctx, req, 5*time.Second)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -153,10 +200,9 @@
t.Run("Command with stderr", func(t *testing.T) {
req := bashInput{
Command: "echo 'Error message' >&2 && echo 'Success'",
- Timeout: "5s",
}
- output, err := executeBash(ctx, req)
+ output, err := executeBash(ctx, req, 5*time.Second)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -171,10 +217,9 @@
t.Run("Failed Command with stderr", func(t *testing.T) {
req := bashInput{
Command: "echo 'Error message' >&2 && exit 1",
- Timeout: "5s",
}
- _, err := executeBash(ctx, req)
+ _, err := executeBash(ctx, req, 5*time.Second)
if err == nil {
t.Errorf("Expected error for failed command, got none")
} else if !strings.Contains(err.Error(), "Error message") {
@@ -186,11 +231,10 @@
t.Run("Command Timeout", func(t *testing.T) {
req := bashInput{
Command: "sleep 1 && echo 'Should not see this'",
- Timeout: "100ms",
}
start := time.Now()
- _, err := executeBash(ctx, req)
+ _, err := executeBash(ctx, req, 100*time.Millisecond)
elapsed := time.Since(start)
// Command should time out after ~100ms, not wait for full 1 second
@@ -207,6 +251,9 @@
}
func TestBackgroundBash(t *testing.T) {
+ var bashTool BashTool
+ tool := bashTool.Tool()
+
// Test basic background execution
t.Run("Basic Background Command", func(t *testing.T) {
inputObj := struct {
@@ -221,7 +268,7 @@
t.Fatalf("Failed to marshal input: %v", err)
}
- result, err := Bash.Run(context.Background(), inputJSON)
+ result, err := tool.Run(context.Background(), inputJSON)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -279,7 +326,7 @@
t.Fatalf("Failed to marshal input: %v", err)
}
- result, err := Bash.Run(context.Background(), inputJSON)
+ result, err := tool.Run(context.Background(), inputJSON)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -337,7 +384,7 @@
}
// Start the command in the background
- result, err := Bash.Run(context.Background(), inputJSON)
+ result, err := tool.Run(context.Background(), inputJSON)
if err != nil {
t.Fatalf("Unexpected error: %v", err)
}
@@ -390,8 +437,8 @@
Command: "echo 'test'",
Background: false,
}
- fgTimeout := foreground.timeout()
- expectedFg := 10 * time.Second
+ fgTimeout := foreground.timeout(nil)
+ expectedFg := 30 * time.Second
if fgTimeout != expectedFg {
t.Errorf("Expected foreground default timeout to be %v, got %v", expectedFg, fgTimeout)
}
@@ -401,22 +448,38 @@
Command: "echo 'test'",
Background: true,
}
- bgTimeout := background.timeout()
- expectedBg := 10 * time.Minute
+ bgTimeout := background.timeout(nil)
+ expectedBg := 24 * time.Hour
if bgTimeout != expectedBg {
t.Errorf("Expected background default timeout to be %v, got %v", expectedBg, bgTimeout)
}
- // Test explicit timeout overrides defaults
- explicit := bashInput{
+ // Test slow_ok timeout
+ slowOk := bashInput{
Command: "echo 'test'",
- Background: true,
- Timeout: "5s",
+ Background: false,
+ SlowOK: true,
}
- explicitTimeout := explicit.timeout()
- expectedExplicit := 5 * time.Second
- if explicitTimeout != expectedExplicit {
- t.Errorf("Expected explicit timeout to be %v, got %v", expectedExplicit, explicitTimeout)
+ slowTimeout := slowOk.timeout(nil)
+ expectedSlow := 15 * time.Minute
+ if slowTimeout != expectedSlow {
+ t.Errorf("Expected slow_ok timeout to be %v, got %v", expectedSlow, slowTimeout)
+ }
+
+ // Test custom timeout config
+ customTimeouts := &Timeouts{
+ Fast: 5 * time.Second,
+ Slow: 2 * time.Minute,
+ Background: 1 * time.Hour,
+ }
+ customFast := bashInput{
+ Command: "echo 'test'",
+ Background: false,
+ }
+ customTimeout := customFast.timeout(customTimeouts)
+ expectedCustom := 5 * time.Second
+ if customTimeout != expectedCustom {
+ t.Errorf("Expected custom timeout to be %v, got %v", expectedCustom, customTimeout)
}
})
}