From 05bf2706efc5f7a54060c4524f39fa007a2eb1fa Mon Sep 17 00:00:00 2001 From: Roo Code Date: Wed, 4 Feb 2026 23:58:02 +0000 Subject: [PATCH] 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 (< { 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) { diff --git a/src/core/tools/__tests__/executeCommandTool.spec.ts b/src/core/tools/__tests__/executeCommandTool.spec.ts index 89b2575288b..6b36fce383b 100644 --- a/src/core/tools/__tests__/executeCommandTool.spec.ts +++ b/src/core/tools/__tests__/executeCommandTool.spec.ts @@ -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 diff --git a/src/shared/__tests__/parse-command.spec.ts b/src/shared/__tests__/parse-command.spec.ts new file mode 100644 index 00000000000..6c9a69cf8dc --- /dev/null +++ b/src/shared/__tests__/parse-command.spec.ts @@ -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 < { + 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 }) + }) + }) +}) diff --git a/src/shared/parse-command.ts b/src/shared/parse-command.ts index a8d87b76acf..a2ce2b254c3 100644 --- a/src/shared/parse-command.ts +++ b/src/shared/parse-command.ts @@ -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 (< { + 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.