From 0d29385e1bdf0f73e663df490a1b88ed3117ae16 Mon Sep 17 00:00:00 2001 From: Gal Zahavi <38544478+galz10@users.noreply.github.com> Date: Wed, 26 Nov 2025 13:43:33 -0800 Subject: [PATCH] feat(core): Add configurable inactivity timeout for shell commands (#13531) --- docs/get-started/configuration.md | 5 +++ packages/cli/src/config/config.ts | 1 + packages/cli/src/config/settings.ts | 1 + packages/cli/src/config/settingsSchema.ts | 10 +++++ packages/core/src/config/config.test.ts | 16 ++++++++ packages/core/src/config/config.ts | 8 ++++ packages/core/src/tools/shell.test.ts | 33 +++++++++++++-- packages/core/src/tools/shell.ts | 50 +++++++++++++++++++++-- schemas/settings.schema.json | 7 ++++ 9 files changed, 124 insertions(+), 7 deletions(-) diff --git a/docs/get-started/configuration.md b/docs/get-started/configuration.md index 7a1ad4475b..3d067e408c 100644 --- a/docs/get-started/configuration.md +++ b/docs/get-started/configuration.md @@ -600,6 +600,11 @@ their corresponding top-level category object in your `settings.json` file. - **Description:** Show color in shell output. - **Default:** `false` +- **`tools.shell.inactivityTimeout`** (number): + - **Description:** The maximum time in seconds allowed without output from the + shell command. Defaults to 5 minutes. + - **Default:** `300` + - **`tools.autoAccept`** (boolean): - **Description:** Automatically accept and execute tool calls that are considered safe (e.g., read-only operations). diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 4ca14e4dc6..7e4f1cd4d0 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -643,6 +643,7 @@ export async function loadCliConfig( useRipgrep: settings.tools?.useRipgrep, enableInteractiveShell: settings.tools?.shell?.enableInteractiveShell ?? true, + shellToolInactivityTimeout: settings.tools?.shell?.inactivityTimeout, skipNextSpeakerCheck: settings.model?.skipNextSpeakerCheck, enablePromptCompletion: settings.general?.enablePromptCompletion ?? false, truncateToolOutputThreshold: settings.tools?.truncateToolOutputThreshold, diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 947a234bcf..4250bd30d0 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -116,6 +116,7 @@ const MIGRATION_MAP: Record = { enableInteractiveShell: 'tools.shell.enableInteractiveShell', shellPager: 'tools.shell.pager', shellShowColor: 'tools.shell.showColor', + shellInactivityTimeout: 'tools.shell.inactivityTimeout', skipNextSpeakerCheck: 'model.skipNextSpeakerCheck', summarizeToolOutput: 'model.summarizeToolOutput', telemetry: 'telemetry', diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index 56d9333c2f..d098b8cd88 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -930,6 +930,16 @@ const SETTINGS_SCHEMA = { description: 'Show color in shell output.', showInDialog: true, }, + inactivityTimeout: { + type: 'number', + label: 'Inactivity Timeout', + category: 'Tools', + requiresRestart: false, + default: 300, + description: + 'The maximum time in seconds allowed without output from the shell command. Defaults to 5 minutes.', + showInDialog: false, + }, }, }, autoAccept: { diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index a3dda9371d..fd97a2aaa6 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -750,6 +750,22 @@ describe('Server Config (config.ts)', () => { }); }); + describe('Shell Tool Inactivity Timeout', () => { + it('should default to 300000ms (300 seconds) when not provided', () => { + const config = new Config(baseParams); + expect(config.getShellToolInactivityTimeout()).toBe(300000); + }); + + it('should convert provided seconds to milliseconds', () => { + const params: ConfigParameters = { + ...baseParams, + shellToolInactivityTimeout: 10, // 10 seconds + }; + const config = new Config(params); + expect(config.getShellToolInactivityTimeout()).toBe(10000); + }); + }); + describe('ContinueOnFailedApiCall Configuration', () => { it('should default continueOnFailedApiCall to false when not provided', () => { const config = new Config(baseParams); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 55a264d525..5e25ae1ae1 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -297,6 +297,7 @@ export interface ConfigParameters { continueOnFailedApiCall?: boolean; retryFetchErrors?: boolean; enableShellOutputEfficiency?: boolean; + shellToolInactivityTimeout?: number; fakeResponses?: string; recordResponses?: string; ptyInfo?: string; @@ -411,6 +412,7 @@ export class Config { private readonly continueOnFailedApiCall: boolean; private readonly retryFetchErrors: boolean; private readonly enableShellOutputEfficiency: boolean; + private readonly shellToolInactivityTimeout: number; readonly fakeResponses?: string; readonly recordResponses?: string; private readonly disableYoloMode: boolean; @@ -547,6 +549,8 @@ export class Config { this.continueOnFailedApiCall = params.continueOnFailedApiCall ?? true; this.enableShellOutputEfficiency = params.enableShellOutputEfficiency ?? true; + this.shellToolInactivityTimeout = + (params.shellToolInactivityTimeout ?? 300) * 1000; // 5 minutes this.extensionManagement = params.extensionManagement ?? true; this.enableExtensionReloading = params.enableExtensionReloading ?? false; this.storage = new Storage(this.targetDir); @@ -1308,6 +1312,10 @@ export class Config { return this.enableShellOutputEfficiency; } + getShellToolInactivityTimeout(): number { + return this.shellToolInactivityTimeout; + } + getShellExecutionConfig(): ShellExecutionConfig { return this.shellExecutionConfig; } diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 21b027cefd..6535f05648 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -92,6 +92,7 @@ describe('ShellTool', () => { getGeminiClient: vi.fn(), getEnableInteractiveShell: vi.fn().mockReturnValue(false), isInteractive: vi.fn().mockReturnValue(true), + getShellToolInactivityTimeout: vi.fn().mockReturnValue(300000), } as unknown as Config; shellTool = new ShellTool(mockConfig); @@ -219,7 +220,7 @@ describe('ShellTool', () => { wrappedCommand, tempRootDir, expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -244,7 +245,7 @@ describe('ShellTool', () => { wrappedCommand, subdir, expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -265,7 +266,7 @@ describe('ShellTool', () => { wrappedCommand, path.join(tempRootDir, 'subdir'), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -292,7 +293,7 @@ describe('ShellTool', () => { 'dir', tempRootDir, expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -376,6 +377,30 @@ describe('ShellTool', () => { expect(result.returnDisplay).toBe('long output'); }); + it('should NOT start a timeout if timeoutMs is <= 0', async () => { + // Mock the timeout config to be 0 + (mockConfig.getShellToolInactivityTimeout as Mock).mockReturnValue(0); + + vi.useFakeTimers(); + + const invocation = shellTool.build({ command: 'sleep 10' }); + const promise = invocation.execute(mockAbortSignal); + + // Verify no timeout logic is triggered even after a long time + resolveShellExecution({ + output: 'finished', + exitCode: 0, + }); + + await promise; + // If we got here without aborting/timing out logic interfering, we're good. + // We can also verify that setTimeout was NOT called for the inactivity timeout. + // However, since we don't have direct access to the internal `resetTimeout`, + // we can infer success by the fact it didn't abort. + + vi.useRealTimers(); + }); + it('should clean up the temp file on synchronous execution error', async () => { const error = new Error('sync spawn error'); mockShellExecutionService.mockImplementation(() => { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 99fd7cd612..482f43caf4 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -150,6 +150,15 @@ export class ShellToolInvocation extends BaseToolInvocation< .toString('hex')}.tmp`; const tempFilePath = path.join(os.tmpdir(), tempFileName); + const timeoutMs = this.config.getShellToolInactivityTimeout(); + const timeoutController = new AbortController(); + let timeoutTimer: NodeJS.Timeout | undefined; + + // Handle signal combination manually to avoid TS issues or runtime missing features + const combinedController = new AbortController(); + + const onAbort = () => combinedController.abort(); + try { // pgrep is not available on Windows, so we can't get background PIDs const commandToExecute = isWindows @@ -169,11 +178,30 @@ export class ShellToolInvocation extends BaseToolInvocation< let lastUpdateTime = Date.now(); let isBinaryStream = false; + const resetTimeout = () => { + if (timeoutMs <= 0) { + return; + } + if (timeoutTimer) clearTimeout(timeoutTimer); + timeoutTimer = setTimeout(() => { + timeoutController.abort(); + }, timeoutMs); + }; + + signal.addEventListener('abort', onAbort, { once: true }); + timeoutController.signal.addEventListener('abort', onAbort, { + once: true, + }); + + // Start timeout + resetTimeout(); + const { result: resultPromise, pid } = await ShellExecutionService.execute( commandToExecute, cwd, (event: ShellOutputEvent) => { + resetTimeout(); // Reset timeout on any event if (!updateOutput) { return; } @@ -211,7 +239,7 @@ export class ShellToolInvocation extends BaseToolInvocation< lastUpdateTime = Date.now(); } }, - signal, + combinedController.signal, this.config.getEnableInteractiveShell(), shellExecutionConfig ?? {}, ); @@ -246,8 +274,17 @@ export class ShellToolInvocation extends BaseToolInvocation< } let llmContent = ''; + let timeoutMessage = ''; if (result.aborted) { - llmContent = 'Command was cancelled by user before it could complete.'; + if (timeoutController.signal.aborted) { + timeoutMessage = `Command was automatically cancelled because it exceeded the timeout of ${( + timeoutMs / 60000 + ).toFixed(1)} minutes without output.`; + llmContent = timeoutMessage; + } else { + llmContent = + 'Command was cancelled by user before it could complete.'; + } if (result.output.trim()) { llmContent += ` Below is the output before it was cancelled:\n${result.output}`; } else { @@ -282,7 +319,11 @@ export class ShellToolInvocation extends BaseToolInvocation< returnDisplayMessage = result.output; } else { if (result.aborted) { - returnDisplayMessage = 'Command cancelled by user.'; + if (timeoutMessage) { + returnDisplayMessage = timeoutMessage; + } else { + returnDisplayMessage = 'Command cancelled by user.'; + } } else if (result.signal) { returnDisplayMessage = `Command terminated by signal: ${result.signal}`; } else if (result.error) { @@ -327,6 +368,9 @@ export class ShellToolInvocation extends BaseToolInvocation< ...executionError, }; } finally { + if (timeoutTimer) clearTimeout(timeoutTimer); + signal.removeEventListener('abort', onAbort); + timeoutController.signal.removeEventListener('abort', onAbort); if (fs.existsSync(tempFilePath)) { fs.unlinkSync(tempFilePath); } diff --git a/schemas/settings.schema.json b/schemas/settings.schema.json index 0ebe3e3a33..c57ba6fed9 100644 --- a/schemas/settings.schema.json +++ b/schemas/settings.schema.json @@ -993,6 +993,13 @@ "markdownDescription": "Show color in shell output.\n\n- Category: `Tools`\n- Requires restart: `no`\n- Default: `false`", "default": false, "type": "boolean" + }, + "inactivityTimeout": { + "title": "Inactivity Timeout", + "description": "The maximum time in seconds allowed without output from the shell command. Defaults to 5 minutes.", + "markdownDescription": "The maximum time in seconds allowed without output from the shell command. Defaults to 5 minutes.\n\n- Category: `Tools`\n- Requires restart: `no`\n- Default: `300`", + "default": 300, + "type": "number" } }, "additionalProperties": false