mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-06 08:18:39 +00:00
feat: detect unbalanced quotes in terminal commands to prevent hanging
When a command has unbalanced quotes (missing closing quote), the shell enters continuation mode waiting for more input. Since stdin is ignored in non-interactive mode, this causes the terminal to hang indefinitely. This change: - Adds validateCommandQuotes() function in parse-command.ts to detect unbalanced single and double quotes before command execution - Correctly handles escaped quotes (\" and \') and escaped backslashes (\\) - Handles special cases like heredocs (<<EOF) and ANSI-C quoting ($'...') - Integrates validation into ExecuteCommandTool.ts to reject commands with unbalanced quotes with a helpful error message - Adds comprehensive tests for both the validation function and integration Resolves EXT-737
This commit is contained in:
parent
1da2b1c457
commit
05bf2706ef
4 changed files with 444 additions and 0 deletions
|
|
@ -12,6 +12,7 @@ import { Task } from "../task/Task"
|
|||
import { ToolUse, ToolResponse } from "../../shared/tools"
|
||||
import { formatResponse } from "../prompts/responses"
|
||||
import { unescapeHtmlEntities } from "../../utils/text-normalization"
|
||||
import { validateCommandQuotes } from "../../shared/parse-command"
|
||||
import { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcess } from "../../integrations/terminal/types"
|
||||
import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry"
|
||||
import { Terminal } from "../../integrations/terminal/Terminal"
|
||||
|
|
@ -54,6 +55,21 @@ export class ExecuteCommandTool extends BaseTool<"execute_command"> {
|
|||
task.consecutiveMistakeCount = 0
|
||||
|
||||
const unescapedCommand = unescapeHtmlEntities(command)
|
||||
|
||||
// Validate quotes before executing the command to prevent terminal from getting stuck
|
||||
const quoteValidation = validateCommandQuotes(unescapedCommand)
|
||||
if (!quoteValidation.valid) {
|
||||
task.consecutiveMistakeCount++
|
||||
task.recordToolError("execute_command")
|
||||
const errorMessage =
|
||||
`Command has unbalanced ${quoteValidation.quoteType} quotes at position ${quoteValidation.position}. ` +
|
||||
`Context: "${quoteValidation.context}". ` +
|
||||
`This would cause the terminal to hang waiting for the closing quote. ` +
|
||||
`Please fix the command and try again.`
|
||||
pushToolResult(errorMessage)
|
||||
return
|
||||
}
|
||||
|
||||
const didApprove = await askApproval("command", unescapedCommand)
|
||||
|
||||
if (!didApprove) {
|
||||
|
|
|
|||
|
|
@ -251,6 +251,78 @@ describe("executeCommandTool", () => {
|
|||
})
|
||||
})
|
||||
|
||||
describe("Unbalanced quote validation", () => {
|
||||
it("should reject commands with unbalanced double quotes", async () => {
|
||||
// Setup - command with missing closing double quote
|
||||
mockToolUse.params.command = 'echo "hello'
|
||||
mockToolUse.nativeArgs = { command: 'echo "hello' }
|
||||
|
||||
// Execute
|
||||
await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
|
||||
askApproval: mockAskApproval as unknown as AskApproval,
|
||||
handleError: mockHandleError as unknown as HandleError,
|
||||
pushToolResult: mockPushToolResult as unknown as PushToolResult,
|
||||
})
|
||||
|
||||
// Verify - should record error and push error result
|
||||
expect(mockCline.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockCline.recordToolError).toHaveBeenCalledWith("execute_command")
|
||||
expect(mockPushToolResult).toHaveBeenCalled()
|
||||
const result = mockPushToolResult.mock.calls[0][0]
|
||||
expect(result).toContain("unbalanced")
|
||||
expect(result).toContain("double")
|
||||
expect(result).toContain("quotes")
|
||||
// Should not reach approval step
|
||||
expect(mockAskApproval).not.toHaveBeenCalled()
|
||||
// executeCommandInTerminal should not be called
|
||||
expect(executeCommandModule.executeCommandInTerminal).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should reject commands with unbalanced single quotes", async () => {
|
||||
// Setup - command with missing closing single quote
|
||||
mockToolUse.params.command = "echo 'hello"
|
||||
mockToolUse.nativeArgs = { command: "echo 'hello" }
|
||||
|
||||
// Execute
|
||||
await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
|
||||
askApproval: mockAskApproval as unknown as AskApproval,
|
||||
handleError: mockHandleError as unknown as HandleError,
|
||||
pushToolResult: mockPushToolResult as unknown as PushToolResult,
|
||||
})
|
||||
|
||||
// Verify - should record error and push error result
|
||||
expect(mockCline.consecutiveMistakeCount).toBe(1)
|
||||
expect(mockCline.recordToolError).toHaveBeenCalledWith("execute_command")
|
||||
expect(mockPushToolResult).toHaveBeenCalled()
|
||||
const result = mockPushToolResult.mock.calls[0][0]
|
||||
expect(result).toContain("unbalanced")
|
||||
expect(result).toContain("single")
|
||||
expect(result).toContain("quotes")
|
||||
// Should not reach approval step
|
||||
expect(mockAskApproval).not.toHaveBeenCalled()
|
||||
// executeCommandInTerminal should not be called
|
||||
expect(executeCommandModule.executeCommandInTerminal).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it("should accept commands with balanced quotes", async () => {
|
||||
// Setup - command with properly balanced quotes
|
||||
mockToolUse.params.command = 'echo "hello world"'
|
||||
mockToolUse.nativeArgs = { command: 'echo "hello world"' }
|
||||
|
||||
// Execute
|
||||
await executeCommandTool.handle(mockCline as unknown as Task, mockToolUse, {
|
||||
askApproval: mockAskApproval as unknown as AskApproval,
|
||||
handleError: mockHandleError as unknown as HandleError,
|
||||
pushToolResult: mockPushToolResult as unknown as PushToolResult,
|
||||
})
|
||||
|
||||
// Verify - should proceed to approval
|
||||
expect(mockAskApproval).toHaveBeenCalled()
|
||||
// Should not record an error for quote validation
|
||||
expect(mockCline.recordToolError).not.toHaveBeenCalledWith("execute_command")
|
||||
})
|
||||
})
|
||||
|
||||
describe("Command execution timeout configuration", () => {
|
||||
it("should include timeout parameter in ExecuteCommandOptions", () => {
|
||||
// This test verifies that the timeout configuration is properly typed
|
||||
|
|
|
|||
197
src/shared/__tests__/parse-command.spec.ts
Normal file
197
src/shared/__tests__/parse-command.spec.ts
Normal file
|
|
@ -0,0 +1,197 @@
|
|||
// npx vitest run src/shared/__tests__/parse-command.spec.ts
|
||||
|
||||
import { describe, it, expect } from "vitest"
|
||||
import { validateCommandQuotes } from "../parse-command"
|
||||
|
||||
describe("validateCommandQuotes", () => {
|
||||
describe("valid commands", () => {
|
||||
it("should accept empty or whitespace-only commands", () => {
|
||||
expect(validateCommandQuotes("")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes(" ")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("\t\n")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept commands without quotes", () => {
|
||||
expect(validateCommandQuotes("echo hello")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("ls -la")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("npm install")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept commands with balanced double quotes", () => {
|
||||
expect(validateCommandQuotes('echo "hello world"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('echo "hello" && echo "world"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('cat "file with spaces.txt"')).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept commands with balanced single quotes", () => {
|
||||
expect(validateCommandQuotes("echo 'hello world'")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("echo 'hello' && echo 'world'")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("cat 'file with spaces.txt'")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept commands with mixed balanced quotes", () => {
|
||||
expect(validateCommandQuotes(`echo "it's working"`)).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes(`echo 'say "hello"'`)).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes(`git commit -m "it's a 'test'"`)).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept commands with escaped double quotes", () => {
|
||||
expect(validateCommandQuotes('echo "hello \\"world\\""')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('echo "test \\"nested\\" value"')).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept heredoc syntax", () => {
|
||||
expect(validateCommandQuotes("cat <<EOF")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("cat <<-EOF")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("cat <<'EOF'")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('cat <<"EOF"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("cat << EOF")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("cat <<MARKER")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept ANSI-C quoting", () => {
|
||||
expect(validateCommandQuotes("echo $'hello\\nworld'")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("echo $'tab\\there'")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept complex real-world commands", () => {
|
||||
expect(validateCommandQuotes('git log --oneline --format="%h %s"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("find . -name '*.ts' -exec cat {} \\;")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('docker run -e "VAR=value" image')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("ssh user@host 'ls -la'")).toEqual({ valid: true })
|
||||
})
|
||||
})
|
||||
|
||||
describe("invalid commands with unbalanced quotes", () => {
|
||||
it("should detect unbalanced double quotes at the start", () => {
|
||||
const result = validateCommandQuotes('echo "hello')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
expect(result.position).toBe(5)
|
||||
}
|
||||
})
|
||||
|
||||
it("should detect unbalanced double quotes in the middle", () => {
|
||||
const result = validateCommandQuotes('echo hello && cat "file.txt')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
}
|
||||
})
|
||||
|
||||
it("should detect unbalanced single quotes at the start", () => {
|
||||
const result = validateCommandQuotes("echo 'hello")
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("single")
|
||||
expect(result.position).toBe(5)
|
||||
}
|
||||
})
|
||||
|
||||
it("should detect unbalanced single quotes in the middle", () => {
|
||||
const result = validateCommandQuotes("echo hello && cat 'file.txt")
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("single")
|
||||
}
|
||||
})
|
||||
|
||||
it("should detect odd number of double quotes", () => {
|
||||
const result = validateCommandQuotes('echo "hello" "world')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
}
|
||||
})
|
||||
|
||||
it("should detect odd number of single quotes", () => {
|
||||
const result = validateCommandQuotes("echo 'hello' 'world")
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("single")
|
||||
}
|
||||
})
|
||||
|
||||
it("should provide context in error result", () => {
|
||||
const result = validateCommandQuotes('echo "hello world')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.context).toContain('"')
|
||||
expect(result.context.length).toBeLessThanOrEqual(30) // Context is bounded
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe("edge cases", () => {
|
||||
it("should handle quotes at the very end of command", () => {
|
||||
const result = validateCommandQuotes('echo test"')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
}
|
||||
})
|
||||
|
||||
it("should handle consecutive quotes", () => {
|
||||
expect(validateCommandQuotes('echo ""')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("echo ''")).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes(`echo """"""`)).toEqual({ valid: true }) // Three pairs
|
||||
})
|
||||
|
||||
it("should handle backslash escaping the closing quote (unbalanced)", () => {
|
||||
// In shell, \" escapes the quote, so echo "test\" is unbalanced
|
||||
// The string `echo "test\"` has an escaped quote, leaving it unclosed
|
||||
const result = validateCommandQuotes('echo "test\\"')
|
||||
expect(result.valid).toBe(false)
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
}
|
||||
})
|
||||
|
||||
it("should handle literal backslash followed by closing quote (balanced)", () => {
|
||||
// To have a backslash at the end of a quoted string, you need \\"
|
||||
// In shell: echo "test\\" means content is "test\" (backslash at end)
|
||||
// In JS: 'echo "test\\\\"' represents the shell command: echo "test\\"
|
||||
expect(validateCommandQuotes('echo "test\\\\"')).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should handle single quote in double quoted string (valid)", () => {
|
||||
expect(validateCommandQuotes(`echo "it's fine"`)).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should handle double quote in single quoted string (valid)", () => {
|
||||
expect(validateCommandQuotes(`echo 'say "hi"'`)).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should handle escaped single quote outside of quotes", () => {
|
||||
expect(validateCommandQuotes("echo it\\'s")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should handle multiple unbalanced - reports first one", () => {
|
||||
// If both quote types are unbalanced, we find whichever starts first
|
||||
const result = validateCommandQuotes(`echo "hello 'world`)
|
||||
expect(result.valid).toBe(false)
|
||||
// Double quote at position 5 starts before single quote at position 12
|
||||
if (!result.valid) {
|
||||
expect(result.quoteType).toBe("double")
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe("shell-specific patterns", () => {
|
||||
it("should accept bash variable substitution in quotes", () => {
|
||||
expect(validateCommandQuotes('echo "$HOME/path"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes('echo "${VAR:-default}"')).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should accept command substitution in quotes", () => {
|
||||
expect(validateCommandQuotes('echo "Today is $(date)"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("echo 'Date: $(date)'")).toEqual({ valid: true })
|
||||
})
|
||||
|
||||
it("should handle PowerShell style quotes", () => {
|
||||
expect(validateCommandQuotes('Write-Output "Hello World"')).toEqual({ valid: true })
|
||||
expect(validateCommandQuotes("Write-Output 'Hello World'")).toEqual({ valid: true })
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -2,6 +2,165 @@ import { parse } from "shell-quote"
|
|||
|
||||
export type ShellToken = string | { op: string } | { command: string }
|
||||
|
||||
/**
|
||||
* Result of quote validation.
|
||||
* - `valid: true` means the command has balanced quotes
|
||||
* - `valid: false` means the command has unbalanced quotes with details
|
||||
*/
|
||||
export type QuoteValidationResult =
|
||||
| { valid: true }
|
||||
| { valid: false; quoteType: "single" | "double"; position: number; context: string }
|
||||
|
||||
/**
|
||||
* Validates that a command has balanced quotes and won't cause the shell to hang
|
||||
* waiting for more input.
|
||||
*
|
||||
* This function detects:
|
||||
* - Unbalanced single quotes (')
|
||||
* - Unbalanced double quotes (")
|
||||
*
|
||||
* It correctly handles:
|
||||
* - Escaped quotes within strings (\' and \")
|
||||
* - Quotes nested within the opposite quote type ("it's" or 'say "hi"')
|
||||
* - Heredoc syntax (<<EOF, <<'EOF', <<"EOF") which legitimately spans lines
|
||||
* - ANSI-C quoting ($'...')
|
||||
*
|
||||
* @param command The shell command to validate
|
||||
* @returns QuoteValidationResult indicating if quotes are balanced
|
||||
*/
|
||||
export function validateCommandQuotes(command: string): QuoteValidationResult {
|
||||
if (!command || command.trim().length === 0) {
|
||||
return { valid: true }
|
||||
}
|
||||
|
||||
// Check for heredoc patterns which legitimately expect more input
|
||||
// Common heredoc patterns: <<EOF, <<-EOF, <<'EOF', <<"EOF", << 'EOF'
|
||||
const heredocPattern = /<<-?\s*['"]?\w+['"]?\s*$/
|
||||
if (heredocPattern.test(command.trim())) {
|
||||
// Heredocs are valid multi-line constructs - don't flag as unbalanced
|
||||
return { valid: true }
|
||||
}
|
||||
|
||||
let inSingleQuote = false
|
||||
let inDoubleQuote = false
|
||||
let singleQuoteStart = -1
|
||||
let doubleQuoteStart = -1
|
||||
let i = 0
|
||||
|
||||
while (i < command.length) {
|
||||
const char = command[i]
|
||||
|
||||
// Handle ANSI-C quoting: $'...' - treat as single quote
|
||||
if (char === "$" && i + 1 < command.length && command[i + 1] === "'") {
|
||||
if (!inSingleQuote && !inDoubleQuote) {
|
||||
inSingleQuote = true
|
||||
singleQuoteStart = i
|
||||
i += 2 // Skip $'
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
// Count consecutive backslashes before this character
|
||||
// An even number of backslashes means the quote is NOT escaped
|
||||
// An odd number means it IS escaped
|
||||
const isEscaped = (): boolean => {
|
||||
let backslashCount = 0
|
||||
let j = i - 1
|
||||
while (j >= 0 && command[j] === "\\") {
|
||||
backslashCount++
|
||||
j--
|
||||
}
|
||||
// Odd number of backslashes = escaped
|
||||
return backslashCount % 2 === 1
|
||||
}
|
||||
|
||||
// Handle single quotes
|
||||
if (char === "'") {
|
||||
// Inside double quotes, single quotes are literal
|
||||
if (inDoubleQuote) {
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
// Check for escape sequence (only valid outside quotes in some shells,
|
||||
// but we're conservative and check for \' pattern)
|
||||
// Note: In single quotes, backslash is literal, so \' doesn't escape
|
||||
// But outside quotes or at string boundaries, it can be an escape
|
||||
if (!inSingleQuote && isEscaped()) {
|
||||
// Escaped single quote outside any quotes - skip it
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
if (inSingleQuote) {
|
||||
// Closing single quote
|
||||
inSingleQuote = false
|
||||
singleQuoteStart = -1
|
||||
} else {
|
||||
// Opening single quote
|
||||
inSingleQuote = true
|
||||
singleQuoteStart = i
|
||||
}
|
||||
}
|
||||
|
||||
// Handle double quotes
|
||||
if (char === '"') {
|
||||
// Inside single quotes, double quotes are literal
|
||||
if (inSingleQuote) {
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
// Check for escape sequence \" (valid inside double quotes)
|
||||
// Must account for escaped backslashes: \\" means backslash + unescaped quote
|
||||
if (isEscaped()) {
|
||||
// Escaped double quote - skip it
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
if (inDoubleQuote) {
|
||||
// Closing double quote
|
||||
inDoubleQuote = false
|
||||
doubleQuoteStart = -1
|
||||
} else {
|
||||
// Opening double quote
|
||||
inDoubleQuote = true
|
||||
doubleQuoteStart = i
|
||||
}
|
||||
}
|
||||
|
||||
i++
|
||||
}
|
||||
|
||||
// Check for unbalanced quotes
|
||||
if (inSingleQuote) {
|
||||
const contextStart = Math.max(0, singleQuoteStart - 10)
|
||||
const contextEnd = Math.min(command.length, singleQuoteStart + 20)
|
||||
const context = command.slice(contextStart, contextEnd)
|
||||
return {
|
||||
valid: false,
|
||||
quoteType: "single",
|
||||
position: singleQuoteStart,
|
||||
context: context,
|
||||
}
|
||||
}
|
||||
|
||||
if (inDoubleQuote) {
|
||||
const contextStart = Math.max(0, doubleQuoteStart - 10)
|
||||
const contextEnd = Math.min(command.length, doubleQuoteStart + 20)
|
||||
const context = command.slice(contextStart, contextEnd)
|
||||
return {
|
||||
valid: false,
|
||||
quoteType: "double",
|
||||
position: doubleQuoteStart,
|
||||
context: context,
|
||||
}
|
||||
}
|
||||
|
||||
return { valid: true }
|
||||
}
|
||||
|
||||
/**
|
||||
* Split a command string into individual sub-commands by
|
||||
* chaining operators (&&, ||, ;, |, or &) and newlines.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue