Accidental execution of tool syntax fix (RooCodeInc#3456)

Co-authored-by: cte <[email protected]>

Author: 2 people

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 <read_file> mention as plain text", () => {
+				const message = "Use the `<read_file>` 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:",
+					"```",
+					"<read_file>",
+					"<path>demo.txt</path>",
+					"</read_file>",
+					"```",
+				].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 = ["<read_file>", "<path>demo.txt</path>", "</read_file>"].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:",
+					"> ```",
+					"> <read_file><path>test.txt</path></read_file>",
+					"> ```",
+				].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 \\`<read_file>\\` tag"
+
+				const result = parser(message)
+				expect(result).toHaveLength(1)
+				expect(result[0].type).toBe("text")
+			})
+
+			it("handles incomplete code blocks", () => {
+				const message = [
+					"```javascript",
+					"<read_file><path>test.txt</path></read_file>",
+					// 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 = "<!-- <read_file><path>test.txt</path></read_file> -->"
+
+				// 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("<!--")
+				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("-->")
+			})
+
+			it("handles tool tags with HTML-like attributes", () => {
+				const message = '<read_file class="example"><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")
+			})
+
+			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 = "**<read_file>**<path>test.txt</path></read_file>"
+
+				// 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](<read_file><path>test.txt</path></read_file>)"
+
+				// 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 = "<execute_command><command>"
+				for (let i = 0; i < 10; i++) {
+					message += `echo "Level ${i}" && `
+				}
+				message += 'echo "Done"</command></execute_command>'
+
+				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")
+			})
 		})
 	})
 })

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)} |`,
 		)

src/core/assistant-message/index.ts

@@ -1,2 +1,3 @@
 export { type AssistantMessageContent, parseAssistantMessage } from "./parseAssistantMessage"
 export { presentAssistantMessage } from "./presentAssistantMessage"
+export { parseAssistantMessageV2 } from "./parseAssistantMessageV2"

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. <read_file>) 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))
 }

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. <read_file>) 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<string, ToolName>()
 	const toolParamOpenTags = new Map<string, ToolParamName>()
@@ -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 = `</${currentParamName}>`
-			// 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