-
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
Merged
mrubens
merged 11 commits into
main
from
feature/configurable-command-execution-timeout
Jul 14, 2025
Merged
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
8e1fe55
feat: add configurable timeout for command execution
roomote ac332f3
fix: resolve integration test failures for command timeout functionality
roomote 3df6abd
feat: change default command execution timeout to 0 (no timeout)
roomote b0a72dc
feat: change command execution timeout from milliseconds to seconds
roomote 7848709
feat: Add commandExecutionTimeout translations for all supported lang…
roomote 21997e6
Clearer error language
mrubens 8a7147c
fix: update command timeout to use seconds configuration
mrubens 224c366
feat: add visual feedback for command execution timeout
daniel-lxs c6d798a
feat: add visual feedback when command execution timeout is reached
daniel-lxs 680c783
feat: add internationalized visual feedback for command timeout
daniel-lxs dcca042
Update src/i18n/locales/id/common.json
daniel-lxs File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
188 changes: 188 additions & 0 deletions
188
src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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") | ||
| }) | ||
| }) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?)