From b879dd21b694cb09205f00097f41c0a8d0c863e2 Mon Sep 17 00:00:00 2001 From: "Steven T. Cramer" Date: Mon, 16 Jun 2025 22:21:38 +0700 Subject: [PATCH] Fix DirectiveStreamingParser to prevent processing nested XML tags as directives inside tool parameters - Modified DirectiveStreamingParser to check if currently inside a tool parameter context - Added logic to treat nested XML tags as plain text when inside tool directive parameters - Made currentContext and currentParamName properties public in ToolDirectiveHandler for access - Updated both onopentag and onclosetag handlers to respect tool parameter boundaries - Fixes issue where tags inside were being processed as separate directives - All existing tests continue to pass, new test case now passes --- debug-log-issue.js | 58 +++ ...rective-streaming-parser-code-block-fix.md | 396 ++++++++++++++++++ junk.txt | 19 + .../DirectiveStreamingParser.ts | 23 +- .../directive-streaming-parser.spec.ts | 88 ++++ .../handlers/ToolDirectiveHandler.ts | 4 +- 6 files changed, 581 insertions(+), 7 deletions(-) create mode 100644 debug-log-issue.js create mode 100644 docs/directive-streaming-parser-code-block-fix.md create mode 100644 junk.txt diff --git a/debug-log-issue.js b/debug-log-issue.js new file mode 100644 index 0000000000..e19f8d2498 --- /dev/null +++ b/debug-log-issue.js @@ -0,0 +1,58 @@ +// Debug script to test the exact scenario from the image +const { DirectiveStreamingParser } = require("./src/core/message-parsing/DirectiveStreamingParser.ts") +const { FallbackParser } = require("./src/core/message-parsing/FallbackParser.ts") + +// Test the exact content that might be causing the issue +const testContent = `I'm happy to provide an example of the XML format for the log_message directive. As specified in the guidelines, log messages are used to output debugging information to the VSCode output channel. Here's an example of how it is formatted: + +In this example: + +The message tag contains the text of the log message. +The level tag specifies the log level, which can be "debug", "info", "warn", or "error". It's optional and defaults to "info" if not provided. + +This format allows for immediate logging without requiring user approval, and it can be used multiple times in a single message if needed. + + +I've provided an example of the XML format for the log_message directive. It includes the message content and an optional level specification, formatted as required for logging to the VSCode output channel. +` + +console.log("Testing DirectiveStreamingParser...") +const result1 = DirectiveStreamingParser.parse(testContent) +console.log("DirectiveStreamingParser result:") +result1.forEach((directive, idx) => { + console.log(` ${idx}: ${directive.type}`) + if (directive.type === "log_message") { + console.log(` āš ļø LOG MESSAGE FOUND: "${directive.message}"`) + } + if (directive.type === "tool_use") { + console.log(` Tool: ${directive.name}`) + console.log(` Params:`, Object.keys(directive.params)) + } +}) + +console.log("\nTesting FallbackParser...") +const result2 = FallbackParser.parse(testContent) +console.log("FallbackParser result:") +result2.forEach((directive, idx) => { + console.log(` ${idx}: ${directive.type}`) + if (directive.type === "log_message") { + console.log(` āš ļø LOG MESSAGE FOUND: "${directive.message}"`) + } + if (directive.type === "tool_use") { + console.log(` Tool: ${directive.name}`) + console.log(` Params:`, Object.keys(directive.params)) + } +}) + +// Check for any log messages +const allLogMessages1 = result1.filter((r) => r.type === "log_message") +const allLogMessages2 = result2.filter((r) => r.type === "log_message") + +console.log(`\nDirectiveStreamingParser found ${allLogMessages1.length} log messages`) +console.log(`FallbackParser found ${allLogMessages2.length} log messages`) + +if (allLogMessages1.length > 0 || allLogMessages2.length > 0) { + console.log("\nšŸšØ ISSUE FOUND: Log messages are still being parsed as directives!") +} else { + console.log("\nāœ… No log message directives found - this is correct!") +} diff --git a/docs/directive-streaming-parser-code-block-fix.md b/docs/directive-streaming-parser-code-block-fix.md new file mode 100644 index 0000000000..9675d68122 --- /dev/null +++ b/docs/directive-streaming-parser-code-block-fix.md @@ -0,0 +1,396 @@ +# DirectiveStreamingParser Code Block Fix - Implementation Plan + +## Problem Statement + +The test `"should not parse directives inside triple backticks as directives"` in [`src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts`](../src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts) is failing because the [`DirectiveStreamingParser`](../src/core/message-parsing/DirectiveStreamingParser.ts) currently parses XML directives inside code blocks (`...`) instead of treating them as plain text. + +### Current Behavior + +```` +Input: "Some text\n```\ncontent\n```\nMore text" +Output: [TextDirective, LogDirective, TextDirective] āŒ +```` + +### Expected Behavior + +```` +Input: "Some text\n```\ncontent\n```\nMore text" +Output: [TextDirective] āœ… (entire content as single text block) +```` + +## Streaming Complexity + +The parser must handle **true streaming scenarios** where code block boundaries can be split across message chunks: + +### Example Streaming Scenario + +```` +Chunk 1: "Some text with\n`" +Chunk 2: "``\n\n" +Chunk 3: "This is an example\nwarn" +Chunk 4: "\n\n```\nMore text" +```` + +**Challenge**: The ``` boundary is split across chunks 1-2, requiring stateful parsing. + +## Solution: Streaming-Aware State Machine + +### Architecture Overview + +````mermaid +flowchart TD + A[Streaming Text Input] --> B[Code Block State Machine] + B --> C{Current State} + C -->|OUTSIDE| D[Check for ``` Start] + C -->|INSIDE| E[Check for ``` End] + C -->|PARTIAL_START| F[Complete ``` Detection] + C -->|PARTIAL_END| G[Complete ``` End Detection] + + D -->|Found ```| H[Enter INSIDE State] + D -->|Partial `| I[Enter PARTIAL_START State] + D -->|Normal Text| J[Process as Text/XML] + + E -->|Found ```| K[Exit to OUTSIDE State] + E -->|Partial `| L[Enter PARTIAL_END State] + E -->|Normal Text| M[Accumulate as Plain Text] + + F -->|Complete ```| H + F -->|More `| F + F -->|Not ```| N[Revert to OUTSIDE + Process] + + G -->|Complete ```| K + G -->|More `| G + G -->|Not ```| M + + H --> O[Suppress XML Parsing] + K --> P[Resume XML Parsing] + M --> Q[Add to Code Block Content] + J --> R[Allow Directive Processing] +```` + +### State Machine Definition + +````typescript +enum CodeBlockState { + OUTSIDE = "outside", // Normal parsing mode + INSIDE = "inside", // Inside code block - suppress XML + PARTIAL_START = "partial_start", // Detected partial ``` at start + PARTIAL_END = "partial_end", // Detected partial ``` at end +} +```` + +## Implementation Plan + +### Phase 1: Extend ParseContext + +**File**: [`src/core/message-parsing/ParseContext.ts`](../src/core/message-parsing/ParseContext.ts) + +````typescript +export interface ParseContext { + currentText: string + contentBlocks: Directive[] + hasXmlTags: boolean + hasIncompleteXml: boolean + + // New code block state tracking + codeBlockState: CodeBlockState + pendingBackticks: string // For partial ``` detection + codeBlockContent: string // Accumulated content inside code blocks + codeBlockStartIndex: number // Track where code block started +} +```` + +### Phase 2: Create Code Block State Machine + +**New File**: `src/core/message-parsing/CodeBlockStateMachine.ts` + +```typescript +export interface ProcessedTextResult { + processedText: string + suppressXmlParsing: boolean + stateChanged: boolean +} + +export class CodeBlockStateMachine { + processText(text: string, context: ParseContext): ProcessedTextResult { + // Core state machine logic + // Handle all edge cases for partial boundaries + // Return processed text and parsing instructions + } + + private detectCodeBlockBoundary( + text: string, + startIndex: number, + ): { + found: boolean + endIndex: number + isComplete: boolean + } + + private handlePartialBoundary(text: string, context: ParseContext): void + + private transitionState(newState: CodeBlockState, context: ParseContext): void +} +``` + +### Phase 3: Enhanced TextDirectiveHandler + +**File**: [`src/core/message-parsing/handlers/TextDirectiveHandler.ts`](../src/core/message-parsing/handlers/TextDirectiveHandler.ts) + +```typescript +export class TextDirectiveHandler extends BaseDirectiveHandler { + private stateMachine = new CodeBlockStateMachine() + + override onText(text: string, context: ParseContext): void { + const result = this.stateMachine.processText(text, context) + + if (result.suppressXmlParsing) { + // Inside code block - accumulate as plain text + context.codeBlockContent += result.processedText + } else { + // Normal text processing + if (this.currentState === "text") { + context.currentText += result.processedText + } + } + } + + override onEnd(context: ParseContext): void { + // Handle any remaining code block content + if (context.codeBlockContent) { + context.currentText += context.codeBlockContent + } + + if (context.currentText.trim()) { + context.contentBlocks.push({ + type: "text", + content: context.currentText.trim(), + partial: true, + } as TextDirective) + } + } +} +``` + +### Phase 4: Parser-Level Integration + +**File**: [`src/core/message-parsing/DirectiveStreamingParser.ts`](../src/core/message-parsing/DirectiveStreamingParser.ts) + +```typescript +export class DirectiveStreamingParser { + static parse(assistantMessage: string): Directive[] { + const context: ParseContext = { + // ... existing fields + codeBlockState: CodeBlockState.OUTSIDE, + pendingBackticks: "", + codeBlockContent: "", + codeBlockStartIndex: -1, + } + + // ... existing parser setup + + parser.onopentag = (node: sax.Tag) => { + // Only process XML tags if NOT inside code block + if (context.codeBlockState !== CodeBlockState.INSIDE) { + // Existing XML processing logic + context.hasXmlTags = true + tagStack.push(node.name) + const handler = this.registry.getHandler(node.name) + // ... rest of existing logic + } else { + // Inside code block - treat as plain text + const tagText = `<${node.name}${this.attributesToString(node.attributes)}>` + this.registry.getTextHandler().onText(tagText, context) + } + } + + parser.onclosetag = (tagName: string) => { + if (context.codeBlockState !== CodeBlockState.INSIDE) { + // Existing close tag logic + } else { + // Inside code block - treat as plain text + this.registry.getTextHandler().onText(``, context) + } + } + + // ... rest of existing logic + } + + private attributesToString(attributes: { [key: string]: string }): string { + return Object.entries(attributes) + .map(([key, value]) => ` ${key}="${value}"`) + .join("") + } +} +``` + +## Edge Cases to Handle + +### 1. Partial Boundaries Across Chunks + +````typescript +// Chunk 1: "text `" +// Chunk 2: "``\ncontent" +// Expected: Detect complete ``` boundary +```` + +### 2. Multiple Code Blocks + +````typescript +// "text ```code1``` more ```code2``` end" +// Expected: Two separate code blocks, both suppressed +```` + +### 3. Nested Backticks + +````typescript +// "```\nSome `code` here\n```" +// Expected: Inner backticks treated as literal text +```` + +### 4. Malformed Boundaries + +```typescript +// "text `` incomplete" +// Expected: Treat as normal text, not code block +``` + +### 5. Mixed Content + +````typescript +// "text ```code``` content" +// Expected: Code block as text, directive processed normally +```` + +## Test Strategy + +### New Test Cases Required + +**File**: [`src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts`](../src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts) + +````typescript +describe("Code Block Handling", () => { + test("should handle partial code block boundaries across chunks", () => { + // Test streaming scenario with split boundaries + }) + + test("should handle multiple code blocks in single message", () => { + // Test multiple ```...``` blocks + }) + + test("should handle mixed content with code blocks and directives", () => { + // Test your example scenario + }) + + test("should handle nested backticks inside code blocks", () => { + // Test backticks within code blocks + }) + + test("should handle malformed code block boundaries", () => { + // Test incomplete or invalid ``` patterns + }) + + test("should maintain performance with large messages", () => { + // Performance regression test + }) +}) +```` + +### Existing Test Verification + +- āœ… All existing tests must continue to pass +- āœ… Specifically: `"should not parse directives inside triple backticks as directives"` +- āœ… No regression in normal directive parsing + +## Performance Considerations + +### Optimization Strategies + +1. **Lazy Activation**: Only activate state machine when backticks detected +2. **Efficient String Processing**: Minimize string concatenation overhead +3. **State Caching**: Cache frequently accessed state information +4. **Early Exit**: Skip processing when clearly outside code blocks + +### Performance Benchmarks + +- Measure parsing time for messages with/without code blocks +- Test memory usage with large messages containing multiple code blocks +- Verify no significant regression in normal parsing scenarios + +## Implementation Checklist + +### Phase 1: Core Infrastructure + +- [ ] Extend [`ParseContext`](../src/core/message-parsing/ParseContext.ts) with code block state +- [ ] Create `CodeBlockStateMachine` class +- [ ] Implement state transition logic +- [ ] Add comprehensive unit tests for state machine + +### Phase 2: Parser Integration + +- [ ] Modify [`DirectiveStreamingParser`](../src/core/message-parsing/DirectiveStreamingParser.ts) to check code block state +- [ ] Update XML tag processing to respect code block state +- [ ] Handle attribute serialization for suppressed tags +- [ ] Test parser-level integration + +### Phase 3: Handler Updates + +- [ ] Enhance [`TextDirectiveHandler`](../src/core/message-parsing/handlers/TextDirectiveHandler.ts) with state machine +- [ ] Update text processing logic +- [ ] Handle code block content accumulation +- [ ] Test handler-level functionality + +### Phase 4: Comprehensive Testing + +- [ ] Add all edge case tests +- [ ] Verify existing test compatibility +- [ ] Performance benchmarking +- [ ] Integration testing with real streaming scenarios + +### Phase 5: Documentation & Cleanup + +- [ ] Update code documentation +- [ ] Add inline comments for complex logic +- [ ] Performance optimization if needed +- [ ] Final integration testing + +## Risk Mitigation + +### Potential Issues + +1. **Complex State Management**: Multiple edge cases to handle +2. **Performance Impact**: Additional processing overhead +3. **Backward Compatibility**: Existing functionality must remain intact +4. **Memory Usage**: State persistence across chunks + +### Mitigation Strategies + +1. **Comprehensive Testing**: Cover all identified edge cases +2. **Performance Benchmarking**: Measure and optimize impact +3. **Gradual Rollout**: Feature flag for new behavior if needed +4. **Fallback Mechanism**: Graceful degradation on state machine errors + +## Success Criteria + +- āœ… Failing test `"should not parse directives inside triple backticks as directives"` passes +- āœ… All existing tests continue to pass +- āœ… Handles streaming scenarios with partial code block boundaries +- āœ… Performance impact < 10% for normal parsing scenarios +- āœ… Memory usage remains stable for large messages +- āœ… Comprehensive test coverage for all edge cases + +## Files to Modify/Create + +### Modified Files + +1. [`src/core/message-parsing/ParseContext.ts`](../src/core/message-parsing/ParseContext.ts) +2. [`src/core/message-parsing/DirectiveStreamingParser.ts`](../src/core/message-parsing/DirectiveStreamingParser.ts) +3. [`src/core/message-parsing/handlers/TextDirectiveHandler.ts`](../src/core/message-parsing/handlers/TextDirectiveHandler.ts) +4. [`src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts`](../src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts) + +### New Files + +1. `src/core/message-parsing/CodeBlockStateMachine.ts` +2. `src/core/message-parsing/__tests__/code-block-state-machine.spec.ts` + +This comprehensive plan addresses the streaming nature of the parser while ensuring robust handling of all edge cases related to code block detection and XML directive suppression. diff --git a/junk.txt b/junk.txt new file mode 100644 index 0000000000..6113a9ec31 --- /dev/null +++ b/junk.txt @@ -0,0 +1,19 @@ +Some text with +``` + +This is an example message that should display in text because it is in a code block +warn + +``` +More text + +This is a warning message that should be handled by the directive handler +warn + +" + + +Partials could have splits like +1. Some text with\n` +2. ``\n\n diff --git a/src/core/message-parsing/DirectiveStreamingParser.ts b/src/core/message-parsing/DirectiveStreamingParser.ts index 39ca47afd0..5486aa9f23 100644 --- a/src/core/message-parsing/DirectiveStreamingParser.ts +++ b/src/core/message-parsing/DirectiveStreamingParser.ts @@ -33,8 +33,14 @@ export class DirectiveStreamingParser { "isInsideParameterCodeBlock" in activeHandler && (activeHandler as any).isInsideParameterCodeBlock()) - // Only process XML tags if NOT inside code block - if (!insideCodeBlock) { + // Check if we're inside a tool parameter (but not at the parameter level itself) + const insideToolParameter = + activeHandler && + activeHandler.constructor.name === "ToolDirectiveHandler" && + (activeHandler as any).currentContext === "param" + + // Only process XML tags if NOT inside code block AND NOT inside tool parameter + if (!insideCodeBlock && !insideToolParameter) { context.hasXmlTags = true tagStack.push(node.name) const handler = this.registry.getHandler(node.name) @@ -47,7 +53,7 @@ export class DirectiveStreamingParser { activeHandler.onOpenTag(node, context) } } else { - // Inside code block - treat as plain text + // Inside code block or tool parameter - treat as plain text const tagText = `<${node.name}${this.attributesToString(node.attributes)}>` if (activeHandler) { activeHandler.onText(tagText, context) @@ -65,7 +71,14 @@ export class DirectiveStreamingParser { "isInsideParameterCodeBlock" in activeHandler && (activeHandler as any).isInsideParameterCodeBlock()) - if (!insideCodeBlock) { + // Check if we're inside a tool parameter (but not at the parameter level itself) + const insideToolParameter = + activeHandler && + activeHandler.constructor.name === "ToolDirectiveHandler" && + (activeHandler as any).currentContext === "param" && + tagName !== (activeHandler as any).currentParamName + + if (!insideCodeBlock && !insideToolParameter) { // Normal XML processing if (activeHandler) { activeHandler.onCloseTag(tagName, context) @@ -76,7 +89,7 @@ export class DirectiveStreamingParser { } tagStack.pop() } else { - // Inside code block - treat as plain text + // Inside code block or tool parameter - treat as plain text if (activeHandler) { activeHandler.onText(``, context) } else { 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 99585a6019..8ff4131ac3 100644 --- a/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts +++ b/src/core/message-parsing/__tests__/directive-streaming-parser.spec.ts @@ -209,4 +209,92 @@ suite("DirectiveStreamingParser", () => { expect((attemptCompletion as any).params.result).toContain("") } }) + + test("should handle real-world scenario with log message example", () => { + // Test a scenario similar to what's shown in the user's image + const input = `I'm happy to provide an example of the XML format for the log_message directive. + + +This is an example log message for demonstration purposes +info + + + +I've provided an example of the XML format for the log_message directive. +` + + const result = DirectiveStreamingParser.parse(input) + + // Should have text, log_message, and attempt_completion + expect(result).toHaveLength(3) + expect(result[0].type).toBe("text") + expect(result[1].type).toBe("log_message") + expect(result[2].type).toBe("tool_use") + + // The log message should be processed as a real directive (this is correct behavior) + expect((result[1] as any).message).toBe("This is an example log message for demonstration purposes") + expect((result[1] as any).level).toBe("info") + }) + + test("should NOT process log messages inside code blocks in attempt_completion", () => { + // Test the problematic scenario + const input = ` +Here's an example: + +\`\`\`xml + +This should NOT be processed as a log directive +debug + +\`\`\` + +That's the format. +` + + const result = DirectiveStreamingParser.parse(input) + + // Should only have the attempt_completion directive + expect(result).toHaveLength(1) + expect(result[0].type).toBe("tool_use") + expect((result[0] as any).name).toBe("attempt_completion") + + // The result should contain the log_message as plain text + expect((result[0] as any).params.result).toContain("") + expect((result[0] as any).params.result).toContain("This should NOT be processed as a log directive") + + // Most importantly: NO separate log_message directive should exist + const logMessages = result.filter((r) => r.type === "log_message") + expect(logMessages).toHaveLength(0) + }) + + test("should handle log message directly inside attempt_completion result", () => { + // Test the actual scenario from the user's image - log message directly inside attempt_completion result + const input = ` +I'm happy to provide an example of the XML format for the log_message directive. + + +This is an example log message for demonstration purposes +info + + +I've provided an example of the XML format for the log_message directive. +` + + const result = DirectiveStreamingParser.parse(input) + + console.log("Result:", JSON.stringify(result, null, 2)) + + // Should only have the attempt_completion directive + expect(result).toHaveLength(1) + expect(result[0].type).toBe("tool_use") + expect((result[0] as any).name).toBe("attempt_completion") + + // The result should contain the log_message as plain text (NOT as a separate directive) + expect((result[0] as any).params.result).toContain("") + expect((result[0] as any).params.result).toContain("This is an example log message for demonstration purposes") + + // Most importantly: NO separate log_message directive should exist + 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 50572fe2b9..f27674b5b8 100644 --- a/src/core/message-parsing/handlers/ToolDirectiveHandler.ts +++ b/src/core/message-parsing/handlers/ToolDirectiveHandler.ts @@ -7,9 +7,9 @@ import { CodeBlockStateMachine } from "../CodeBlockStateMachine" export class ToolDirectiveHandler extends BaseDirectiveHandler { readonly tagName: string private currentToolDirective?: ToolDirective - private currentParamName?: ToolParamName + public currentParamName?: ToolParamName private currentParamValue = "" - private currentContext: "param" | "none" = "none" + public currentContext: "param" | "none" = "none" private stateMachine = new CodeBlockStateMachine() private paramCodeBlockState: CodeBlockState = CodeBlockState.OUTSIDE