From 1bc4a2db16646f71d9bf772581069ca52bf39bb7 Mon Sep 17 00:00:00 2001 From: xyOz Date: Tue, 13 May 2025 17:20:32 +0100 Subject: [PATCH] Accidental execution of tool syntax fix (#3456) Co-authored-by: cte --- .../__tests__/parseAssistantMessage.test.ts | 152 ++++++++++++++++ .../parseAssistantMessageBenchmark.ts | 8 +- src/core/assistant-message/index.ts | 1 + .../parseAssistantMessage.ts | 65 ++++++- .../parseAssistantMessageV2.ts | 73 ++++++-- src/core/task/Task.ts | 6 +- .../utils/command-validation.test.ts | 158 ----------------- .../__tests__/command-validation.test.ts | 162 ++++++++++++++++++ 8 files changed, 442 insertions(+), 183 deletions(-) delete mode 100644 webview-ui/src/__tests__/utils/command-validation.test.ts diff --git a/src/core/assistant-message/__tests__/parseAssistantMessage.test.ts b/src/core/assistant-message/__tests__/parseAssistantMessage.test.ts index 19f88a91d7..aa57e13896 100644 --- a/src/core/assistant-message/__tests__/parseAssistantMessage.test.ts +++ b/src/core/assistant-message/__tests__/parseAssistantMessage.test.ts @@ -335,6 +335,158 @@ const isEmptyTextContent = (block: AssistantMessageContent) => expect(result[5].type).toBe("tool_use") expect((result[5] as ToolUse).name).toBe("execute_command") }) + + it("treats inline mention as plain text", () => { + const message = "Use the `` tool when you need to read a file." + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("treats fenced code block mention as plain text", () => { + const message = [ + "Here is an example:", + "```", + "", + "demo.txt", + "", + "```", + ].join("\n") + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("parses real tool invocation with newline correctly", () => { + const invocation = ["", "demo.txt", ""].join("\n") + const result = parser(invocation) + expect(result).toHaveLength(1) + const tool = result[0] as ToolUse + expect(tool.type).toBe("tool_use") + expect(tool.name).toBe("read_file") + expect(tool.params.path).toBe("demo.txt") + }) + }) + + describe("code block edge cases", () => { + it("handles nested code blocks with tool tags", () => { + const message = [ + "Here's a nested code block:", + "> ```", + "> test.txt", + "> ```", + ].join("\n") + + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("handles escaped backticks", () => { + const message = "This has an escaped \\`\\` tag" + + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("handles incomplete code blocks", () => { + const message = [ + "```javascript", + "test.txt", + // Missing closing backticks + ].join("\n") + + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + }) + + describe("tool tag edge cases", () => { + it("handles HTML comments containing tool tags", () => { + const message = "" + + // Note: Currently both parsers treat tool tags in HTML comments as actual tool invocations + // This test documents the current behavior rather than the ideal behavior + const result = parser(message) + expect(result).toHaveLength(3) + expect(result[0].type).toBe("text") + expect((result[0] as TextContent).content).toBe("") + }) + + it("handles tool tags with HTML-like attributes", () => { + const message = 'test.txt' + + // This should be treated as plain text since it's not a valid tool format + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("handles malformed tool tags with extra spaces", () => { + const message = "< read_file >< path >test.txt< /path >< /read_file >" + + // This should be treated as plain text since it's not a valid tool format + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + }) + + describe("mixed formatting edge cases", () => { + it("handles tool tags with markdown formatting", () => { + const message = "****test.txt" + + // The opening tag is malformed due to the bold formatting + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + }) + + it("handles tool tags in markdown links", () => { + const message = "[Example](test.txt)" + + // Note: Currently both parsers treat tool tags in markdown links as actual tool invocations + // This test documents the current behavior rather than the ideal behavior + const result = parser(message) + expect(result).toHaveLength(3) + expect(result[0].type).toBe("text") + expect((result[0] as TextContent).content).toBe("[Example](") + expect(result[1].type).toBe("tool_use") + expect((result[1] as ToolUse).name).toBe("read_file") + expect(result[2].type).toBe("text") + expect((result[2] as TextContent).content).toBe(")") + }) + }) + + describe("performance edge cases", () => { + it("handles very long text with no tool uses", () => { + const message = "A".repeat(10000) + + const result = parser(message) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + expect((result[0] as TextContent).content.length).toBe(10000) + }) + + it("handles deeply nested tool parameters", () => { + // Create a message with a tool that has many nested parameters + let message = "" + for (let i = 0; i < 10; i++) { + message += `echo "Level ${i}" && ` + } + message += 'echo "Done"' + + const result = parser(message).filter((block) => !isEmptyTextContent(block)) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("tool_use") + expect((result[0] as ToolUse).name).toBe("execute_command") + }) }) }) }) diff --git a/src/core/assistant-message/__tests__/parseAssistantMessageBenchmark.ts b/src/core/assistant-message/__tests__/parseAssistantMessageBenchmark.ts index bea161330b..1779fae75a 100644 --- a/src/core/assistant-message/__tests__/parseAssistantMessageBenchmark.ts +++ b/src/core/assistant-message/__tests__/parseAssistantMessageBenchmark.ts @@ -81,16 +81,13 @@ const runBenchmark = () => { const namePadding = maxNameLength + 2 console.log( - `| ${"Test Case".padEnd(namePadding)} | V1 Time (ms) | V2 Time (ms) | V1/V2 Ratio | V1 Heap (bytes) | V2 Heap (bytes) |`, - ) - console.log( - `| ${"-".repeat(namePadding)} | ------------ | ------------ | ----------- | ---------------- | ---------------- |`, + `| ${"Test Case".padEnd(namePadding)} | V1 Time (ms) | V2 Time (ms) | V1 Heap (bytes) | V2 Heap (bytes) |`, ) + console.log(`| ${"-".repeat(namePadding)} | ------------ | ------------ | ---------------- | ---------------- |`) for (const testCase of testCases) { const v1Time = measureExecutionTime(parseAssistantMessageV1, testCase.input) const v2Time = measureExecutionTime(parseAssistantMessageV2, testCase.input) - const timeRatio = v1Time / v2Time const v1Memory = measureMemoryUsage(parseAssistantMessageV1, testCase.input) const v2Memory = measureMemoryUsage(parseAssistantMessageV2, testCase.input) @@ -99,7 +96,6 @@ const runBenchmark = () => { `| ${testCase.name.padEnd(namePadding)} | ` + `${v1Time.toFixed(4).padStart(12)} | ` + `${v2Time.toFixed(4).padStart(12)} | ` + - `${timeRatio.toFixed(2).padStart(11)} | ` + `${formatNumber(Math.round(v1Memory.heapUsed)).padStart(16)} | ` + `${formatNumber(Math.round(v2Memory.heapUsed)).padStart(16)} |`, ) diff --git a/src/core/assistant-message/index.ts b/src/core/assistant-message/index.ts index 72201b7722..ee707b8bf4 100644 --- a/src/core/assistant-message/index.ts +++ b/src/core/assistant-message/index.ts @@ -1,2 +1,3 @@ export { type AssistantMessageContent, parseAssistantMessage } from "./parseAssistantMessage" export { presentAssistantMessage } from "./presentAssistantMessage" +export { parseAssistantMessageV2 } from "./parseAssistantMessageV2" diff --git a/src/core/assistant-message/parseAssistantMessage.ts b/src/core/assistant-message/parseAssistantMessage.ts index f641cabdaa..ae861519c8 100644 --- a/src/core/assistant-message/parseAssistantMessage.ts +++ b/src/core/assistant-message/parseAssistantMessage.ts @@ -13,10 +13,52 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag let currentParamValueStartIndex = 0 let accumulator = "" + // Track whether we are inside markdown code blocks or inline code to avoid treating textual mentions + // of tool tags (e.g. ) as actual tool invocations. + let insideCodeBlock = false // ``` fenced code block + let insideInlineCode = false // `inline code` + + // Helper to decide if we should parse for tool-related tags at the current position + const shouldParseToolTags = () => !insideCodeBlock && !insideInlineCode + for (let i = 0; i < assistantMessage.length; i++) { const char = assistantMessage[i] + + // Detect fenced code block (```). + if (!insideInlineCode && assistantMessage.slice(i, i + 3) === "```") { + insideCodeBlock = !insideCodeBlock + // Append the full trio of backticks to accumulator. + accumulator += "```" + i += 2 // Skip the two extra backticks we just added. + + // When toggling code block state, continue to next iteration as + // these chars are text. + continue + } + + // Detect inline code (`) when not inside a fenced code block and not part of triple backticks + if (!insideCodeBlock && char === "`") { + insideInlineCode = !insideInlineCode + } + accumulator += char + // If we are in any kind of code context, treat everything as plain text. + if (!shouldParseToolTags()) { + // Handle accumulating text content block. + if (currentTextContent === undefined) { + currentTextContentStartIndex = i + } + + currentTextContent = { + type: "text", + content: accumulator.slice(currentTextContentStartIndex).trim(), + partial: true, + } + + continue + } + // There should not be a param without a tool use. if (currentToolUse && currentParamName) { const currentParamValue = accumulator.slice(currentParamValueStartIndex) @@ -45,6 +87,7 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag continue } else { const possibleParamOpeningTags = toolParamNames.map((name) => `<${name}>`) + for (const paramOpeningTag of possibleParamOpeningTags) { if (accumulator.endsWith(paramOpeningTag)) { // Start of a new parameter. @@ -89,6 +132,25 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag for (const toolUseOpeningTag of possibleToolUseOpeningTags) { if (accumulator.endsWith(toolUseOpeningTag)) { + // Check that this is likely an actual tool invocation and not + // an inline textual reference. + // We consider it an invocation only if the next non-whitespace + // character is a newline (\n or \r) or an opening angle bracket + // '<' (which would start the first parameter tag). + + let j = i + 1 // Position after the closing '>' of the opening tag. + + while (j < assistantMessage.length && assistantMessage[j] === " ") { + j++ + } + + const nextChar = assistantMessage[j] ?? "" + + if (nextChar && nextChar !== "<" && nextChar !== "\n" && nextChar !== "\r") { + // Treat as plain text, not a tool invocation. + continue + } + // Start of a new tool use. currentToolUse = { type: "tool_use", @@ -151,5 +213,6 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag contentBlocks.push(currentTextContent) } - return contentBlocks + // Remove any empty text blocks that may have been created by whitespace or newlines before/after tool calls + return contentBlocks.filter((block) => !(block.type === "text" && block.content.trim().length === 0)) } diff --git a/src/core/assistant-message/parseAssistantMessageV2.ts b/src/core/assistant-message/parseAssistantMessageV2.ts index d24a67f83d..f04084f57e 100644 --- a/src/core/assistant-message/parseAssistantMessageV2.ts +++ b/src/core/assistant-message/parseAssistantMessageV2.ts @@ -46,6 +46,14 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess let currentParamValueStart = 0 // Index *after* the opening tag of the current param. let currentParamName: ToolParamName | undefined = undefined + // Track whether we are inside markdown code blocks or inline code to avoid treating textual mentions + // of tool tags (e.g. ) as actual tool invocations. + let insideCodeBlock = false // ``` fenced code block + let insideInlineCode = false // `inline code` + + // Helper to decide if we should parse for tool-related tags at the current position + const shouldParseToolTags = () => !insideCodeBlock && !insideInlineCode + // Precompute tags for faster lookups. const toolUseOpenTags = new Map() const toolParamOpenTags = new Map() @@ -63,16 +71,39 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess for (let i = 0; i < len; i++) { const currentCharIndex = i + // Detect fenced code block (```). + if (!insideInlineCode && i + 2 < len && assistantMessage.slice(i, i + 3) === "```") { + insideCodeBlock = !insideCodeBlock + i += 2 // Skip the two extra backticks. + continue + } + + // Detect inline code (`) when not inside a fenced code block and not + // part of triple backticks. + if (!insideCodeBlock && assistantMessage[i] === "`") { + insideInlineCode = !insideInlineCode + } + + // If we are in any kind of code context, treat everything as plain text. + if (!shouldParseToolTags()) { + // If we're not already tracking text content, start now. + if (!currentTextContent) { + currentTextContentStart = currentCharIndex + currentTextContent = { type: "text", content: "", partial: true } + } + + continue + } + // Parsing a tool parameter if (currentToolUse && currentParamName) { const closeTag = `` - // Check if the string *ending* at index `i` matches the closing tag + + // Check if the string *ending* at index `i` matches the closing tag. if ( currentCharIndex >= closeTag.length - 1 && - assistantMessage.startsWith( - closeTag, - currentCharIndex - closeTag.length + 1, // Start checking from potential start of tag. - ) + // Start checking from potential start of tag. + assistantMessage.startsWith(closeTag, currentCharIndex - closeTag.length + 1) ) { // Found the closing tag for the parameter. const value = assistantMessage @@ -175,6 +206,23 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess currentCharIndex >= tag.length - 1 && assistantMessage.startsWith(tag, currentCharIndex - tag.length + 1) ) { + // Check that this is likely an actual tool invocation and not + // an inline textual reference. + // We consider it an invocation only if the next non-whitespace + // character is a newline (\n or \r) or an opening angle bracket + // '<' (which would start the first parameter tag). + let j = currentCharIndex + 1 // Position after the closing '>' of the opening tag. + + while (j < len && assistantMessage[j] === " ") { + j++ + } + + const nextChar = assistantMessage[j] ?? "" + + if (nextChar && nextChar !== "<" && nextChar !== "\n" && nextChar !== "\r") { + // Treat as plain text, not a tool invocation. + continue + } // End current text block if one was active. if (currentTextContent) { currentTextContent.content = assistantMessage @@ -201,22 +249,13 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess .trim() if (potentialText.length > 0) { - contentBlocks.push({ - type: "text", - content: potentialText, - partial: false, - }) + contentBlocks.push({ type: "text", content: potentialText, partial: false }) } } // Start the new tool use. - currentToolUse = { - type: "tool_use", - name: toolName, - params: {}, - partial: true, // Assume partial until closing tag is found. - } - + // Assume partial until closing tag is found. + currentToolUse = { type: "tool_use", name: toolName, params: {}, partial: true } currentToolUseStart = currentCharIndex + 1 // Tool content starts after the opening tag. startedNewTool = true diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index bdc041ef57..2d2180e391 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -60,7 +60,11 @@ import { SYSTEM_PROMPT } from "../prompts/system" import { ToolRepetitionDetector } from "../tools/ToolRepetitionDetector" import { FileContextTracker } from "../context-tracking/FileContextTracker" import { RooIgnoreController } from "../ignore/RooIgnoreController" -import { type AssistantMessageContent, parseAssistantMessage, presentAssistantMessage } from "../assistant-message" +import { + type AssistantMessageContent, + parseAssistantMessageV2 as parseAssistantMessage, + presentAssistantMessage, +} from "../assistant-message" import { truncateConversationIfNeeded } from "../sliding-window" import { ClineProvider } from "../webview/ClineProvider" import { MultiSearchReplaceDiffStrategy } from "../diff/strategies/multi-search-replace" diff --git a/webview-ui/src/__tests__/utils/command-validation.test.ts b/webview-ui/src/__tests__/utils/command-validation.test.ts deleted file mode 100644 index d1e3fc6010..0000000000 --- a/webview-ui/src/__tests__/utils/command-validation.test.ts +++ /dev/null @@ -1,158 +0,0 @@ -import { parseCommand, validateCommand } from "../../utils/command-validation" - -/** - * Tests for the command-validation module - * - * These tests include a reproduction of a bug where the shell-quote library - * used in parseCommand throws an error when parsing commands that contain - * the Bash $RANDOM variable in array indexing expressions. - * - * Error: "Bad substitution: levels[$RANDOM" - * - * The issue occurs specifically with complex expressions like: - * ${levels[$RANDOM % ${#levels[@]}]} - */ -describe("command-validation", () => { - describe("parseCommand", () => { - it("should correctly parse simple commands", () => { - const result = parseCommand("echo hello") - expect(result).toEqual(["echo hello"]) - }) - - it("should correctly parse commands with chaining operators", () => { - const result = parseCommand("echo hello && echo world") - expect(result).toEqual(["echo hello", "echo world"]) - }) - - it("should correctly parse commands with quotes", () => { - const result = parseCommand('echo "hello world"') - expect(result).toEqual(['echo "hello world"']) - }) - - it("should correctly parse commands with redirections", () => { - const result = parseCommand("echo hello 2>&1") - expect(result).toEqual(["echo hello 2>&1"]) - }) - - it("should correctly parse commands with subshells", () => { - const result = parseCommand("echo $(date)") - expect(result).toEqual(["echo", "date"]) - }) - - it("should not throw an error when parsing commands with simple array indexing", () => { - // Simple array indexing works fine - const commandWithArrayIndex = "value=${array[$index]}" - - expect(() => { - parseCommand(commandWithArrayIndex) - }).not.toThrow() - }) - - it("should not throw an error when parsing commands with $RANDOM in array index", () => { - // This test reproduces the specific bug reported in the error - const commandWithRandom = "level=${levels[$RANDOM % ${#levels[@]}]}" - - expect(() => { - parseCommand(commandWithRandom) - }).not.toThrow() - }) - - it("should not throw an error with simple $RANDOM variable", () => { - // Simple $RANDOM usage works fine - const commandWithRandom = "echo $RANDOM" - - expect(() => { - parseCommand(commandWithRandom) - }).not.toThrow() - }) - - it("should not throw an error with simple array indexing using $RANDOM", () => { - // Simple array indexing with $RANDOM works fine - const commandWithRandomIndex = "echo ${array[$RANDOM]}" - - expect(() => { - parseCommand(commandWithRandomIndex) - }).not.toThrow() - }) - - it("should not throw an error with complex array indexing using $RANDOM and arithmetic", () => { - // This test reproduces the exact expression from the original error - const commandWithComplexRandom = "echo ${levels[$RANDOM % ${#levels[@]}]}" - - expect(() => { - parseCommand(commandWithComplexRandom) - }).not.toThrow("Bad substitution") - }) - - it("should not throw an error when parsing the full log generator command", () => { - // This is the exact command from the original error message - const logGeneratorCommand = `while true; do \\ - levels=(INFO WARN ERROR DEBUG); \\ - msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ - level=\${levels[$RANDOM % \${#levels[@]}]}; \\ - msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ - echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ - sleep 1; \\ -done` - - // This reproduces the original error - expect(() => { - parseCommand(logGeneratorCommand) - }).not.toThrow("Bad substitution: levels[$RANDOM") - }) - - it("should not throw an error when parsing just the problematic part", () => { - // This isolates just the part mentioned in the error message - const problematicPart = "level=${levels[$RANDOM" - - expect(() => { - parseCommand(problematicPart) - }).not.toThrow("Bad substitution") - }) - }) - - describe("validateCommand", () => { - it("should validate allowed commands", () => { - const result = validateCommand("echo hello", ["echo"]) - expect(result).toBe(true) - }) - - it("should reject disallowed commands", () => { - const result = validateCommand("rm -rf /", ["echo", "ls"]) - expect(result).toBe(false) - }) - - it("should not fail validation for commands with simple $RANDOM variable", () => { - const commandWithRandom = "echo $RANDOM" - - expect(() => { - validateCommand(commandWithRandom, ["echo"]) - }).not.toThrow() - }) - - it("should not fail validation for commands with simple array indexing using $RANDOM", () => { - const commandWithRandomIndex = "echo ${array[$RANDOM]}" - - expect(() => { - validateCommand(commandWithRandomIndex, ["echo"]) - }).not.toThrow() - }) - - it("should return false for the full log generator command due to subshell detection", () => { - // This is the exact command from the original error message - const logGeneratorCommand = `while true; do \\ - levels=(INFO WARN ERROR DEBUG); \\ - msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ - level=\${levels[$RANDOM % \${#levels[@]}]}; \\ - msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ - echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ - sleep 1; \\ -done` - - // validateCommand should return false due to subshell detection - // without throwing an error - const result = validateCommand(logGeneratorCommand, ["while"]) - expect(result).toBe(false) - }) - }) -}) diff --git a/webview-ui/src/utils/__tests__/command-validation.test.ts b/webview-ui/src/utils/__tests__/command-validation.test.ts index c1c73548b9..9d89beec46 100644 --- a/webview-ui/src/utils/__tests__/command-validation.test.ts +++ b/webview-ui/src/utils/__tests__/command-validation.test.ts @@ -1,3 +1,8 @@ +/* eslint-disable no-useless-escape */ +/* eslint-disable no-template-curly-in-string */ + +// npx jest src/utils/__tests__/command-validation.test.ts + import { parseCommand, isAllowedSingleCommand, validateCommand } from "../command-validation" describe("Command Validation", () => { @@ -119,3 +124,160 @@ describe("Command Validation", () => { }) }) }) + +/** + * Tests for the command-validation module + * + * These tests include a reproduction of a bug where the shell-quote library + * used in parseCommand throws an error when parsing commands that contain + * the Bash $RANDOM variable in array indexing expressions. + * + * Error: "Bad substitution: levels[$RANDOM" + * + * The issue occurs specifically with complex expressions like: + * ${levels[$RANDOM % ${#levels[@]}]} + */ +describe("command-validation", () => { + describe("parseCommand", () => { + it("should correctly parse simple commands", () => { + const result = parseCommand("echo hello") + expect(result).toEqual(["echo hello"]) + }) + + it("should correctly parse commands with chaining operators", () => { + const result = parseCommand("echo hello && echo world") + expect(result).toEqual(["echo hello", "echo world"]) + }) + + it("should correctly parse commands with quotes", () => { + const result = parseCommand('echo "hello world"') + expect(result).toEqual(['echo "hello world"']) + }) + + it("should correctly parse commands with redirections", () => { + const result = parseCommand("echo hello 2>&1") + expect(result).toEqual(["echo hello 2>&1"]) + }) + + it("should correctly parse commands with subshells", () => { + const result = parseCommand("echo $(date)") + expect(result).toEqual(["echo", "date"]) + }) + + it("should not throw an error when parsing commands with simple array indexing", () => { + // Simple array indexing works fine + const commandWithArrayIndex = "value=${array[$index]}" + + expect(() => { + parseCommand(commandWithArrayIndex) + }).not.toThrow() + }) + + it("should not throw an error when parsing commands with $RANDOM in array index", () => { + // This test reproduces the specific bug reported in the error + const commandWithRandom = "level=${levels[$RANDOM % ${#levels[@]}]}" + + expect(() => { + parseCommand(commandWithRandom) + }).not.toThrow() + }) + + it("should not throw an error with simple $RANDOM variable", () => { + // Simple $RANDOM usage works fine + const commandWithRandom = "echo $RANDOM" + + expect(() => { + parseCommand(commandWithRandom) + }).not.toThrow() + }) + + it("should not throw an error with simple array indexing using $RANDOM", () => { + // Simple array indexing with $RANDOM works fine + const commandWithRandomIndex = "echo ${array[$RANDOM]}" + + expect(() => { + parseCommand(commandWithRandomIndex) + }).not.toThrow() + }) + + it("should not throw an error with complex array indexing using $RANDOM and arithmetic", () => { + // This test reproduces the exact expression from the original error + const commandWithComplexRandom = "echo ${levels[$RANDOM % ${#levels[@]}]}" + + expect(() => { + parseCommand(commandWithComplexRandom) + }).not.toThrow("Bad substitution") + }) + + it("should not throw an error when parsing the full log generator command", () => { + // This is the exact command from the original error message + const logGeneratorCommand = `while true; do \\ + levels=(INFO WARN ERROR DEBUG); \\ + msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ + level=\${levels[$RANDOM % \${#levels[@]}]}; \\ + msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ + echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ + sleep 1; \\ +done` + + // This reproduces the original error + expect(() => { + parseCommand(logGeneratorCommand) + }).not.toThrow("Bad substitution: levels[$RANDOM") + }) + + it("should not throw an error when parsing just the problematic part", () => { + // This isolates just the part mentioned in the error message + const problematicPart = "level=${levels[$RANDOM" + + expect(() => { + parseCommand(problematicPart) + }).not.toThrow("Bad substitution") + }) + }) + + describe("validateCommand", () => { + it("should validate allowed commands", () => { + const result = validateCommand("echo hello", ["echo"]) + expect(result).toBe(true) + }) + + it("should reject disallowed commands", () => { + const result = validateCommand("rm -rf /", ["echo", "ls"]) + expect(result).toBe(false) + }) + + it("should not fail validation for commands with simple $RANDOM variable", () => { + const commandWithRandom = "echo $RANDOM" + + expect(() => { + validateCommand(commandWithRandom, ["echo"]) + }).not.toThrow() + }) + + it("should not fail validation for commands with simple array indexing using $RANDOM", () => { + const commandWithRandomIndex = "echo ${array[$RANDOM]}" + + expect(() => { + validateCommand(commandWithRandomIndex, ["echo"]) + }).not.toThrow() + }) + + it("should return false for the full log generator command due to subshell detection", () => { + // This is the exact command from the original error message + const logGeneratorCommand = `while true; do \\ + levels=(INFO WARN ERROR DEBUG); \\ + msgs=("User logged in" "Connection timeout" "Processing request" "Cache miss" "Database query"); \\ + level=\${levels[$RANDOM % \${#levels[@]}]}; \\ + msg=\${msgs[$RANDOM % \${#msgs[@]}]}; \\ + echo "\$(date '+%Y-%m-%d %H:%M:%S') [$level] $msg"; \\ + sleep 1; \\ +done` + + // validateCommand should return false due to subshell detection + // without throwing an error + const result = validateCommand(logGeneratorCommand, ["while"]) + expect(result).toBe(false) + }) + }) +})