From 16939c0bc8cbe2ee0e7743e6fd96fced51f35a10 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Wed, 10 Dec 2025 13:49:51 +0800 Subject: [PATCH] Refactor ShellTool: remove ping hack and timeout, optimize cleanup --- .../src/services/shellExecutionService.ts | 26 +++++++++++----- packages/core/src/tools/shell.test.ts | 6 ++-- packages/core/src/tools/shell.ts | 31 +++---------------- 3 files changed, 26 insertions(+), 37 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index e501e6ec..4a638c62 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -123,15 +123,25 @@ export class ShellExecutionService { } // 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'); + if (os.platform() === 'win32') { + if (this.activeChildProcesses.size > 0) { + try { + const args = ['/f', '/t']; + for (const pid of this.activeChildProcesses) { + args.push('/pid', pid.toString()); + } + spawnSync('taskkill', args); + } catch { + // ignore + } + } + } else { + for (const pid of this.activeChildProcesses) { + try { + process.kill(-pid, 'SIGKILL'); + } catch { + // ignore } - } catch { - // ignore } } } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index b98e7158..3484c53b 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -833,12 +833,12 @@ describe('ShellTool', () => { }); describe('Windows background execution', () => { - it('should append keep-alive ping with && on Windows for background tasks', async () => { + it('should clean up trailing ampersand on Windows for background tasks', async () => { vi.mocked(os.platform).mockReturnValue('win32'); const mockAbortSignal = new AbortController().signal; const invocation = shellTool.build({ - command: 'npm start', + command: 'npm start &', is_background: true, }); @@ -859,7 +859,7 @@ describe('ShellTool', () => { await promise; expect(mockShellExecutionService).toHaveBeenCalledWith( - expect.stringContaining('npm start && ping -n 86400 127.0.0.1 >nul'), + 'npm start', expect.any(String), expect.any(Function), expect.any(AbortSignal), diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 8d7610d4..a886010d 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -49,7 +49,6 @@ export interface ShellToolParams { is_background: boolean; description?: string; directory?: string; - timeout?: number; } export class ShellToolInvocation extends BaseToolInvocation< @@ -138,14 +137,6 @@ 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); @@ -165,16 +156,15 @@ export class ShellToolInvocation extends BaseToolInvocation< 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. + // On Windows, we rely on the race logic below to handle background tasks. + // We just ensure the command string is clean. if (isWindows && shouldRunInBackground) { - // Remove trailing & if present to avoid syntax errors (e.g. "cmd & & ping") let cmd = finalCommand.trim(); + // Remove trailing & (common Linux habit, invalid on Windows at end of line) while (cmd.endsWith('&')) { cmd = cmd.slice(0, -1).trim(); } - finalCommand = cmd + ' && ping -n 86400 127.0.0.1 >nul'; + finalCommand = cmd; } // pgrep is not available on Windows, so we can't get background PIDs @@ -239,7 +229,7 @@ export class ShellToolInvocation extends BaseToolInvocation< lastUpdateTime = Date.now(); } }, - abortController.signal, + signal, this.config.getShouldUseNodePtyShell(), shellExecutionConfig ?? {}, ); @@ -277,7 +267,6 @@ export class ShellToolInvocation extends BaseToolInvocation< ]; if (errorPatterns.some((pattern) => outputStr.includes(pattern))) { - abortController.abort(); return { llmContent: `Command failed to start: ${outputStr}`, returnDisplay: `Command failed to start: ${outputStr}`, @@ -407,8 +396,6 @@ export class ShellToolInvocation extends BaseToolInvocation< ...executionError, }; } finally { - clearTimeout(timeoutId); - signal.removeEventListener('abort', onAbort); if (fs.existsSync(tempFilePath)) { fs.unlinkSync(tempFilePath); } @@ -542,11 +529,6 @@ 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'], }, @@ -571,9 +553,6 @@ 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.'; }