From 5ef186300eb4680a87bddf0a793dbe8d4ba326e0 Mon Sep 17 00:00:00 2001 From: "Steven T. Cramer" Date: Mon, 16 Jun 2025 22:01:18 +0700 Subject: [PATCH] Fix log message parsing inside code blocks within tool parameters - Enhanced DirectiveStreamingParser to track code block state within tool parameters - Fixed FallbackParser to respect code blocks and properly parse nested XML structures - Added comprehensive tests for both streaming and fallback scenarios - Prevents log messages inside code blocks from being processed as actual directives Fixes issue where tags inside code blocks within tool parameters were being parsed as separate log directives instead of plain text content. --- .../DirectiveStreamingParser.ts | 30 ++++++- src/core/message-parsing/FallbackParser.ts | 89 +++++++++++++++++-- .../directive-streaming-parser.spec.ts | 72 +++++++++++++++ .../__tests__/fallback-parser.spec.ts | 81 +++++++++++++++++ .../handlers/ToolDirectiveHandler.ts | 28 +++++- 5 files changed, 288 insertions(+), 12 deletions(-) create mode 100644 src/core/message-parsing/__tests__/fallback-parser.spec.ts diff --git a/src/core/message-parsing/DirectiveStreamingParser.ts b/src/core/message-parsing/DirectiveStreamingParser.ts index 0e64d64749..39ca47afd0 100644 --- a/src/core/message-parsing/DirectiveStreamingParser.ts +++ b/src/core/message-parsing/DirectiveStreamingParser.ts @@ -26,8 +26,15 @@ export class DirectiveStreamingParser { let activeHandler: any = null parser.onopentag = (node: sax.Tag) => { + // Check if we're inside a code block (either global or within tool parameters) + const insideCodeBlock = + context.codeBlockState === CodeBlockState.INSIDE || + (activeHandler && + "isInsideParameterCodeBlock" in activeHandler && + (activeHandler as any).isInsideParameterCodeBlock()) + // Only process XML tags if NOT inside code block - if (context.codeBlockState !== CodeBlockState.INSIDE) { + if (!insideCodeBlock) { context.hasXmlTags = true tagStack.push(node.name) const handler = this.registry.getHandler(node.name) @@ -42,12 +49,23 @@ export class DirectiveStreamingParser { } else { // Inside code block - treat as plain text const tagText = `<${node.name}${this.attributesToString(node.attributes)}>` - this.registry.getTextHandler().onText(tagText, context) + if (activeHandler) { + activeHandler.onText(tagText, context) + } else { + this.registry.getTextHandler().onText(tagText, context) + } } } parser.onclosetag = (tagName: string) => { - if (context.codeBlockState !== CodeBlockState.INSIDE) { + // Check if we're inside a code block (either global or within tool parameters) + const insideCodeBlock = + context.codeBlockState === CodeBlockState.INSIDE || + (activeHandler && + "isInsideParameterCodeBlock" in activeHandler && + (activeHandler as any).isInsideParameterCodeBlock()) + + if (!insideCodeBlock) { // Normal XML processing if (activeHandler) { activeHandler.onCloseTag(tagName, context) @@ -59,7 +77,11 @@ export class DirectiveStreamingParser { tagStack.pop() } else { // Inside code block - treat as plain text - this.registry.getTextHandler().onText(``, context) + if (activeHandler) { + activeHandler.onText(``, context) + } else { + this.registry.getTextHandler().onText(``, context) + } } } diff --git a/src/core/message-parsing/FallbackParser.ts b/src/core/message-parsing/FallbackParser.ts index d281640b15..998d693b2d 100644 --- a/src/core/message-parsing/FallbackParser.ts +++ b/src/core/message-parsing/FallbackParser.ts @@ -6,12 +6,34 @@ export class FallbackParser { static parse(assistantMessage: string): Directive[] { const contentBlocks: Directive[] = [] + // Check if we're inside code blocks before parsing log messages + const codeBlockRegex = /```[\s\S]*?```/g + const codeBlocks: Array<{ start: number; end: number }> = [] + let codeBlockMatch + + // Find all code block ranges + while ((codeBlockMatch = codeBlockRegex.exec(assistantMessage)) !== null) { + codeBlocks.push({ + start: codeBlockMatch.index, + end: codeBlockMatch.index + codeBlockMatch[0].length, + }) + } + + // Helper function to check if a position is inside a code block + const isInsideCodeBlock = (position: number): boolean => { + return codeBlocks.some((block) => position >= block.start && position < block.end) + } + // Handle multiple log messages const logMessageRegex = /([\s\S]*?)(?:<\/log_message>|$)/g let lastIndex = 0 let match while ((match = logMessageRegex.exec(assistantMessage)) !== null) { + // Skip log messages that are inside code blocks + if (isInsideCodeBlock(match.index)) { + continue + } // Add any text before this log message if (match.index > lastIndex) { const textBefore = assistantMessage.substring(lastIndex, match.index).trim() @@ -65,14 +87,69 @@ export class FallbackParser { const toolContent = toolMatch[0] const params: Record = {} - // Extract parameters - const paramRegex = /<(\w+)>(.*?)(?:<\/\1>|$)/g - let paramMatch - while ((paramMatch = paramRegex.exec(toolContent)) !== null) { - const [, paramName, paramValue] = paramMatch - if (paramName !== toolName) { + // Extract parameters - need to be more careful about nested structures + // Find direct child parameters of the tool, not nested ones + const toolInnerContent = toolContent + .replace(new RegExp(`^<${toolName}>`), "") + .replace(new RegExp(`$`), "") + + // Use a more sophisticated approach to find top-level parameters + let currentIndex = 0 + while (currentIndex < toolInnerContent.length) { + // Find the next opening tag + const tagMatch = toolInnerContent.substring(currentIndex).match(/<(\w+)>/) + if (!tagMatch) break + + const paramName = tagMatch[1] + const tagStart = currentIndex + tagMatch.index! + const contentStart = tagStart + tagMatch[0].length + + // Find the matching closing tag, accounting for nested tags + let depth = 1 + let searchIndex = contentStart + let paramValue = "" + + while (depth > 0 && searchIndex < toolInnerContent.length) { + const nextTag = toolInnerContent.substring(searchIndex).match(/<\/?(\w+)>/) + if (!nextTag) { + // No more tags, take the rest as content + paramValue = toolInnerContent.substring(contentStart) + break + } + + const tagName = nextTag[1] + const isClosing = nextTag[0].startsWith(" 0) { + // Unclosed tag, take the rest + paramValue = toolInnerContent.substring(contentStart) + params[paramName] = paramValue + break + } } const ToolDirective: ToolDirective = { diff --git a/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts b/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts index 51aa245d9d..99585a6019 100644 --- a/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts +++ b/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts @@ -137,4 +137,76 @@ suite("DirectiveStreamingParser", () => { } as TextDirective, ]) }) + + test("should not parse directives inside code blocks within tool directive parameters", () => { + const input = + "Here's the format:\n\n```xml\n\nThis should be plain text\ndebug\n\n```\n\nThat's the format." + const result = DirectiveStreamingParser.parse(input) + expect(result).toHaveLength(1) + expect(result[0].type).toBe("tool_use") + expect((result[0] as any).name).toBe("attempt_completion") + expect((result[0] as any).params.result).toContain("```xml") + expect((result[0] as any).params.result).toContain("") + expect((result[0] as any).params.result).toContain("This should be plain text") + // The key test: ensure it's treated as one text block, not parsed as separate directives + expect((result[0] as any).params.result).toBe( + "Here's the format:\n\n```xml\n\nThis should be plain text\ndebug\n\n```\n\nThat's the format.", + ) + }) + + test("should not parse directives inside code blocks within tool directive parameters during streaming", () => { + // Simulate streaming chunks + const chunks = [ + "", + "Here's the format:\n\n```xml\n", + "\nThis should be plain text\ndebug\n\n", + "```\n\nThat's the format.", + "", + ] + + let accumulatedMessage = "" + let finalResult: any[] = [] + + // Test each streaming chunk + for (const chunk of chunks) { + accumulatedMessage += chunk + const result = DirectiveStreamingParser.parse(accumulatedMessage) + finalResult = result + } + + // Final result should have only one directive (attempt_completion) + expect(finalResult).toHaveLength(1) + expect(finalResult[0].type).toBe("tool_use") + expect(finalResult[0].name).toBe("attempt_completion") + + // The result parameter should contain the log_message as plain text + expect(finalResult[0].params.result).toContain("") + expect(finalResult[0].params.result).toContain("This should be plain text") + + // Most importantly: there should be NO separate log_message directive + const logMessages = finalResult.filter((r: any) => r.type === "log_message") + expect(logMessages).toHaveLength(0) + }) + + test("should handle malformed XML that might trigger FallbackParser", () => { + // Test a scenario that might cause parse errors and trigger FallbackParser + const input = + "Here's the format:\n\n```xml\n\nThis should be plain text\ndebug\n\n```\n\nThat's the format." + + // Add some malformed XML to potentially trigger fallback + const malformedInput = input + "" + + const result = DirectiveStreamingParser.parse(malformedInput) + + // Should still not parse log_message as separate directive + const logMessages = result.filter((r: any) => r.type === "log_message") + expect(logMessages).toHaveLength(0) + + // Should have attempt_completion with log_message preserved as text + const attemptCompletion = result.find((r: any) => r.type === "tool_use" && r.name === "attempt_completion") + expect(attemptCompletion).toBeDefined() + if (attemptCompletion && attemptCompletion.type === "tool_use") { + expect((attemptCompletion as any).params.result).toContain("") + } + }) }) diff --git a/src/core/message-parsing/__tests__/fallback-parser.spec.ts b/src/core/message-parsing/__tests__/fallback-parser.spec.ts new file mode 100644 index 0000000000..0a8502c83d --- /dev/null +++ b/src/core/message-parsing/__tests__/fallback-parser.spec.ts @@ -0,0 +1,81 @@ +import { FallbackParser } from "../FallbackParser" +import { LogDirective, TextDirective } from "../directives" + +describe("FallbackParser", () => { + test("should not parse log messages inside code blocks", () => { + const input = `Here's the format: + +\`\`\`xml + +This should be plain text +debug + +\`\`\` + +That should be treated as code.` + + const result = FallbackParser.parse(input) + + // Should only have text directive, no log message directive + expect(result).toHaveLength(1) + expect(result[0].type).toBe("text") + expect((result[0] as TextDirective).content).toContain("") + expect((result[0] as TextDirective).content).toContain("This should be plain text") + }) + + test("should parse log messages outside code blocks", () => { + const input = `Some text + + +This is a real log message +info + + +More text with code: + +\`\`\`xml + +This should be ignored +debug + +\`\`\` + +End text.` + + const result = FallbackParser.parse(input) + + // Should have text + log message + text + expect(result).toHaveLength(3) + expect(result[0].type).toBe("text") + expect(result[1].type).toBe("log_message") + expect((result[1] as LogDirective).message).toBe("This is a real log message") + expect(result[2].type).toBe("text") + expect((result[2] as TextDirective).content).toContain("This should be ignored") + }) + + test("should handle attempt_completion with log messages in code blocks", () => { + const input = `Here's the format: + +\`\`\`xml + +This should be plain text +debug + +\`\`\` + +That's the format.` + + const result = FallbackParser.parse(input) + + // Should have tool directive, no separate log message + expect(result).toHaveLength(1) + expect(result[0].type).toBe("tool_use") + expect((result[0] as any).name).toBe("attempt_completion") + expect((result[0] as any).params.result).toContain("") + expect((result[0] as any).params.result).toContain("This should be plain text") + + // No separate log message directive + const logMessages = result.filter((r) => r.type === "log_message") + expect(logMessages).toHaveLength(0) + }) +}) diff --git a/src/core/message-parsing/handlers/ToolDirectiveHandler.ts b/src/core/message-parsing/handlers/ToolDirectiveHandler.ts index 4f37558d80..50572fe2b9 100644 --- a/src/core/message-parsing/handlers/ToolDirectiveHandler.ts +++ b/src/core/message-parsing/handlers/ToolDirectiveHandler.ts @@ -1,7 +1,8 @@ import * as sax from "sax" import { BaseDirectiveHandler } from "./BaseDirectiveHandler" -import { ParseContext } from "../ParseContext" +import { ParseContext, CodeBlockState } from "../ParseContext" import { ToolDirective, ToolParamName } from "../directives" +import { CodeBlockStateMachine } from "../CodeBlockStateMachine" export class ToolDirectiveHandler extends BaseDirectiveHandler { readonly tagName: string @@ -9,6 +10,8 @@ export class ToolDirectiveHandler extends BaseDirectiveHandler { private currentParamName?: ToolParamName private currentParamValue = "" private currentContext: "param" | "none" = "none" + private stateMachine = new CodeBlockStateMachine() + private paramCodeBlockState: CodeBlockState = CodeBlockState.OUTSIDE constructor(toolName: string) { super() @@ -29,6 +32,8 @@ export class ToolDirectiveHandler extends BaseDirectiveHandler { this.currentParamName = node.name as ToolParamName this.currentParamValue = "" this.currentContext = "param" + // Reset code block state for new parameter + this.paramCodeBlockState = CodeBlockState.OUTSIDE } } @@ -49,7 +54,19 @@ export class ToolDirectiveHandler extends BaseDirectiveHandler { override onText(text: string, context: ParseContext): void { if (this.currentContext === "param" && this.currentParamName && this.currentToolDirective) { - this.currentParamValue += text + // Create a temporary context to track code block state within this parameter + const tempContext = { + ...context, + codeBlockState: this.paramCodeBlockState, + } + + // Process text through the code block state machine + const result = this.stateMachine.processText(text, tempContext) + + // Update our parameter-specific code block state + this.paramCodeBlockState = tempContext.codeBlockState + + this.currentParamValue += result.processedText } } @@ -63,4 +80,11 @@ export class ToolDirectiveHandler extends BaseDirectiveHandler { context.contentBlocks.push(this.currentToolDirective) } } + + /** + * Check if we're currently inside a code block within a tool parameter + */ + isInsideParameterCodeBlock(): boolean { + return this.currentContext === "param" && this.paramCodeBlockState === CodeBlockState.INSIDE + } }