diff --git a/.gitignore b/.gitignore index 2c3156b9..bf3aa421 100644 --- a/.gitignore +++ b/.gitignore @@ -57,3 +57,6 @@ gha-creds-*.json # Log files patch_output.log + +# test files +demo-app \ No newline at end of file diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index be0c26ff..a9ad273b 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -7,7 +7,7 @@ import stripAnsi from 'strip-ansi'; import type { PtyImplementation } from '../utils/getPty.js'; import { getPty } from '../utils/getPty.js'; -import { spawn as cpSpawn } from 'node:child_process'; +import { spawn as cpSpawn, spawnSync } from 'node:child_process'; import { TextDecoder } from 'node:util'; import os from 'node:os'; import type { IPty } from '@lydell/node-pty'; @@ -106,6 +106,51 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { export class ShellExecutionService { private static activePtys = new Map(); + private static activeChildProcesses = new Set(); + + static { + const cleanup = () => { + // Cleanup PTYs + for (const [pid, pty] of this.activePtys) { + try { + if (os.platform() === 'win32') { + pty.ptyProcess.kill(); + } else { + process.kill(-pid, 'SIGKILL'); + } + } catch { + // ignore + } + } + + // Cleanup child processes + for (const pid of this.activeChildProcesses) { + try { + if (os.platform() === 'win32') { + spawnSync('taskkill', ['/pid', pid.toString(), '/f', '/t']); + } else { + process.kill(-pid, 'SIGKILL'); + } + } catch { + // ignore + } + } + }; + + process.on('exit', cleanup); + + // Ensure cleanup happens on SIGINT/SIGTERM + const signalHandler = () => { + process.exit(); + }; + + // We only attach these if we are in a node environment where we can control the process + if (typeof process !== 'undefined' && process.on) { + process.on('SIGINT', signalHandler); + process.on('SIGTERM', signalHandler); + } + } + /** * Executes a shell command using `node-pty`, capturing all output and lifecycle events. * @@ -281,9 +326,13 @@ export class ShellExecutionService { abortSignal.addEventListener('abort', abortHandler, { once: true }); + if (child.pid) { + this.activeChildProcesses.add(child.pid); + } + child.on('exit', (code, signal) => { if (child.pid) { - this.activePtys.delete(child.pid); + this.activeChildProcesses.delete(child.pid); } handleExit(code, signal); }); @@ -310,7 +359,7 @@ export class ShellExecutionService { } }); - return { pid: undefined, result }; + return { pid: child.pid, result }; } catch (e) { const error = e as Error; return { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 17e40dbe..b303b0fa 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -29,6 +29,7 @@ import { summarizeToolOutput } from '../utils/summarizer.js'; import type { ShellExecutionConfig, ShellOutputEvent, + ShellExecutionResult, } from '../services/shellExecutionService.js'; import { ShellExecutionService } from '../services/shellExecutionService.js'; import { formatMemoryUsage } from '../utils/formatters.js'; @@ -47,6 +48,7 @@ export interface ShellToolParams { is_background: boolean; description?: string; directory?: string; + timeout?: number; } export class ShellToolInvocation extends BaseToolInvocation< @@ -132,6 +134,14 @@ export class ShellToolInvocation extends BaseToolInvocation< .toString('hex')}.tmp`; const tempFilePath = path.join(os.tmpdir(), tempFileName); + const timeoutMs = this.params.timeout ?? 3600000; + const abortController = new AbortController(); + const onAbort = () => abortController.abort(); + signal.addEventListener('abort', onAbort); + const timeoutId = setTimeout(() => { + abortController.abort(); + }, timeoutMs); + try { // Add co-author to git commit commands const processedCommand = this.addCoAuthorToGitCommit(strippedCommand); @@ -139,11 +149,30 @@ export class ShellToolInvocation extends BaseToolInvocation< const shouldRunInBackground = this.params.is_background; let finalCommand = processedCommand; - // If explicitly marked as background and doesn't already end with &, add it - if (shouldRunInBackground && !finalCommand.trim().endsWith('&')) { + // On non-Windows, use & to run in background. + // On Windows, we don't use start /B because it creates a detached process that + // doesn't die when the parent dies. Instead, we rely on the race logic below + // to return early while keeping the process attached (detached: false). + if ( + !isWindows && + shouldRunInBackground && + !finalCommand.trim().endsWith('&') + ) { finalCommand = finalCommand.trim() + ' &'; } + // On Windows, append a keep-alive command to ensure the shell process + // stays alive even if the main command exits (e.g. spawns a detached child). + // This ensures we always have a valid PID for cleanup. + if (isWindows && shouldRunInBackground) { + // Remove trailing & if present to avoid syntax errors (e.g. "cmd & & ping") + let cmd = finalCommand.trim(); + while (cmd.endsWith('&')) { + cmd = cmd.slice(0, -1).trim(); + } + finalCommand = cmd + ' & ping -n 86400 127.0.0.1 >nul'; + } + // pgrep is not available on Windows, so we can't get background PIDs const commandToExecute = isWindows ? finalCommand @@ -206,7 +235,7 @@ export class ShellToolInvocation extends BaseToolInvocation< lastUpdateTime = Date.now(); } }, - signal, + abortController.signal, this.config.getShouldUseNodePtyShell(), shellExecutionConfig ?? {}, ); @@ -215,7 +244,34 @@ export class ShellToolInvocation extends BaseToolInvocation< setPidCallback(pid); } - const result = await resultPromise; + let result: ShellExecutionResult; + if (shouldRunInBackground && isWindows) { + // For Windows background tasks, we wait a short time to catch immediate errors. + // If it's still running, we return early. + const startupDelay = 1000; + const raceResult = await Promise.race([ + resultPromise, + new Promise((resolve) => + setTimeout(() => resolve(null), startupDelay), + ), + ]); + + if (raceResult === null) { + // Timeout reached, process is still running. + const pidMsg = pid ? ` PID: ${pid}` : ''; + const winHint = isWindows + ? ' (Note: Use taskkill /F /T /PID to stop)' + : ''; + return { + llmContent: `Background command started.${pidMsg}${winHint}`, + returnDisplay: `Background command started.${pidMsg}${winHint}`, + }; + } else { + result = raceResult; + } + } else { + result = await resultPromise; + } const backgroundPIDs: number[] = []; if (os.platform() !== 'win32') { @@ -321,6 +377,8 @@ export class ShellToolInvocation extends BaseToolInvocation< ...executionError, }; } finally { + clearTimeout(timeoutId); + signal.removeEventListener('abort', onAbort); if (fs.existsSync(tempFilePath)) { fs.unlinkSync(tempFilePath); } @@ -454,6 +512,11 @@ export class ShellTool extends BaseDeclarativeTool< description: '(OPTIONAL) The absolute path of the directory to run the command in. If not provided, the project root directory is used. Must be a directory within the workspace and must already exist.', }, + timeout: { + type: 'number', + description: + '(OPTIONAL) The timeout in milliseconds for the command. If not provided, a default timeout (1 hour) is applied.', + }, }, required: ['command', 'is_background'], }, @@ -478,6 +541,9 @@ export class ShellTool extends BaseDeclarativeTool< if (!params.command.trim()) { return 'Command cannot be empty.'; } + if (params.timeout !== undefined && params.timeout <= 0) { + return 'Timeout must be a positive number.'; + } if (getCommandRoots(params.command).length === 0) { return 'Could not identify command root to obtain permission from user.'; }