From 5c020481a4732da5024e7432587f97731c42d8cb Mon Sep 17 00:00:00 2001 From: kujtimiihoxha Date: Wed, 29 Oct 2025 15:20:28 +0100 Subject: [PATCH] chore: auto background and remove persistent shell --- internal/agent/agent_test.go | 2 - internal/agent/tools/bash.go | 168 +++++++++++-------- internal/agent/tools/bash.tpl | 13 +- internal/agent/tools/bash_background_test.go | 75 +++++++++ internal/shell/background.go | 44 +++-- internal/shell/doc.go | 7 +- internal/shell/persistent.go | 43 ----- internal/shell/shell.go | 11 +- 8 files changed, 214 insertions(+), 149 deletions(-) delete mode 100644 internal/shell/persistent.go diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 0682083ae20daaa1c17747afdfd6b4db358b88fa..bd586ccd4169b096bc7c9f53fe9bf61a310c6829 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -10,7 +10,6 @@ import ( "charm.land/fantasy" "github.com/charmbracelet/crush/internal/agent/tools" "github.com/charmbracelet/crush/internal/message" - "github.com/charmbracelet/crush/internal/shell" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gopkg.in/dnaeon/go-vcr.v4/pkg/recorder" @@ -40,7 +39,6 @@ func setupAgent(t *testing.T, pair modelPair) (SessionAgent, env) { createSimpleGoProject(t, env.workingDir) agent, err := coderAgent(r, env, large, small) - shell.Reset(env.workingDir) require.NoError(t, err) return agent, env } diff --git a/internal/agent/tools/bash.go b/internal/agent/tools/bash.go index 29b83fbaa99429c8fc2c03d58711479519d040f8..bfc95888538430a464052b33c6a55adacbed4a4a 100644 --- a/internal/agent/tools/bash.go +++ b/internal/agent/tools/bash.go @@ -21,14 +21,14 @@ import ( type BashParams struct { Command string `json:"command" description:"The command to execute"` Description string `json:"description,omitempty" description:"A brief description of what the command does"` - Timeout int `json:"timeout,omitempty" description:"Optional timeout in milliseconds (max 600000)"` + WorkingDir string `json:"working_dir,omitempty" description:"The working directory to execute the command in (defaults to current directory)"` Background bool `json:"background,omitempty" description:"Run the command in a background shell. Returns a shell ID for managing the process."` } type BashPermissionsParams struct { Command string `json:"command"` Description string `json:"description"` - Timeout int `json:"timeout"` + WorkingDir string `json:"working_dir"` Background bool `json:"background"` } @@ -45,10 +45,9 @@ type BashResponseMetadata struct { const ( BashToolName = "bash" - DefaultTimeout = 1 * 60 * 1000 // 1 minutes in milliseconds - MaxTimeout = 10 * 60 * 1000 // 10 minutes in milliseconds - MaxOutputLength = 30000 - BashNoOutput = "no output" + AutoBackgroundThreshold = 1 * time.Minute // Commands taking longer automatically become background jobs + MaxOutputLength = 30000 + BashNoOutput = "no output" ) //go:embed bash.tpl @@ -185,23 +184,20 @@ func blockFuncs() []shell.BlockFunc { } func NewBashTool(permissions permission.Service, workingDir string, attribution *config.Attribution) fantasy.AgentTool { - // Set up command blocking on the persistent shell - persistentShell := shell.GetPersistentShell(workingDir) - persistentShell.SetBlockFuncs(blockFuncs()) return fantasy.NewAgentTool( BashToolName, string(bashDescription(attribution)), func(ctx context.Context, params BashParams, call fantasy.ToolCall) (fantasy.ToolResponse, error) { - if params.Timeout > MaxTimeout { - params.Timeout = MaxTimeout - } else if params.Timeout <= 0 { - params.Timeout = DefaultTimeout - } - if params.Command == "" { return fantasy.NewTextErrorResponse("missing command"), nil } + // Determine working directory + execWorkingDir := workingDir + if params.WorkingDir != "" { + execWorkingDir = params.WorkingDir + } + isSafeReadOnly := false cmdLower := strings.ToLower(params.Command) @@ -219,25 +215,15 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution return fantasy.ToolResponse{}, fmt.Errorf("session ID is required for executing shell command") } if !isSafeReadOnly { - var shellDir string - if params.Background { - shellDir = workingDir - } else { - shellDir = shell.GetPersistentShell(workingDir).GetWorkingDir() - } p := permissions.Request( permission.CreatePermissionRequest{ SessionID: sessionID, - Path: shellDir, + Path: execWorkingDir, ToolCallID: call.ID, ToolName: BashToolName, Action: "execute", Description: fmt.Sprintf("Execute command: %s", params.Command), - Params: BashPermissionsParams{ - Command: params.Command, - Description: params.Description, - Background: params.Background, - }, + Params: BashPermissionsParams(params), }, ) if !p { @@ -245,10 +231,11 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution } } + // If explicitly requested as background, start immediately if params.Background { startTime := time.Now() bgManager := shell.GetBackgroundShellManager() - bgShell, err := bgManager.Start(ctx, workingDir, blockFuncs(), params.Command) + bgShell, err := bgManager.Start(ctx, execWorkingDir, blockFuncs(), params.Command) if err != nil { return fantasy.ToolResponse{}, fmt.Errorf("error starting background shell: %w", err) } @@ -265,65 +252,110 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution return fantasy.WithResponseMetadata(fantasy.NewTextResponse(response), metadata), nil } + // Start synchronous execution with auto-background support startTime := time.Now() - if params.Timeout > 0 { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, time.Duration(params.Timeout)*time.Millisecond) - defer cancel() - } - persistentShell := shell.GetPersistentShell(workingDir) - stdout, stderr, err := persistentShell.Exec(ctx, params.Command) + // Start background shell immediately but wait for threshold before deciding + bgManager := shell.GetBackgroundShellManager() + bgShell, err := bgManager.Start(ctx, execWorkingDir, blockFuncs(), params.Command) + if err != nil { + return fantasy.ToolResponse{}, fmt.Errorf("error starting shell: %w", err) + } - currentWorkingDir := persistentShell.GetWorkingDir() - interrupted := shell.IsInterrupt(err) - exitCode := shell.ExitCode(err) - if exitCode == 0 && !interrupted && err != nil { - return fantasy.ToolResponse{}, fmt.Errorf("error executing command: %w", err) + // Wait for either completion, auto-background threshold, or context cancellation + ticker := time.NewTicker(100 * time.Millisecond) + defer ticker.Stop() + timeout := time.After(AutoBackgroundThreshold) + + var stdout, stderr string + var done bool + var execErr error + + waitLoop: + for { + select { + case <-ticker.C: + stdout, stderr, done, execErr = bgShell.GetOutput() + if done { + break waitLoop + } + case <-timeout: + stdout, stderr, done, execErr = bgShell.GetOutput() + break waitLoop + case <-ctx.Done(): + // Context was cancelled, kill the shell and return error + bgManager.Kill(bgShell.ID) + return fantasy.ToolResponse{}, ctx.Err() + } } - stdout = truncateOutput(stdout) - stderr = truncateOutput(stderr) + if done { + // Command completed within threshold - return synchronously + // Remove from background manager since we're returning directly + // Don't call Kill() as it cancels the context and corrupts the exit code + bgManager.Remove(bgShell.ID) - errorMessage := stderr - if errorMessage == "" && err != nil { - errorMessage = err.Error() - } + interrupted := shell.IsInterrupt(execErr) + exitCode := shell.ExitCode(execErr) + if exitCode == 0 && !interrupted && execErr != nil { + return fantasy.ToolResponse{}, fmt.Errorf("error executing command: %w", execErr) + } - if interrupted { - if errorMessage != "" { - errorMessage += "\n" + stdout = truncateOutput(stdout) + stderr = truncateOutput(stderr) + + errorMessage := stderr + if errorMessage == "" && execErr != nil { + errorMessage = execErr.Error() } - errorMessage += "Command was aborted before completion" - } else if exitCode != 0 { - if errorMessage != "" { - errorMessage += "\n" + + if interrupted { + if errorMessage != "" { + errorMessage += "\n" + } + errorMessage += "Command was aborted before completion" + } else if exitCode != 0 { + if errorMessage != "" { + errorMessage += "\n" + } + errorMessage += fmt.Sprintf("Exit code %d", exitCode) } - errorMessage += fmt.Sprintf("Exit code %d", exitCode) - } - hasBothOutputs := stdout != "" && stderr != "" + hasBothOutputs := stdout != "" && stderr != "" - if hasBothOutputs { - stdout += "\n" - } + if hasBothOutputs { + stdout += "\n" + } - if errorMessage != "" { - stdout += "\n" + errorMessage + if errorMessage != "" { + stdout += "\n" + errorMessage + } + + metadata := BashResponseMetadata{ + StartTime: startTime.UnixMilli(), + EndTime: time.Now().UnixMilli(), + Output: stdout, + Description: params.Description, + WorkingDirectory: bgShell.GetWorkingDir(), + } + if stdout == "" { + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(BashNoOutput), metadata), nil + } + stdout += fmt.Sprintf("\n\n%s", normalizeWorkingDir(bgShell.GetWorkingDir())) + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(stdout), metadata), nil } + // Still running - keep as background job metadata := BashResponseMetadata{ StartTime: startTime.UnixMilli(), EndTime: time.Now().UnixMilli(), - Output: stdout, Description: params.Description, - WorkingDirectory: currentWorkingDir, - } - if stdout == "" { - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(BashNoOutput), metadata), nil + WorkingDirectory: bgShell.GetWorkingDir(), + Background: true, + ShellID: bgShell.ID, } - stdout += fmt.Sprintf("\n\n%s", normalizeWorkingDir(currentWorkingDir)) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(stdout), metadata), nil + response := fmt.Sprintf("Command is taking longer than expected and has been moved to background.\n\nBackground shell ID: %s\n\nUse bash_output tool to view output or bash_kill to terminate.", bgShell.ID) + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(response), metadata), nil }) } diff --git a/internal/agent/tools/bash.tpl b/internal/agent/tools/bash.tpl index 59fee859ead966c9ffcbe7422b151ae33d30d9de..1f3c1878eb7a866e8ab24b69d0c1de89b7ee5115 100644 --- a/internal/agent/tools/bash.tpl +++ b/internal/agent/tools/bash.tpl @@ -1,4 +1,4 @@ -Executes bash commands in persistent shell session with timeout and security measures. +Executes bash commands with automatic background conversion for long-running tasks. Uses mvdan/sh interpreter (Bash-compatible on all platforms including Windows). @@ -10,21 +10,22 @@ Common shell builtins and core utils available on Windows. 1. Directory Verification: If creating directories/files, use LS tool to verify parent exists 2. Security Check: Banned commands ({{ .BannedCommands }}) return error - explain to user. Safe read-only commands execute without prompts 3. Command Execution: Execute with proper quoting, capture output -4. Output Processing: Truncate if exceeds {{ .MaxOutputLength }} characters -5. Return Result: Include errors, metadata with tags +4. Auto-Background: Commands exceeding 1 minute automatically move to background and return shell ID +5. Output Processing: Truncate if exceeds {{ .MaxOutputLength }} characters +6. Return Result: Include errors, metadata with tags -- Command required, timeout optional (max 600000ms/10min, default 30min if unspecified) +- Command required, working_dir optional (defaults to current directory) - IMPORTANT: Use Grep/Glob/Agent tools instead of 'find'/'grep'. Use View/LS tools instead of 'cat'/'head'/'tail'/'ls' - Chain with ';' or '&&', avoid newlines except in quoted strings -- Shell state persists (env vars, virtual envs, cwd, etc.) unless running in background +- Each command runs in independent shell (no state persistence between calls) - Prefer absolute paths over 'cd' (use 'cd' only if user explicitly requests) - Set background=true to run commands in a separate background shell -- Background shells don't share state with the persistent shell +- Commands taking longer than 1 minute automatically convert to background - Returns a shell ID for managing the background process - Use bash_output tool to view current output from background shell - Use bash_kill tool to terminate a background shell diff --git a/internal/agent/tools/bash_background_test.go b/internal/agent/tools/bash_background_test.go index 2f0c0261b7b274eac9dde26f847d7b0a042b2768..8e9675d419fa6c3532b43a8cbef37cd19f6969b5 100644 --- a/internal/agent/tools/bash_background_test.go +++ b/internal/agent/tools/bash_background_test.go @@ -355,3 +355,78 @@ func TestBackgroundShell_IDFormat(t *testing.T) { require.Greater(t, len(bgShell.ID), 5, "ID should be long enough") require.Less(t, len(bgShell.ID), 100, "ID should not be too long") } + +func TestBackgroundShell_AutoBackground(t *testing.T) { + t.Parallel() + + workingDir := t.TempDir() + ctx := context.Background() + + // Test that a quick command completes synchronously + t.Run("quick command completes synchronously", func(t *testing.T) { + t.Parallel() + bgManager := shell.GetBackgroundShellManager() + bgShell, err := bgManager.Start(ctx, workingDir, nil, "echo 'quick'") + require.NoError(t, err) + + // Wait threshold time + time.Sleep(5 * time.Second) + + // Should be done by now + stdout, stderr, done, err := bgShell.GetOutput() + require.NoError(t, err) + require.True(t, done, "Quick command should be done") + require.Contains(t, stdout, "quick") + require.Empty(t, stderr) + + // Clean up + bgManager.Kill(bgShell.ID) + }) + + // Test that a long command stays in background + t.Run("long command stays in background", func(t *testing.T) { + t.Parallel() + bgManager := shell.GetBackgroundShellManager() + bgShell, err := bgManager.Start(ctx, workingDir, nil, "sleep 20 && echo '20 seconds completed'") + require.NoError(t, err) + defer bgManager.Kill(bgShell.ID) + + // Wait threshold time + time.Sleep(5 * time.Second) + + // Should still be running + stdout, stderr, done, err := bgShell.GetOutput() + require.NoError(t, err) + require.False(t, done, "Long command should still be running") + require.Empty(t, stdout, "No output yet from sleep command") + require.Empty(t, stderr) + + // Verify we can get the shell from manager + retrieved, ok := bgManager.Get(bgShell.ID) + require.True(t, ok, "Should be able to retrieve background shell") + require.Equal(t, bgShell.ID, retrieved.ID) + }) + + // Test that we can check output of long-running command later + t.Run("can check output after completion", func(t *testing.T) { + t.Parallel() + bgManager := shell.GetBackgroundShellManager() + bgShell, err := bgManager.Start(ctx, workingDir, nil, "sleep 3 && echo 'completed'") + require.NoError(t, err) + defer bgManager.Kill(bgShell.ID) + + // Initially should be running + _, _, done, _ := bgShell.GetOutput() + require.False(t, done, "Should be running initially") + + // Wait for completion + time.Sleep(4 * time.Second) + + // Now should be done + stdout, stderr, done, err := bgShell.GetOutput() + require.NoError(t, err) + require.True(t, done, "Should be done after waiting") + require.Contains(t, stdout, "completed") + require.Empty(t, stderr) + }) +} diff --git a/internal/shell/background.go b/internal/shell/background.go index 10b28fa5335fb964ca2193b19f8d77a574eb9efa..f037d8f88db8b716456d798a9f7a4f8bceab9c09 100644 --- a/internal/shell/background.go +++ b/internal/shell/background.go @@ -13,15 +13,14 @@ import ( // BackgroundShell represents a shell running in the background. type BackgroundShell struct { - ID string - Shell *Shell - ctx context.Context - cancel context.CancelFunc - stdout *bytes.Buffer - stderr *bytes.Buffer - done chan struct{} - exitErr error - workingDir string + ID string + Shell *Shell + ctx context.Context + cancel context.CancelFunc + stdout *bytes.Buffer + stderr *bytes.Buffer + done chan struct{} + exitErr error } // BackgroundShellManager manages background shell instances. @@ -56,14 +55,13 @@ func (m *BackgroundShellManager) Start(ctx context.Context, workingDir string, b shellCtx, cancel := context.WithCancel(ctx) bgShell := &BackgroundShell{ - ID: id, - Shell: shell, - ctx: shellCtx, - cancel: cancel, - stdout: &bytes.Buffer{}, - stderr: &bytes.Buffer{}, - done: make(chan struct{}), - workingDir: workingDir, + ID: id, + Shell: shell, + ctx: shellCtx, + cancel: cancel, + stdout: &bytes.Buffer{}, + stderr: &bytes.Buffer{}, + done: make(chan struct{}), } m.shells.Set(id, bgShell) @@ -86,6 +84,16 @@ func (m *BackgroundShellManager) Get(id string) (*BackgroundShell, bool) { return m.shells.Get(id) } +// Remove removes a background shell from the manager without terminating it. +// This is useful when a shell has already completed and you just want to clean up tracking. +func (m *BackgroundShellManager) Remove(id string) error { + _, ok := m.shells.Take(id) + if !ok { + return fmt.Errorf("background shell not found: %s", id) + } + return nil +} + // Kill terminates a background shell by ID. func (m *BackgroundShellManager) Kill(id string) error { shell, ok := m.shells.Take(id) @@ -148,5 +156,5 @@ func (bs *BackgroundShell) Wait() { // GetWorkingDir returns the current working directory of the background shell. func (bs *BackgroundShell) GetWorkingDir() string { - return bs.workingDir + return bs.Shell.GetWorkingDir() } diff --git a/internal/shell/doc.go b/internal/shell/doc.go index 13bfb5553628376f7fe7e49e5d707cb9c28fcbc7..feed86db011d04178a1643d757d618d313c1f579 100644 --- a/internal/shell/doc.go +++ b/internal/shell/doc.go @@ -16,12 +16,7 @@ package shell // shell.Exec(ctx, "export FOO=bar") // shell.Exec(ctx, "echo $FOO") // Will print "bar" // -// 3. For the singleton persistent shell (used by tools): -// -// shell := shell.GetPersistentShell("/path/to/cwd") -// stdout, stderr, err := shell.Exec(ctx, "ls -la") -// -// 4. Managing environment and working directory: +// 3. Managing environment and working directory: // // shell := shell.NewShell(nil) // shell.SetEnv("MY_VAR", "value") diff --git a/internal/shell/persistent.go b/internal/shell/persistent.go deleted file mode 100644 index 5f6fd4556518ae2ee700a16b74ea6f18b1c4d1d9..0000000000000000000000000000000000000000 --- a/internal/shell/persistent.go +++ /dev/null @@ -1,43 +0,0 @@ -package shell - -import ( - "log/slog" - "sync" -) - -// PersistentShell is a singleton shell instance that maintains state across the application -type PersistentShell struct { - *Shell -} - -var ( - once sync.Once - shellInstance *PersistentShell -) - -// GetPersistentShell returns the singleton persistent shell instance -// This maintains backward compatibility with the existing API -func GetPersistentShell(cwd string) *PersistentShell { - once.Do(func() { - shellInstance = &PersistentShell{ - Shell: NewShell(&Options{ - WorkingDir: cwd, - Logger: &loggingAdapter{}, - }), - } - }) - return shellInstance -} - -// INFO: only used for tests -func Reset(cwd string) { - once = sync.Once{} - _ = GetPersistentShell(cwd) -} - -// slog.dapter adapts the internal slog.package to the Logger interface -type loggingAdapter struct{} - -func (l *loggingAdapter) InfoPersist(msg string, keysAndValues ...any) { - slog.Info(msg, keysAndValues...) -} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index 5a10be9537714162e4d5ed25360b42690395793f..6ee1f988e33ca8ec3a4a751c4757e216ce26a2e7 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -1,13 +1,12 @@ // Package shell provides cross-platform shell execution capabilities. // -// This package offers two main types: -// - Shell: A general-purpose shell executor for one-off or managed commands -// - PersistentShell: A singleton shell that maintains state across the application +// This package provides Shell instances for executing commands with their own +// working directory and environment. Each shell execution is independent. // // WINDOWS COMPATIBILITY: -// This implementation provides both POSIX shell emulation (mvdan.cc/sh/v3), -// even on Windows. Some caution has to be taken: commands should have forward -// slashes (/) as path separators to work, even on Windows. +// This implementation provides POSIX shell emulation (mvdan.cc/sh/v3) even on +// Windows. Commands should use forward slashes (/) as path separators to work +// correctly on all platforms. package shell import (