From d9928eab664a7280315c4c59867d525cf37b5eed Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Thu, 4 Dec 2025 15:20:45 +0800 Subject: [PATCH 01/12] fix: improve windows background process handling and cleanup --- .gitignore | 3 + .../src/services/shellExecutionService.ts | 55 +++++++++++++- packages/core/src/tools/shell.ts | 74 ++++++++++++++++++- 3 files changed, 125 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index 2c3156b96..bf3aa4211 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 be0c26ff7..a9ad273bf 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 17e40dbe7..b303b0fac 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.'; } From 403fd061171be14d6e738e600f394b4f0a663dc5 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Thu, 4 Dec 2025 15:51:08 +0800 Subject: [PATCH 02/12] chore: update .gitignore --- .gitignore | 3 --- 1 file changed, 3 deletions(-) diff --git a/.gitignore b/.gitignore index bf3aa4211..2c3156b96 100644 --- a/.gitignore +++ b/.gitignore @@ -57,6 +57,3 @@ gha-creds-*.json # Log files patch_output.log - -# test files -demo-app \ No newline at end of file From 4c69d536acfe9b04a08695d5dd1804f9712ccafc Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Fri, 5 Dec 2025 10:47:06 +0800 Subject: [PATCH 03/12] test: fix shell tool tests by updating pid expectation and AbortSignal matching --- .../services/shellExecutionService.test.ts | 2 +- packages/core/src/tools/shell.test.ts | 30 +++++++++---------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 4dfc48918..c5a6e0776 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -589,7 +589,7 @@ describe('ShellExecutionService child_process fallback', () => { expect(result.error).toBeNull(); expect(result.aborted).toBe(false); expect(result.output).toBe('file1.txt\na warning'); - expect(handle.pid).toBe(undefined); + expect(handle.pid).toBe(12345); expect(onOutputEventMock).toHaveBeenCalledWith({ type: 'data', diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 043ab0c64..bde508370 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -210,7 +210,7 @@ describe('ShellTool', () => { wrappedCommand, '/test/dir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -237,7 +237,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -262,7 +262,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -287,7 +287,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -312,7 +312,7 @@ describe('ShellTool', () => { wrappedCommand, '/test/dir/subdir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -340,7 +340,7 @@ describe('ShellTool', () => { 'dir', '/test/dir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -433,7 +433,7 @@ describe('ShellTool', () => { expect(summarizer.summarizeToolOutput).toHaveBeenCalledWith( expect.any(String), mockConfig.getGeminiClient(), - mockAbortSignal, + expect.any(AbortSignal), 1000, ); expect(result.llmContent).toBe('summarized output'); @@ -542,7 +542,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -572,7 +572,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -602,7 +602,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -631,7 +631,7 @@ describe('ShellTool', () => { expect.stringContaining('npm install'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -660,7 +660,7 @@ describe('ShellTool', () => { expect.stringContaining('git commit'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -690,7 +690,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -726,7 +726,7 @@ describe('ShellTool', () => { expect.stringContaining('git commit -m "Initial commit"'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -763,7 +763,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); From 28d178b5c1878570fa193e5342675b1f251db0ce Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Tue, 9 Dec 2025 11:24:30 +0800 Subject: [PATCH 04/12] fix: handle windows background execution errors and add tests --- .../src/services/shellExecutionService.ts | 67 ++++++++----------- packages/core/src/tools/shell.test.ts | 29 ++++++++ packages/core/src/tools/shell.ts | 38 +++++++++-- 3 files changed, 90 insertions(+), 44 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index a9ad273bf..e501e6ecd 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -108,47 +108,38 @@ 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 + static 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); } + + // 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 + } + } + } + + static { + process.on('exit', () => { + ShellExecutionService.cleanup(); + }); } /** diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index bde508370..b431f494f 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -831,4 +831,33 @@ describe('ShellTool', () => { expect(shellTool.description).toMatchSnapshot(); }); }); + + describe('Windows background execution', () => { + it('should detect immediate failure in Windows background task', async () => { + vi.mocked(os.platform).mockReturnValue('win32'); + const mockAbortSignal = new AbortController().signal; + + const invocation = shellTool.build({ + command: 'invalid_command', + is_background: true, + }); + + const promise = invocation.execute(mockAbortSignal); + + // Wait a tick to ensure mockShellOutputCallback is assigned + await new Promise((resolve) => setTimeout(resolve, 0)); + + if (mockShellOutputCallback) { + mockShellOutputCallback({ + type: 'data', + chunk: + "'invalid_command' is not recognized as an internal or external command,\r\noperable program or batch file.\r\n", + }); + } + + const result = await promise; + expect(result.error).toBeDefined(); + expect(result.llmContent).toContain('Command failed to start'); + }); + }); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index b303b0fac..4ee7e79c6 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -194,16 +194,16 @@ export class ShellToolInvocation extends BaseToolInvocation< commandToExecute, cwd, (event: ShellOutputEvent) => { - if (!updateOutput) { - return; - } - let shouldUpdate = false; switch (event.type) { case 'data': if (isBinaryStream) break; - cumulativeOutput = event.chunk; + if (typeof cumulativeOutput === 'string') { + cumulativeOutput += event.chunk; + } else { + cumulativeOutput = event.chunk; + } shouldUpdate = true; break; case 'binary_detected': @@ -226,7 +226,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } } - if (shouldUpdate) { + if (shouldUpdate && updateOutput) { updateOutput( typeof cumulativeOutput === 'string' ? cumulativeOutput @@ -258,6 +258,32 @@ export class ShellToolInvocation extends BaseToolInvocation< if (raceResult === null) { // Timeout reached, process is still running. + // throw new Error(`DEBUG: raceResult is null. Output: ${JSON.stringify(cumulativeOutput)}`); + + // Check for common Windows error messages in the output + const outputStr = + typeof cumulativeOutput === 'string' + ? cumulativeOutput + : JSON.stringify(cumulativeOutput); + console.log('DEBUG: outputStr:', outputStr); + const errorPatterns = [ + 'is not recognized as an internal or external command', + 'The system cannot find the path specified', + 'Access is denied', + ]; + + if (errorPatterns.some((pattern) => outputStr.includes(pattern))) { + abortController.abort(); + return { + llmContent: `Command failed to start: ${outputStr}`, + returnDisplay: `Command failed to start: ${outputStr}`, + error: { + type: ToolErrorType.EXECUTION_FAILED, + message: `Command failed to start: ${outputStr}`, + }, + }; + } + const pidMsg = pid ? ` PID: ${pid}` : ''; const winHint = isWindows ? ' (Note: Use taskkill /F /T /PID to stop)' From 6fc09a82fb6f3cc9e7dd50ea706bddd6e346724b Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Tue, 9 Dec 2025 13:33:42 +0800 Subject: [PATCH 05/12] fix: use && for windows background keep-alive ping and add test --- packages/core/src/tools/shell.test.ts | 35 +++++++++++++++++++++++++++ packages/core/src/tools/shell.ts | 2 +- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index b431f494f..b98e71584 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -833,6 +833,41 @@ describe('ShellTool', () => { }); describe('Windows background execution', () => { + it('should append keep-alive ping with && on Windows for background tasks', async () => { + vi.mocked(os.platform).mockReturnValue('win32'); + const mockAbortSignal = new AbortController().signal; + + const invocation = shellTool.build({ + command: 'npm start', + is_background: true, + }); + + const promise = invocation.execute(mockAbortSignal); + + // Simulate immediate success (process started) + resolveExecutionPromise({ + rawOutput: Buffer.from(''), + output: '', + exitCode: 0, + signal: null, + error: null, + aborted: false, + pid: 12345, + executionMethod: 'child_process', + }); + + await promise; + + expect(mockShellExecutionService).toHaveBeenCalledWith( + expect.stringContaining('npm start && ping -n 86400 127.0.0.1 >nul'), + expect.any(String), + expect.any(Function), + expect.any(AbortSignal), + false, + {}, + ); + }); + it('should detect immediate failure in Windows background task', async () => { vi.mocked(os.platform).mockReturnValue('win32'); const mockAbortSignal = new AbortController().signal; diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 55bc4df02..8d7610d46 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -174,7 +174,7 @@ export class ShellToolInvocation extends BaseToolInvocation< while (cmd.endsWith('&')) { cmd = cmd.slice(0, -1).trim(); } - finalCommand = cmd + ' & ping -n 86400 127.0.0.1 >nul'; + finalCommand = cmd + ' && ping -n 86400 127.0.0.1 >nul'; } // pgrep is not available on Windows, so we can't get background PIDs From 16939c0bc8cbe2ee0e7743e6fd96fced51f35a10 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Wed, 10 Dec 2025 13:49:51 +0800 Subject: [PATCH 06/12] 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 e501e6ecd..4a638c620 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 b98e71584..3484c53b2 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 8d7610d46..a886010d9 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.'; } From 574d89da14cf50ad458b76850c7bec5069063ef7 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Fri, 12 Dec 2025 17:03:04 +0800 Subject: [PATCH 07/12] Refactor ShellExecutionService cleanup to use strategy pattern --- .../src/services/shellExecutionService.ts | 70 ++++++++++++------- 1 file changed, 44 insertions(+), 26 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 4a638c620..f78094329 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -98,6 +98,48 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { return lines.join('\n').trimEnd(); }; +interface ProcessCleanupStrategy { + killPty(pid: number, pty: ActivePty): void; + killChildProcesses(pids: Set): void; +} + +const windowsStrategy: ProcessCleanupStrategy = { + killPty: (_pid, pty) => { + pty.ptyProcess.kill(); + }, + killChildProcesses: (pids) => { + if (pids.size > 0) { + try { + const args = ['/f', '/t']; + for (const pid of pids) { + args.push('/pid', pid.toString()); + } + spawnSync('taskkill', args); + } catch { + // ignore + } + } + }, +}; + +const posixStrategy: ProcessCleanupStrategy = { + killPty: (pid, _pty) => { + process.kill(-pid, 'SIGKILL'); + }, + killChildProcesses: (pids) => { + for (const pid of pids) { + try { + process.kill(-pid, 'SIGKILL'); + } catch { + // ignore + } + } + }, +}; + +const cleanupStrategy = + os.platform() === 'win32' ? windowsStrategy : posixStrategy; + /** * A centralized service for executing shell commands with robust process * management, cross-platform compatibility, and streaming output capabilities. @@ -112,38 +154,14 @@ export class ShellExecutionService { // Cleanup PTYs for (const [pid, pty] of this.activePtys) { try { - if (os.platform() === 'win32') { - pty.ptyProcess.kill(); - } else { - process.kill(-pid, 'SIGKILL'); - } + cleanupStrategy.killPty(pid, pty); } catch { // ignore } } // Cleanup child processes - 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 - } - } - } + cleanupStrategy.killChildProcesses(this.activeChildProcesses); } static { From b272ac0119e930c568eb1869770dfa0b9d3b9204 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Fri, 12 Dec 2025 17:47:03 +0800 Subject: [PATCH 08/12] Fix: Make cleanup strategy dynamic to support testing mocks --- packages/core/src/services/shellExecutionService.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index f78094329..853a4c89f 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -137,7 +137,7 @@ const posixStrategy: ProcessCleanupStrategy = { }, }; -const cleanupStrategy = +const getCleanupStrategy = () => os.platform() === 'win32' ? windowsStrategy : posixStrategy; /** @@ -151,17 +151,18 @@ export class ShellExecutionService { private static activeChildProcesses = new Set(); static cleanup() { + const strategy = getCleanupStrategy(); // Cleanup PTYs for (const [pid, pty] of this.activePtys) { try { - cleanupStrategy.killPty(pid, pty); + strategy.killPty(pid, pty); } catch { // ignore } } // Cleanup child processes - cleanupStrategy.killChildProcesses(this.activeChildProcesses); + strategy.killChildProcesses(this.activeChildProcesses); } static { From 8673426d5c2847a1bf4a3392ce7fa2816263e077 Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Tue, 16 Dec 2025 10:26:20 +0800 Subject: [PATCH 09/12] fix(core): use current chunk for shell output update instead of cumulative --- packages/core/src/tools/shell.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index a886010d9..194de4e86 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -193,11 +193,7 @@ export class ShellToolInvocation extends BaseToolInvocation< switch (event.type) { case 'data': if (isBinaryStream) break; - if (typeof cumulativeOutput === 'string') { - cumulativeOutput += event.chunk; - } else { - cumulativeOutput = event.chunk; - } + cumulativeOutput = event.chunk; shouldUpdate = true; break; case 'binary_detected': From f610133660f2ff6d2c5006bf30bbe2108eca1d29 Mon Sep 17 00:00:00 2001 From: cris Date: Sun, 28 Dec 2025 22:14:16 +0800 Subject: [PATCH 10/12] improve ad hoc method for windows background terminal task --- .../src/services/shellExecutionService.ts | 2 +- packages/core/src/tools/shell.ts | 89 ++++++++----------- 2 files changed, 37 insertions(+), 54 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 853a4c89f..c870b5f4e 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -229,7 +229,7 @@ export class ShellExecutionService { stdio: ['ignore', 'pipe', 'pipe'], windowsVerbatimArguments: true, shell: isWindows ? true : 'bash', - detached: !isWindows, + detached: true, env: { ...process.env, QWEN_CODE: '1', diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index c223d0e5f..b4cbb195b 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -30,7 +30,6 @@ 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'; @@ -159,12 +158,7 @@ export class ShellToolInvocation extends BaseToolInvocation< // On Windows, we rely on the race logic below to handle background tasks. // We just ensure the command string is clean. if (isWindows && shouldRunInBackground) { - 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; + finalCommand = finalCommand.trim().replace(/&+$/, '').trim(); } // pgrep is not available on Windows, so we can't get background PIDs @@ -234,60 +228,49 @@ export class ShellToolInvocation extends BaseToolInvocation< setPidCallback(pid); } - 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 (shouldRunInBackground) { + // Check for obvious startup errors from captured output + const outputStr = + typeof cumulativeOutput === 'string' + ? cumulativeOutput + : JSON.stringify(cumulativeOutput); - if (raceResult === null) { - // Timeout reached, process is still running. - // throw new Error(`DEBUG: raceResult is null. Output: ${JSON.stringify(cumulativeOutput)}`); + const errorPatterns = [ + 'is not recognized as an internal or external command', + 'The system cannot find the path specified', + 'Access is denied', + 'command not found', + 'No such file or directory', + 'Permission denied', + ]; - // Check for common Windows error messages in the output - const outputStr = - typeof cumulativeOutput === 'string' - ? cumulativeOutput - : JSON.stringify(cumulativeOutput); - console.log('DEBUG: outputStr:', outputStr); - const errorPatterns = [ - 'is not recognized as an internal or external command', - 'The system cannot find the path specified', - 'Access is denied', - ]; + const hasEarlyError = errorPatterns.some((pat) => + outputStr.includes(pat), + ); - if (errorPatterns.some((pattern) => outputStr.includes(pattern))) { - return { - llmContent: `Command failed to start: ${outputStr}`, - returnDisplay: `Command failed to start: ${outputStr}`, - error: { - type: ToolErrorType.EXECUTION_FAILED, - message: `Command failed to start: ${outputStr}`, - }, - }; - } - - const pidMsg = pid ? ` PID: ${pid}` : ''; - const winHint = isWindows - ? ' (Note: Use taskkill /F /T /PID to stop)' - : ''; + if (hasEarlyError) { return { - llmContent: `Background command started.${pidMsg}${winHint}`, - returnDisplay: `Background command started.${pidMsg}${winHint}`, + llmContent: `Command failed to start: ${outputStr}`, + returnDisplay: `Command failed to start: ${outputStr}`, + error: { + type: ToolErrorType.EXECUTION_FAILED, + message: `Command failed to start: ${outputStr}`, + }, }; - } else { - result = raceResult; } - } else { - result = await resultPromise; + + 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}`, + }; } + const result = await resultPromise; + const backgroundPIDs: number[] = []; if (os.platform() !== 'win32') { if (fs.existsSync(tempFilePath)) { From 98c043bf50a969d8d4b4d46fff4878149a9c9a0e Mon Sep 17 00:00:00 2001 From: xuewenjie Date: Mon, 29 Dec 2025 11:37:54 +0800 Subject: [PATCH 11/12] test: update tests for detached process changes --- .../services/shellExecutionService.test.ts | 2 +- packages/core/src/tools/shell.test.ts | 27 ------------------- 2 files changed, 1 insertion(+), 28 deletions(-) diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index c5a6e0776..e63fba28d 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -829,7 +829,7 @@ describe('ShellExecutionService child_process fallback', () => { [], expect.objectContaining({ shell: true, - detached: false, + detached: true, }), ); }); diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index eb8a17418..8db33b563 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -960,32 +960,5 @@ spanning multiple lines"`; {}, ); }); - - it('should detect immediate failure in Windows background task', async () => { - vi.mocked(os.platform).mockReturnValue('win32'); - const mockAbortSignal = new AbortController().signal; - - const invocation = shellTool.build({ - command: 'invalid_command', - is_background: true, - }); - - const promise = invocation.execute(mockAbortSignal); - - // Wait a tick to ensure mockShellOutputCallback is assigned - await new Promise((resolve) => setTimeout(resolve, 0)); - - if (mockShellOutputCallback) { - mockShellOutputCallback({ - type: 'data', - chunk: - "'invalid_command' is not recognized as an internal or external command,\r\noperable program or batch file.\r\n", - }); - } - - const result = await promise; - expect(result.error).toBeDefined(); - expect(result.llmContent).toContain('Command failed to start'); - }); }); }); From e2d6ab9b7e5323c772c7a98afd5eec678fd2e375 Mon Sep 17 00:00:00 2001 From: xwj02155382 Date: Tue, 6 Jan 2026 16:46:56 +0800 Subject: [PATCH 12/12] refactor: simplify background shell command handling - Remove ineffective error detection for background processes (stdio is detached/ignored, so cumulativeOutput is always empty) - Add kill command hints for both Windows and macOS/Linux - Simplify code from 40 lines to 12 lines with clearer logic - Add explanatory comment about why startup errors cannot be reliably detected --- packages/core/src/tools/shell.ts | 44 +++++++------------------------- 1 file changed, 9 insertions(+), 35 deletions(-) diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index b4cbb195b..d7afae599 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -229,43 +229,17 @@ export class ShellToolInvocation extends BaseToolInvocation< } if (shouldRunInBackground) { - // Check for obvious startup errors from captured output - const outputStr = - typeof cumulativeOutput === 'string' - ? cumulativeOutput - : JSON.stringify(cumulativeOutput); - - const errorPatterns = [ - 'is not recognized as an internal or external command', - 'The system cannot find the path specified', - 'Access is denied', - 'command not found', - 'No such file or directory', - 'Permission denied', - ]; - - const hasEarlyError = errorPatterns.some((pat) => - outputStr.includes(pat), - ); - - if (hasEarlyError) { - return { - llmContent: `Command failed to start: ${outputStr}`, - returnDisplay: `Command failed to start: ${outputStr}`, - error: { - type: ToolErrorType.EXECUTION_FAILED, - message: `Command failed to start: ${outputStr}`, - }, - }; - } - + // For background tasks, return immediately with PID info + // Note: We cannot reliably detect startup errors for background processes + // since their stdio is typically detached/ignored const pidMsg = pid ? ` PID: ${pid}` : ''; - const winHint = isWindows - ? ' (Note: Use taskkill /F /T /PID to stop)' - : ''; + const killHint = isWindows + ? ' (Use taskkill /F /T /PID to stop)' + : ' (Use kill to stop)'; + return { - llmContent: `Background command started.${pidMsg}${winHint}`, - returnDisplay: `Background command started.${pidMsg}${winHint}`, + llmContent: `Background command started.${pidMsg}${killHint}`, + returnDisplay: `Background command started.${pidMsg}${killHint}`, }; }