-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Add configurable timeout for command execution #5668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 8 commits
8e1fe55
ac332f3
3df6abd
b0a72dc
7848709
21997e6
8a7147c
224c366
c6d798a
680c783
dcca042
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| // Integration tests for command execution timeout functionality | ||
| // npx vitest run src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts | ||
|
|
||
| import * as vscode from "vscode" | ||
| import * as fs from "fs/promises" | ||
| import { executeCommand, ExecuteCommandOptions } from "../executeCommandTool" | ||
| import { Task } from "../../task/Task" | ||
| import { TerminalRegistry } from "../../../integrations/terminal/TerminalRegistry" | ||
|
|
||
| // Mock dependencies | ||
| vitest.mock("vscode", () => ({ | ||
| workspace: { | ||
| getConfiguration: vitest.fn(), | ||
| }, | ||
| })) | ||
|
|
||
| vitest.mock("fs/promises") | ||
| vitest.mock("../../../integrations/terminal/TerminalRegistry") | ||
| vitest.mock("../../task/Task") | ||
|
|
||
| describe("Command Execution Timeout Integration", () => { | ||
| let mockTask: any | ||
| let mockTerminal: any | ||
| let mockProcess: any | ||
|
|
||
| beforeEach(() => { | ||
| vitest.clearAllMocks() | ||
|
|
||
| // Mock fs.access to resolve successfully for working directory | ||
| ;(fs.access as any).mockResolvedValue(undefined) | ||
|
|
||
| // Mock task | ||
| mockTask = { | ||
| cwd: "/test/directory", | ||
| terminalProcess: undefined, | ||
| providerRef: { | ||
| deref: vitest.fn().mockResolvedValue({ | ||
| postMessageToWebview: vitest.fn(), | ||
| }), | ||
| }, | ||
| } | ||
|
|
||
| // Mock terminal process | ||
| mockProcess = { | ||
| abort: vitest.fn(), | ||
| then: vitest.fn(), | ||
| catch: vitest.fn(), | ||
| } | ||
|
|
||
| // Mock terminal | ||
| mockTerminal = { | ||
| runCommand: vitest.fn().mockReturnValue(mockProcess), | ||
| getCurrentWorkingDirectory: vitest.fn().mockReturnValue("/test/directory"), | ||
| } | ||
|
|
||
| // Mock TerminalRegistry | ||
| ;(TerminalRegistry.getOrCreateTerminal as any).mockResolvedValue(mockTerminal) | ||
|
|
||
| // Mock VSCode configuration | ||
| const mockGetConfiguration = vitest.fn().mockReturnValue({ | ||
| get: vitest.fn().mockReturnValue(0), // Default 0 (no timeout) | ||
| }) | ||
| ;(vscode.workspace.getConfiguration as any).mockReturnValue(mockGetConfiguration()) | ||
| }) | ||
|
|
||
| it("should pass timeout configuration to executeCommand", async () => { | ||
| const customTimeoutMs = 15000 // 15 seconds in milliseconds | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "echo test", | ||
| commandExecutionTimeout: customTimeoutMs, | ||
| } | ||
|
|
||
| // Mock a quick-completing process | ||
| const quickProcess = Promise.resolve() | ||
| mockTerminal.runCommand.mockReturnValue(quickProcess) | ||
|
|
||
| await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Verify that the terminal was called with the command | ||
| expect(mockTerminal.runCommand).toHaveBeenCalledWith("echo test", expect.any(Object)) | ||
| }) | ||
|
|
||
| it("should handle timeout scenario", async () => { | ||
| const shortTimeoutMs = 100 // Very short timeout in milliseconds | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "sleep 10", | ||
| commandExecutionTimeout: shortTimeoutMs, | ||
| } | ||
|
|
||
| // Create a process that never resolves but has an abort method | ||
| const longRunningProcess = new Promise(() => { | ||
| // Never resolves to simulate a hanging command | ||
| }) | ||
|
|
||
| // Add abort method to the promise | ||
| ;(longRunningProcess as any).abort = vitest.fn() | ||
|
|
||
| mockTerminal.runCommand.mockReturnValue(longRunningProcess) | ||
|
|
||
| // Execute with timeout | ||
| const result = await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Should return timeout error | ||
| expect(result[0]).toBe(false) // Not rejected by user | ||
| expect(result[1]).toContain("terminated after exceeding") | ||
| expect(result[1]).toContain("0.1s") // Should show seconds in error message | ||
| }, 10000) // Increase test timeout to 10 seconds | ||
|
|
||
| it("should abort process on timeout", async () => { | ||
| const shortTimeoutMs = 50 // Short timeout in milliseconds | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "sleep 10", | ||
| commandExecutionTimeout: shortTimeoutMs, | ||
| } | ||
|
|
||
| // Create a process that can be aborted | ||
| const abortSpy = vitest.fn() | ||
|
|
||
| // Mock the process to never resolve but be abortable | ||
| const neverResolvingPromise = new Promise(() => {}) | ||
| ;(neverResolvingPromise as any).abort = abortSpy | ||
|
|
||
| mockTerminal.runCommand.mockReturnValue(neverResolvingPromise) | ||
|
|
||
| await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Verify abort was called | ||
| expect(abortSpy).toHaveBeenCalled() | ||
| }, 5000) // Increase test timeout to 5 seconds | ||
|
|
||
| it("should clean up timeout on successful completion", async () => { | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "echo test", | ||
| commandExecutionTimeout: 5000, | ||
| } | ||
|
|
||
| // Mock a process that completes quickly | ||
| const quickProcess = Promise.resolve() | ||
| mockTerminal.runCommand.mockReturnValue(quickProcess) | ||
|
|
||
| const result = await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Should complete successfully without timeout | ||
| expect(result[0]).toBe(false) // Not rejected | ||
| expect(result[1]).not.toContain("terminated after exceeding") | ||
| }) | ||
|
|
||
| it("should use default timeout when not specified (0 = no timeout)", async () => { | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "echo test", | ||
| // commandExecutionTimeout not specified, should use default (0) | ||
| } | ||
|
|
||
| const quickProcess = Promise.resolve() | ||
| mockTerminal.runCommand.mockReturnValue(quickProcess) | ||
|
|
||
| await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Should complete without issues using default (no timeout) | ||
| expect(mockTerminal.runCommand).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it("should not timeout when commandExecutionTimeout is 0", async () => { | ||
| const options: ExecuteCommandOptions = { | ||
| executionId: "test-execution", | ||
| command: "sleep 10", | ||
| commandExecutionTimeout: 0, // No timeout | ||
| } | ||
|
|
||
| // Create a process that resolves after a delay to simulate a long-running command | ||
| const longRunningProcess = new Promise((resolve) => { | ||
| setTimeout(resolve, 200) // 200ms delay | ||
| }) | ||
|
|
||
| mockTerminal.runCommand.mockReturnValue(longRunningProcess) | ||
|
|
||
| const result = await executeCommand(mockTask as Task, options) | ||
|
|
||
| // Should complete successfully without timeout | ||
| expect(result[0]).toBe(false) // Not rejected | ||
| expect(result[1]).not.toContain("terminated after exceeding") | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import fs from "fs/promises" | ||
| import * as path from "path" | ||
| import * as vscode from "vscode" | ||
|
|
||
| import delay from "delay" | ||
|
|
||
|
|
@@ -14,6 +15,7 @@ import { unescapeHtmlEntities } from "../../utils/text-normalization" | |
| import { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcess } from "../../integrations/terminal/types" | ||
| import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry" | ||
| import { Terminal } from "../../integrations/terminal/Terminal" | ||
| import { Package } from "../../shared/package" | ||
|
|
||
| class ShellIntegrationError extends Error {} | ||
|
|
||
|
|
@@ -62,12 +64,21 @@ export async function executeCommandTool( | |
| const clineProviderState = await clineProvider?.getState() | ||
| const { terminalOutputLineLimit = 500, terminalShellIntegrationDisabled = false } = clineProviderState ?? {} | ||
|
|
||
| // Get command execution timeout from VSCode configuration (in seconds) | ||
| const commandExecutionTimeoutSeconds = vscode.workspace | ||
| .getConfiguration(Package.name) | ||
| .get<number>("commandExecutionTimeout", 0) | ||
|
|
||
| // Convert seconds to milliseconds for internal use | ||
| const commandExecutionTimeout = commandExecutionTimeoutSeconds * 1000 | ||
|
|
||
| const options: ExecuteCommandOptions = { | ||
| executionId, | ||
| command, | ||
| customCwd, | ||
| terminalShellIntegrationDisabled, | ||
| terminalOutputLineLimit, | ||
| commandExecutionTimeout, | ||
| } | ||
|
|
||
| try { | ||
|
|
@@ -113,6 +124,7 @@ export type ExecuteCommandOptions = { | |
| customCwd?: string | ||
| terminalShellIntegrationDisabled?: boolean | ||
| terminalOutputLineLimit?: number | ||
| commandExecutionTimeout?: number | ||
| } | ||
|
|
||
| export async function executeCommand( | ||
|
|
@@ -123,8 +135,11 @@ export async function executeCommand( | |
| customCwd, | ||
| terminalShellIntegrationDisabled = false, | ||
| terminalOutputLineLimit = 500, | ||
| commandExecutionTimeout = 0, | ||
| }: ExecuteCommandOptions, | ||
| ): Promise<[boolean, ToolResponse]> { | ||
| // Convert milliseconds back to seconds for display purposes | ||
| const commandExecutionTimeoutSeconds = commandExecutionTimeout / 1000 | ||
| let workingDir: string | ||
|
|
||
| if (!customCwd) { | ||
|
|
@@ -211,8 +226,55 @@ export async function executeCommand( | |
| const process = terminal.runCommand(command, callbacks) | ||
| cline.terminalProcess = process | ||
|
|
||
| await process | ||
| cline.terminalProcess = undefined | ||
| // Implement command execution timeout (skip if timeout is 0) | ||
| if (commandExecutionTimeout > 0) { | ||
| let timeoutId: NodeJS.Timeout | undefined | ||
| let isTimedOut = false | ||
|
|
||
| const timeoutPromise = new Promise<void>((_, reject) => { | ||
| timeoutId = setTimeout(() => { | ||
| isTimedOut = true | ||
| // Try to abort the process | ||
| if (cline.terminalProcess) { | ||
| cline.terminalProcess.abort() | ||
| } | ||
| reject(new Error(`Command execution timed out after ${commandExecutionTimeout}ms`)) | ||
| }, commandExecutionTimeout) | ||
| }) | ||
|
|
||
| try { | ||
| await Promise.race([process, timeoutPromise]) | ||
| } catch (error) { | ||
| if (isTimedOut) { | ||
| // Handle timeout case | ||
| const status: CommandExecutionStatus = { executionId, status: "timeout" } | ||
| clineProvider?.postMessageToWebview({ type: "commandExecutionStatus", text: JSON.stringify(status) }) | ||
|
|
||
| // Add visual feedback for timeout | ||
| await cline.say("text", `Command execution timed out after ${commandExecutionTimeoutSeconds} seconds`) | ||
|
||
|
|
||
| cline.terminalProcess = undefined | ||
|
|
||
| return [ | ||
| false, | ||
| `The command was terminated after exceeding a user-configured ${commandExecutionTimeoutSeconds}s timeout. Do not try to re-run the command.`, | ||
mrubens marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ] | ||
| } | ||
| throw error | ||
| } finally { | ||
| if (timeoutId) { | ||
| clearTimeout(timeoutId) | ||
| } | ||
| cline.terminalProcess = undefined | ||
| } | ||
| } else { | ||
| // No timeout - just wait for the process to complete | ||
| try { | ||
| await process | ||
| } finally { | ||
| cline.terminalProcess = undefined | ||
| } | ||
| } | ||
|
|
||
| if (shellIntegrationError) { | ||
| throw new ShellIntegrationError(shellIntegrationError) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ever actually undefined? (Somewhat relatedly: is
executeCommandjust exported to make the tests easier, or am I missing a way it's being called?)