Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/types/src/terminal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export const commandExecutionStatusSchema = z.discriminatedUnion("status", [
executionId: z.string(),
status: z.literal("fallback"),
}),
z.object({
executionId: z.string(),
status: z.literal("timeout"),
}),
])

export type CommandExecutionStatus = z.infer<typeof commandExecutionStatusSchema>
189 changes: 189 additions & 0 deletions src/core/tools/__tests__/executeCommandTimeout.integration.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
// 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(),
}),
},
say: vitest.fn().mockResolvedValue(undefined),
}

// 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")
})
})
43 changes: 43 additions & 0 deletions src/core/tools/__tests__/executeCommandTool.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// npx vitest run src/core/tools/__tests__/executeCommandTool.spec.ts

import type { ToolUsage } from "@roo-code/types"
import * as vscode from "vscode"

import { Task } from "../../task/Task"
import { formatResponse } from "../../prompts/responses"
Expand All @@ -12,6 +13,12 @@ vitest.mock("execa", () => ({
execa: vitest.fn(),
}))

vitest.mock("vscode", () => ({
workspace: {
getConfiguration: vitest.fn(),
},
}))

vitest.mock("../../task/Task")
vitest.mock("../../prompts/responses")

Expand Down Expand Up @@ -266,4 +273,40 @@ describe("executeCommandTool", () => {
expect(mockExecuteCommand).not.toHaveBeenCalled()
})
})

describe("Command execution timeout configuration", () => {
it("should include timeout parameter in ExecuteCommandOptions", () => {
// This test verifies that the timeout configuration is properly typed
// The actual timeout logic is tested in integration tests
// Note: timeout is stored internally in milliseconds but configured in seconds
const timeoutSeconds = 15
const options = {
executionId: "test-id",
command: "echo test",
commandExecutionTimeout: timeoutSeconds * 1000, // Convert to milliseconds
}

// Verify the options object has the expected structure
expect(options.commandExecutionTimeout).toBe(15000)
expect(typeof options.commandExecutionTimeout).toBe("number")
})

it("should handle timeout parameter in function signature", () => {
// Test that the executeCommand function accepts timeout parameter
// This is a compile-time check that the types are correct
const mockOptions = {
executionId: "test-id",
command: "echo test",
customCwd: undefined,
terminalShellIntegrationDisabled: false,
terminalOutputLineLimit: 500,
commandExecutionTimeout: 0,
}

// Verify all required properties exist
expect(mockOptions.executionId).toBeDefined()
expect(mockOptions.command).toBeDefined()
expect(mockOptions.commandExecutionTimeout).toBeDefined()
})
})
})
67 changes: 65 additions & 2 deletions src/core/tools/executeCommandTool.ts
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"

Expand All @@ -14,6 +15,8 @@ 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"
import { t } from "../../i18n"

class ShellIntegrationError extends Error {}

Expand Down Expand Up @@ -62,12 +65,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 {
Expand Down Expand Up @@ -113,6 +125,7 @@ export type ExecuteCommandOptions = {
customCwd?: string
terminalShellIntegrationDisabled?: boolean
terminalOutputLineLimit?: number
commandExecutionTimeout?: number
Copy link
Collaborator

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 executeCommand just exported to make the tests easier, or am I missing a way it's being called?)

}

export async function executeCommand(
Expand All @@ -123,8 +136,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) {
Expand Down Expand Up @@ -211,8 +227,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", t("common:command_timeout", { seconds: commandExecutionTimeoutSeconds }))

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.`,
]
}
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)
Expand Down
1 change: 1 addition & 0 deletions src/i18n/locales/ca/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
"url_page_not_found": "No s'ha trobat la pàgina. Comprova si la URL és correcta.",
"url_fetch_failed": "Error en obtenir el contingut de la URL: {{error}}",
"url_fetch_error_with_url": "Error en obtenir contingut per {{url}}: {{error}}",
"command_timeout": "L'execució de la comanda ha superat el temps d'espera de {{seconds}} segons",
"share_task_failed": "Ha fallat compartir la tasca. Si us plau, torna-ho a provar.",
"share_no_active_task": "No hi ha cap tasca activa per compartir",
"share_auth_required": "Es requereix autenticació. Si us plau, inicia sessió per compartir tasques.",
Expand Down
1 change: 1 addition & 0 deletions src/i18n/locales/de/common.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"url_page_not_found": "Die Seite wurde nicht gefunden. Bitte prüfe, ob die URL korrekt ist.",
"url_fetch_failed": "Fehler beim Abrufen des URL-Inhalts: {{error}}",
"url_fetch_error_with_url": "Fehler beim Abrufen des Inhalts für {{url}}: {{error}}",
"command_timeout": "Zeitüberschreitung bei der Befehlsausführung nach {{seconds}} Sekunden",
"share_task_failed": "Teilen der Aufgabe fehlgeschlagen. Bitte versuche es erneut.",
"share_no_active_task": "Keine aktive Aufgabe zum Teilen",
"share_auth_required": "Authentifizierung erforderlich. Bitte melde dich an, um Aufgaben zu teilen.",
Expand Down
Loading