Skip to content

Commit 622d3e3

Browse files
ctemehmetsunkur
cte
authored and
mehmetsunkur
committed
Revert "Accidental execution of tool syntax fix" (RooCodeInc#3560)
1 parent 460215b commit 622d3e3

File tree

8 files changed

+183
-442
lines changed

8 files changed

+183
-442
lines changed

‎src/core/assistant-message/__tests__/parseAssistantMessage.test.ts‎

Lines changed: 0 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -335,158 +335,6 @@ const isEmptyTextContent = (block: AssistantMessageContent) =>
335335
expect(result[5].type).toBe("tool_use")
336336
expect((result[5] as ToolUse).name).toBe("execute_command")
337337
})
338-
339-
it("treats inline <read_file> mention as plain text", () => {
340-
const message = "Use the `<read_file>` tool when you need to read a file."
341-
const result = parser(message)
342-
expect(result).toHaveLength(1)
343-
expect(result[0].type).toBe("text")
344-
})
345-
346-
it("treats fenced code block mention as plain text", () => {
347-
const message = [
348-
"Here is an example:",
349-
"```",
350-
"<read_file>",
351-
"<path>demo.txt</path>",
352-
"</read_file>",
353-
"```",
354-
].join("\n")
355-
const result = parser(message)
356-
expect(result).toHaveLength(1)
357-
expect(result[0].type).toBe("text")
358-
})
359-
360-
it("parses real tool invocation with newline correctly", () => {
361-
const invocation = ["<read_file>", "<path>demo.txt</path>", "</read_file>"].join("\n")
362-
const result = parser(invocation)
363-
expect(result).toHaveLength(1)
364-
const tool = result[0] as ToolUse
365-
expect(tool.type).toBe("tool_use")
366-
expect(tool.name).toBe("read_file")
367-
expect(tool.params.path).toBe("demo.txt")
368-
})
369-
})
370-
371-
describe("code block edge cases", () => {
372-
it("handles nested code blocks with tool tags", () => {
373-
const message = [
374-
"Here's a nested code block:",
375-
"> ```",
376-
"> <read_file><path>test.txt</path></read_file>",
377-
"> ```",
378-
].join("\n")
379-
380-
const result = parser(message)
381-
expect(result).toHaveLength(1)
382-
expect(result[0].type).toBe("text")
383-
})
384-
385-
it("handles escaped backticks", () => {
386-
const message = "This has an escaped \\`<read_file>\\` tag"
387-
388-
const result = parser(message)
389-
expect(result).toHaveLength(1)
390-
expect(result[0].type).toBe("text")
391-
})
392-
393-
it("handles incomplete code blocks", () => {
394-
const message = [
395-
"```javascript",
396-
"<read_file><path>test.txt</path></read_file>",
397-
// Missing closing backticks
398-
].join("\n")
399-
400-
const result = parser(message)
401-
expect(result).toHaveLength(1)
402-
expect(result[0].type).toBe("text")
403-
})
404-
})
405-
406-
describe("tool tag edge cases", () => {
407-
it("handles HTML comments containing tool tags", () => {
408-
const message = "<!-- <read_file><path>test.txt</path></read_file> -->"
409-
410-
// Note: Currently both parsers treat tool tags in HTML comments as actual tool invocations
411-
// This test documents the current behavior rather than the ideal behavior
412-
const result = parser(message)
413-
expect(result).toHaveLength(3)
414-
expect(result[0].type).toBe("text")
415-
expect((result[0] as TextContent).content).toBe("<!--")
416-
expect(result[1].type).toBe("tool_use")
417-
expect((result[1] as ToolUse).name).toBe("read_file")
418-
expect(result[2].type).toBe("text")
419-
expect((result[2] as TextContent).content).toBe("-->")
420-
})
421-
422-
it("handles tool tags with HTML-like attributes", () => {
423-
const message = '<read_file class="example"><path>test.txt</path></read_file>'
424-
425-
// This should be treated as plain text since it's not a valid tool format
426-
const result = parser(message)
427-
expect(result).toHaveLength(1)
428-
expect(result[0].type).toBe("text")
429-
})
430-
431-
it("handles malformed tool tags with extra spaces", () => {
432-
const message = "< read_file >< path >test.txt< /path >< /read_file >"
433-
434-
// This should be treated as plain text since it's not a valid tool format
435-
const result = parser(message)
436-
expect(result).toHaveLength(1)
437-
expect(result[0].type).toBe("text")
438-
})
439-
})
440-
441-
describe("mixed formatting edge cases", () => {
442-
it("handles tool tags with markdown formatting", () => {
443-
const message = "**<read_file>**<path>test.txt</path></read_file>"
444-
445-
// The opening tag is malformed due to the bold formatting
446-
const result = parser(message)
447-
expect(result).toHaveLength(1)
448-
expect(result[0].type).toBe("text")
449-
})
450-
451-
it("handles tool tags in markdown links", () => {
452-
const message = "[Example](<read_file><path>test.txt</path></read_file>)"
453-
454-
// Note: Currently both parsers treat tool tags in markdown links as actual tool invocations
455-
// This test documents the current behavior rather than the ideal behavior
456-
const result = parser(message)
457-
expect(result).toHaveLength(3)
458-
expect(result[0].type).toBe("text")
459-
expect((result[0] as TextContent).content).toBe("[Example](")
460-
expect(result[1].type).toBe("tool_use")
461-
expect((result[1] as ToolUse).name).toBe("read_file")
462-
expect(result[2].type).toBe("text")
463-
expect((result[2] as TextContent).content).toBe(")")
464-
})
465-
})
466-
467-
describe("performance edge cases", () => {
468-
it("handles very long text with no tool uses", () => {
469-
const message = "A".repeat(10000)
470-
471-
const result = parser(message)
472-
expect(result).toHaveLength(1)
473-
expect(result[0].type).toBe("text")
474-
expect((result[0] as TextContent).content.length).toBe(10000)
475-
})
476-
477-
it("handles deeply nested tool parameters", () => {
478-
// Create a message with a tool that has many nested parameters
479-
let message = "<execute_command><command>"
480-
for (let i = 0; i < 10; i++) {
481-
message += `echo "Level ${i}" && `
482-
}
483-
message += 'echo "Done"</command></execute_command>'
484-
485-
const result = parser(message).filter((block) => !isEmptyTextContent(block))
486-
expect(result).toHaveLength(1)
487-
expect(result[0].type).toBe("tool_use")
488-
expect((result[0] as ToolUse).name).toBe("execute_command")
489-
})
490338
})
491339
})
492340
})

