From eb0add23d241392f66b5dbdc53246829abbcfa9f Mon Sep 17 00:00:00 2001 From: Roo Code Date: Tue, 23 Sep 2025 01:32:33 +0000 Subject: [PATCH] fix: prevent tool parsing within code blocks - Added code block detection to parseAssistantMessage.ts - Added code block detection to parseAssistantMessageV2.ts - Added code block detection to AssistantMessageParser.ts - Added comprehensive tests for code block scenarios - Fixes issue where tool XML tags in code examples were incorrectly parsed as actual tool invocations Fixes #8242 --- .../AssistantMessageParser.ts | 122 +++++++++----- .../__tests__/parseAssistantMessage.spec.ts | 149 ++++++++++++++++++ .../parseAssistantMessage.ts | 91 ++++++++--- .../parseAssistantMessageV2.ts | 120 +++++++++----- 4 files changed, 378 insertions(+), 104 deletions(-) diff --git a/src/core/assistant-message/AssistantMessageParser.ts b/src/core/assistant-message/AssistantMessageParser.ts index 364ec603f2..e0ca74a72b 100644 --- a/src/core/assistant-message/AssistantMessageParser.ts +++ b/src/core/assistant-message/AssistantMessageParser.ts @@ -17,6 +17,9 @@ export class AssistantMessageParser { private readonly MAX_ACCUMULATOR_SIZE = 1024 * 1024 // 1MB limit private readonly MAX_PARAM_LENGTH = 1024 * 100 // 100KB per parameter limit private accumulator = "" + private inCodeBlock = false + private inInlineCode = false + private codeBlockDelimiterCount = 0 /** * Initialize a new AssistantMessageParser instance. @@ -37,6 +40,9 @@ export class AssistantMessageParser { this.currentParamName = undefined this.currentParamValueStartIndex = 0 this.accumulator = "" + this.inCodeBlock = false + this.inInlineCode = false + this.codeBlockDelimiterCount = 0 } /** @@ -63,6 +69,41 @@ export class AssistantMessageParser { this.accumulator += char const currentPosition = accumulatorStartLength + i + // Track code blocks and inline code + if (char === "`") { + this.codeBlockDelimiterCount++ + if (this.codeBlockDelimiterCount === 3) { + this.inCodeBlock = !this.inCodeBlock + this.codeBlockDelimiterCount = 0 + this.inInlineCode = false // Code blocks take precedence + } + } else { + // If we had one backtick and now a different char, toggle inline code + if (this.codeBlockDelimiterCount === 1 && !this.inCodeBlock) { + this.inInlineCode = !this.inInlineCode + } + this.codeBlockDelimiterCount = 0 + } + + // Skip tool parsing if we're inside code blocks or inline code + if (this.inCodeBlock || this.inInlineCode) { + // Continue accumulating text content + if (this.currentTextContent === undefined && !this.currentToolUse) { + this.currentTextContentStartIndex = currentPosition + this.currentTextContent = { + type: "text", + content: this.accumulator.slice(this.currentTextContentStartIndex).trim(), + partial: true, + } + // Add the new text content to contentBlocks immediately + this.contentBlocks.push(this.currentTextContent) + } else if (this.currentTextContent) { + // Update the existing text content + this.currentTextContent.content = this.accumulator.slice(this.currentTextContentStartIndex).trim() + } + continue + } + // There should not be a param without a tool use. if (this.currentToolUse && this.currentParamName) { const currentParamValue = this.accumulator.slice(this.currentParamValueStartIndex) @@ -159,47 +200,50 @@ export class AssistantMessageParser { for (const toolUseOpeningTag of possibleToolUseOpeningTags) { if (this.accumulator.endsWith(toolUseOpeningTag)) { - // Extract and validate the tool name - const extractedToolName = toolUseOpeningTag.slice(1, -1) + // Only process tool tags if we're not in code blocks + if (!this.inCodeBlock && !this.inInlineCode) { + // Extract and validate the tool name + const extractedToolName = toolUseOpeningTag.slice(1, -1) - // Check if the extracted tool name is valid - if (!toolNames.includes(extractedToolName as ToolName)) { - // Invalid tool name, treat as plain text and continue - continue + // Check if the extracted tool name is valid + if (!toolNames.includes(extractedToolName as ToolName)) { + // Invalid tool name, treat as plain text and continue + continue + } + + // Start of a new tool use. + this.currentToolUse = { + type: "tool_use", + name: extractedToolName as ToolName, + params: {}, + partial: true, + } + + this.currentToolUseStartIndex = this.accumulator.length + + // This also indicates the end of the current text content. + if (this.currentTextContent) { + this.currentTextContent.partial = false + + // Remove the partially accumulated tool use tag from the + // end of text ( block === this.currentToolUse) + if (idx === -1) { + this.contentBlocks.push(this.currentToolUse) + } + + didStartToolUse = true + break } - - // Start of a new tool use. - this.currentToolUse = { - type: "tool_use", - name: extractedToolName as ToolName, - params: {}, - partial: true, - } - - this.currentToolUseStartIndex = this.accumulator.length - - // This also indicates the end of the current text content. - if (this.currentTextContent) { - this.currentTextContent.partial = false - - // Remove the partially accumulated tool use tag from the - // end of text ( block === this.currentToolUse) - if (idx === -1) { - this.contentBlocks.push(this.currentToolUse) - } - - didStartToolUse = true - break } } diff --git a/src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts b/src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts index f5ae600bee..ac6a40783b 100644 --- a/src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts +++ b/src/core/assistant-message/__tests__/parseAssistantMessage.spec.ts @@ -336,5 +336,154 @@ const isEmptyTextContent = (block: AssistantMessageContent) => expect((result[5] as ToolUse).name).toBe("execute_command") }) }) + + describe("code block handling", () => { + it("should not parse tool tags within code blocks", () => { + const message = `Here's an example of the ask_followup_question tool: + +\`\`\`xml + +What is the path to the frontend-config.json file? + +./src/frontend-config.json +./config/frontend-config.json +./frontend-config.json + + +\`\`\` + +This is how you use it.` + + const result = parser(message) + + // Should only have text content, no tool use + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toContain("Here's an example") + expect(textContent.content).toContain("") + expect(textContent.content).toContain("This is how you use it") + expect(textContent.partial).toBe(true) + }) + + it("should not parse tool tags within inline code", () => { + const message = "Use the \`file.ts\` tool to read files." + const result = parser(message) + + // Should only have text content, no tool use + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toBe(message) + expect(textContent.partial).toBe(true) + }) + + it("should parse tool tags outside of code blocks", () => { + const message = `Here's an example: + +\`\`\` +code +\`\`\` + +Now let me read a file: + +test.ts` + + const result = parser(message) + + // Should have text content and a tool use + expect(result).toHaveLength(2) + + // First should be text containing the code block + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toContain("Here's an example") + expect(textContent.content).toContain("code") + expect(textContent.content).toContain("Now let me read a file:") + expect(textContent.partial).toBe(false) + + // Second should be the actual tool use + expect(result[1].type).toBe("tool_use") + const toolUse = result[1] as ToolUse + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.path).toBe("test.ts") + expect(toolUse.partial).toBe(false) + }) + + it("should handle mixed inline code and actual tool uses", () => { + const message = + "The tool \`\` is used like this: actual.ts" + const result = parser(message) + + // Should have text and tool use + expect(result).toHaveLength(2) + + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toContain("The tool \`\` is used like this:") + + expect(result[1].type).toBe("tool_use") + const toolUse = result[1] as ToolUse + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.path).toBe("actual.ts") + }) + + it("should handle code blocks with triple backticks inside", () => { + const message = `Here's a markdown example: + +\`\`\`markdown +# Example +\`\`\`python +print("hello") +\`\`\` +not_a_tool.ts +\`\`\` + +That was the example.` + + const result = parser(message) + + // Should only have text content + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toContain("Here's a markdown example") + expect(textContent.content).toContain("not_a_tool.ts") + expect(textContent.content).toContain("That was the example") + }) + + it("should correctly handle the exact issue scenario", () => { + const message = `## ask_followup_question +Description: Ask the user a question to gather additional information needed to complete the task. + +Usage: +\`\`\` + +Your question here + +First suggestion +Action with mode switch + + +\`\`\` + +This tool helps gather information.` + + const result = parser(message) + + // Should only have text content, no tool invocation + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + const textContent = result[0] as TextContent + expect(textContent.content).toContain("ask_followup_question") + expect(textContent.content).toContain("Description:") + expect(textContent.content).toContain("") + expect(textContent.content).toContain("This tool helps gather information") + + // Ensure no tool_use blocks were created + const toolUses = result.filter((block) => block.type === "tool_use") + expect(toolUses).toHaveLength(0) + }) + }) }) }) diff --git a/src/core/assistant-message/parseAssistantMessage.ts b/src/core/assistant-message/parseAssistantMessage.ts index ebb8674c8f..a8d13f91fd 100644 --- a/src/core/assistant-message/parseAssistantMessage.ts +++ b/src/core/assistant-message/parseAssistantMessage.ts @@ -13,11 +13,49 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag let currentParamName: ToolParamName | undefined = undefined let currentParamValueStartIndex = 0 let accumulator = "" + let inCodeBlock = false + let inInlineCode = false + let codeBlockDelimiterCount = 0 + let lastTwoChars = "" for (let i = 0; i < assistantMessage.length; i++) { const char = assistantMessage[i] accumulator += char + // Track last two characters for inline code detection + lastTwoChars = (lastTwoChars + char).slice(-2) + + // Check for code block delimiters (```) + if (char === "`") { + codeBlockDelimiterCount++ + if (codeBlockDelimiterCount === 3) { + inCodeBlock = !inCodeBlock + codeBlockDelimiterCount = 0 + } + } else { + // Check for inline code (single backtick) + if (codeBlockDelimiterCount === 1 && !inCodeBlock) { + inInlineCode = !inInlineCode + } + codeBlockDelimiterCount = 0 + } + + // Skip tool parsing if we're inside a code block or inline code + if (inCodeBlock || inInlineCode) { + // If we're in text content, keep accumulating + if (currentTextContent === undefined && !currentToolUse) { + currentTextContentStartIndex = i + currentTextContent = { + type: "text", + content: accumulator.slice(currentTextContentStartIndex).trim(), + partial: true, + } + } else if (currentTextContent) { + currentTextContent.content = accumulator.slice(currentTextContentStartIndex).trim() + } + continue + } + // There should not be a param without a tool use. if (currentToolUse && currentParamName) { const currentParamValue = accumulator.slice(currentParamValueStartIndex) @@ -97,32 +135,35 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag for (const toolUseOpeningTag of possibleToolUseOpeningTags) { if (accumulator.endsWith(toolUseOpeningTag)) { - // Start of a new tool use. - currentToolUse = { - type: "tool_use", - name: toolUseOpeningTag.slice(1, -1) as ToolName, - params: {}, - partial: true, + // Only start a new tool use if we're not in a code block + if (!inCodeBlock && !inInlineCode) { + // Start of a new tool use. + currentToolUse = { + type: "tool_use", + name: toolUseOpeningTag.slice(1, -1) as ToolName, + params: {}, + partial: true, + } + + currentToolUseStartIndex = accumulator.length + + // This also indicates the end of the current text content. + if (currentTextContent) { + currentTextContent.partial = false + + // Remove the partially accumulated tool use tag from the + // end of text (() const toolParamOpenTags = new Map() @@ -63,6 +68,38 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess for (let i = 0; i < len; i++) { const currentCharIndex = i + const char = assistantMessage[i] + + // Track code blocks and inline code + if (char === "`") { + backtickCount++ + // Check if we have three backticks for code block + if (backtickCount === 3) { + inCodeBlock = !inCodeBlock + backtickCount = 0 + inInlineCode = false // Code blocks take precedence + } + } else { + // If we had one backtick and now a different char, toggle inline code + if (backtickCount === 1 && !inCodeBlock) { + inInlineCode = !inInlineCode + } + backtickCount = 0 + } + + // Skip tool parsing if we're inside code blocks or inline code + if (inCodeBlock || inInlineCode) { + // Continue accumulating text content + if (!currentTextContent && !currentToolUse) { + currentTextContentStart = currentCharIndex + currentTextContent = { + type: "text", + content: "", + partial: true, + } + } + continue + } // Parsing a tool parameter if (currentToolUse && currentParamName) { @@ -177,52 +214,55 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess currentCharIndex >= tag.length - 1 && assistantMessage.startsWith(tag, currentCharIndex - tag.length + 1) ) { - // End current text block if one was active. - if (currentTextContent) { - currentTextContent.content = assistantMessage - .slice( - currentTextContentStart, // From where text started. - currentCharIndex - tag.length + 1, // To before the tool tag starts. - ) - .trim() + // Only process tool tags if we're not in code blocks + if (!inCodeBlock && !inInlineCode) { + // End current text block if one was active. + if (currentTextContent) { + currentTextContent.content = assistantMessage + .slice( + currentTextContentStart, // From where text started. + currentCharIndex - tag.length + 1, // To before the tool tag starts. + ) + .trim() - currentTextContent.partial = false // Ended because tool started. + currentTextContent.partial = false // Ended because tool started. - if (currentTextContent.content.length > 0) { - contentBlocks.push(currentTextContent) + if (currentTextContent.content.length > 0) { + contentBlocks.push(currentTextContent) + } + + currentTextContent = undefined + } else { + // Check for any text between the last block and this tag. + const potentialText = assistantMessage + .slice( + currentTextContentStart, // From where text *might* have started. + currentCharIndex - tag.length + 1, // To before the tool tag starts. + ) + .trim() + + if (potentialText.length > 0) { + contentBlocks.push({ + type: "text", + content: potentialText, + partial: false, + }) + } } - currentTextContent = undefined - } else { - // Check for any text between the last block and this tag. - const potentialText = assistantMessage - .slice( - currentTextContentStart, // From where text *might* have started. - currentCharIndex - tag.length + 1, // To before the tool tag starts. - ) - .trim() - - if (potentialText.length > 0) { - 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. } + + currentToolUseStart = currentCharIndex + 1 // Tool content starts after the opening tag. + startedNewTool = true + + break } - - // Start the new tool use. - currentToolUse = { - type: "tool_use", - name: toolName, - params: {}, - partial: true, // Assume partial until closing tag is found. - } - - currentToolUseStart = currentCharIndex + 1 // Tool content starts after the opening tag. - startedNewTool = true - - break } }