‎src/core/assistant-message/__tests__/parseAssistantMessageBenchmark.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,16 @@ const runBenchmark = () => {
8181
const namePadding = maxNameLength + 2
8282

8383
console.log(
84-
`| ${"Test Case".padEnd(namePadding)} | V1 Time (ms) | V2 Time (ms) | V1 Heap (bytes) | V2 Heap (bytes) |`,
84+
`| ${"Test Case".padEnd(namePadding)} | V1 Time (ms) | V2 Time (ms) | V1/V2 Ratio | V1 Heap (bytes) | V2 Heap (bytes) |`,
85+
)
86+
console.log(
87+
`| ${"-".repeat(namePadding)} | ------------ | ------------ | ----------- | ---------------- | ---------------- |`,
8588
)
86-
console.log(`| ${"-".repeat(namePadding)} | ------------ | ------------ | ---------------- | ---------------- |`)
8789

8890
for (const testCase of testCases) {
8991
const v1Time = measureExecutionTime(parseAssistantMessageV1, testCase.input)
9092
const v2Time = measureExecutionTime(parseAssistantMessageV2, testCase.input)
93+
const timeRatio = v1Time / v2Time
9194

9295
const v1Memory = measureMemoryUsage(parseAssistantMessageV1, testCase.input)
9396
const v2Memory = measureMemoryUsage(parseAssistantMessageV2, testCase.input)
@@ -96,6 +99,7 @@ const runBenchmark = () => {
9699
`| ${testCase.name.padEnd(namePadding)} | ` +
97100
`${v1Time.toFixed(4).padStart(12)} | ` +
98101
`${v2Time.toFixed(4).padStart(12)} | ` +
102+
`${timeRatio.toFixed(2).padStart(11)} | ` +
99103
`${formatNumber(Math.round(v1Memory.heapUsed)).padStart(16)} | ` +
100104
`${formatNumber(Math.round(v2Memory.heapUsed)).padStart(16)} |`,
101105
)

‎src/core/assistant-message/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,2 @@
11
export { type AssistantMessageContent, parseAssistantMessage } from "./parseAssistantMessage"
22
export { presentAssistantMessage } from "./presentAssistantMessage"
3-
export { parseAssistantMessageV2 } from "./parseAssistantMessageV2"

‎src/core/assistant-message/parseAssistantMessage.ts‎

Lines changed: 1 addition & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -13,52 +13,10 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag
1313
let currentParamValueStartIndex = 0
1414
let accumulator = ""
1515

16-
// Track whether we are inside markdown code blocks or inline code to avoid treating textual mentions
17-
// of tool tags (e.g. <read_file>) as actual tool invocations.
18-
let insideCodeBlock = false // ``` fenced code block
19-
let insideInlineCode = false // `inline code`
20-
21-
// Helper to decide if we should parse for tool-related tags at the current position
22-
const shouldParseToolTags = () => !insideCodeBlock && !insideInlineCode
23-
2416
for (let i = 0; i < assistantMessage.length; i++) {
2517
const char = assistantMessage[i]
26-
27-
// Detect fenced code block (```).
28-
if (!insideInlineCode && assistantMessage.slice(i, i + 3) === "```") {
29-
insideCodeBlock = !insideCodeBlock
30-
// Append the full trio of backticks to accumulator.
31-
accumulator += "```"
32-
i += 2 // Skip the two extra backticks we just added.
33-
34-
// When toggling code block state, continue to next iteration as
35-
// these chars are text.
36-
continue
37-
}
38-
39-
// Detect inline code (`) when not inside a fenced code block and not part of triple backticks
40-
if (!insideCodeBlock && char === "`") {
41-
insideInlineCode = !insideInlineCode
42-
}
43-
4418
accumulator += char
4519

46-
// If we are in any kind of code context, treat everything as plain text.
47-
if (!shouldParseToolTags()) {
48-
// Handle accumulating text content block.
49-
if (currentTextContent === undefined) {
50-
currentTextContentStartIndex = i
51-
}
52-
53-
currentTextContent = {
54-
type: "text",
55-
content: accumulator.slice(currentTextContentStartIndex).trim(),
56-
partial: true,
57-
}
58-
59-
continue
60-
}
61-
6220
// There should not be a param without a tool use.
6321
if (currentToolUse && currentParamName) {
6422
const currentParamValue = accumulator.slice(currentParamValueStartIndex)
@@ -87,7 +45,6 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag
8745
continue
8846
} else {
8947
const possibleParamOpeningTags = toolParamNames.map((name) => `<${name}>`)
90-
9148
for (const paramOpeningTag of possibleParamOpeningTags) {
9249
if (accumulator.endsWith(paramOpeningTag)) {
9350
// Start of a new parameter.
@@ -132,25 +89,6 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag
13289

13390
for (const toolUseOpeningTag of possibleToolUseOpeningTags) {
13491
if (accumulator.endsWith(toolUseOpeningTag)) {
135-
// Check that this is likely an actual tool invocation and not
136-
// an inline textual reference.
137-
// We consider it an invocation only if the next non-whitespace
138-
// character is a newline (\n or \r) or an opening angle bracket
139-
// '<' (which would start the first parameter tag).
140-
141-
let j = i + 1 // Position after the closing '>' of the opening tag.
142-
143-
while (j < assistantMessage.length && assistantMessage[j] === " ") {
144-
j++
145-
}
146-
147-
const nextChar = assistantMessage[j] ?? ""
148-
149-
if (nextChar && nextChar !== "<" && nextChar !== "\n" && nextChar !== "\r") {
150-
// Treat as plain text, not a tool invocation.
151-
continue
152-
}
153-
15492
// Start of a new tool use.
15593
currentToolUse = {
15694
type: "tool_use",
@@ -213,6 +151,5 @@ export function parseAssistantMessage(assistantMessage: string): AssistantMessag
213151
contentBlocks.push(currentTextContent)
214152
}
215153

216-
// Remove any empty text blocks that may have been created by whitespace or newlines before/after tool calls
217-
return contentBlocks.filter((block) => !(block.type === "text" && block.content.trim().length === 0))
154+
return contentBlocks
218155
}

‎src/core/assistant-message/parseAssistantMessageV2.ts‎

Lines changed: 17 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,6 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess
4646
let currentParamValueStart = 0 // Index *after* the opening tag of the current param.
4747
let currentParamName: ToolParamName | undefined = undefined
4848

49-
// Track whether we are inside markdown code blocks or inline code to avoid treating textual mentions
50-
// of tool tags (e.g. <read_file>) as actual tool invocations.
51-
let insideCodeBlock = false // ``` fenced code block
52-
let insideInlineCode = false // `inline code`
53-
54-
// Helper to decide if we should parse for tool-related tags at the current position
55-
const shouldParseToolTags = () => !insideCodeBlock && !insideInlineCode
56-
5749
// Precompute tags for faster lookups.
5850
const toolUseOpenTags = new Map<string, ToolName>()
5951
const toolParamOpenTags = new Map<string, ToolParamName>()
@@ -71,39 +63,16 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess
7163
for (let i = 0; i < len; i++) {
7264
const currentCharIndex = i
7365

74-
// Detect fenced code block (```).
75-
if (!insideInlineCode && i + 2 < len && assistantMessage.slice(i, i + 3) === "```") {
76-
insideCodeBlock = !insideCodeBlock
77-
i += 2 // Skip the two extra backticks.
78-
continue
79-
}
80-
81-
// Detect inline code (`) when not inside a fenced code block and not
82-
// part of triple backticks.
83-
if (!insideCodeBlock && assistantMessage[i] === "`") {
84-
insideInlineCode = !insideInlineCode
85-
}
86-
87-
// If we are in any kind of code context, treat everything as plain text.
88-
if (!shouldParseToolTags()) {
89-
// If we're not already tracking text content, start now.
90-
if (!currentTextContent) {
91-
currentTextContentStart = currentCharIndex
92-
currentTextContent = { type: "text", content: "", partial: true }
93-
}
94-
95-
continue
96-
}
97-
9866
// Parsing a tool parameter
9967
if (currentToolUse && currentParamName) {
10068
const closeTag = `</${currentParamName}>`
101-
102-
// Check if the string *ending* at index `i` matches the closing tag.
69+
// Check if the string *ending* at index `i` matches the closing tag
10370
if (
10471
currentCharIndex >= closeTag.length - 1 &&
105-
// Start checking from potential start of tag.
106-
assistantMessage.startsWith(closeTag, currentCharIndex - closeTag.length + 1)
72+
assistantMessage.startsWith(
73+
closeTag,
74+
currentCharIndex - closeTag.length + 1, // Start checking from potential start of tag.
75+
)
10776
) {
10877
// Found the closing tag for the parameter.
10978
const value = assistantMessage
@@ -206,23 +175,6 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess
206175
currentCharIndex >= tag.length - 1 &&
207176
assistantMessage.startsWith(tag, currentCharIndex - tag.length + 1)
208177
) {
209-
// Check that this is likely an actual tool invocation and not
210-
// an inline textual reference.
211-
// We consider it an invocation only if the next non-whitespace
212-
// character is a newline (\n or \r) or an opening angle bracket
213-
// '<' (which would start the first parameter tag).
214-
let j = currentCharIndex + 1 // Position after the closing '>' of the opening tag.
215-
216-
while (j < len && assistantMessage[j] === " ") {
217-
j++
218-
}
219-
220-
const nextChar = assistantMessage[j] ?? ""
221-
222-
if (nextChar && nextChar !== "<" && nextChar !== "\n" && nextChar !== "\r") {
223-
// Treat as plain text, not a tool invocation.
224-
continue
225-
}
226178
// End current text block if one was active.
227179
if (currentTextContent) {
228180
currentTextContent.content = assistantMessage
@@ -249,13 +201,22 @@ export function parseAssistantMessageV2(assistantMessage: string): AssistantMess
249201
.trim()
250202

251203
if (potentialText.length > 0) {
252-
contentBlocks.push({ type: "text", content: potentialText, partial: false })
204+
contentBlocks.push({
205+
type: "text",
206+
content: potentialText,
207+
partial: false,
208+
})
253209
}
254210
}
255211

256212
// Start the new tool use.
257-
// Assume partial until closing tag is found.
258-
currentToolUse = { type: "tool_use", name: toolName, params: {}, partial: true }
213+
currentToolUse = {
214+
type: "tool_use",
215+
name: toolName,
216+
params: {},
217+
partial: true, // Assume partial until closing tag is found.
218+
}
219+
259220
currentToolUseStart = currentCharIndex + 1 // Tool content starts after the opening tag.
260221
startedNewTool = true
261222

0 commit comments

Comments
 (0)