-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent tool parsing within code blocks #8244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 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
| } else { | ||
| // If we had one backtick and now a different char, toggle inline code | ||
| if (this.codeBlockDelimiterCount === 1 && !this.inCodeBlock) { | ||
| this.inInlineCode = !this.inInlineCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inline/code block tracking logic is duplicated here. The heuristic toggles inline code on the first non-backtick after a single backtick, which works in the tests but may be fragile in edge cases. Consider explicitly matching a closing backtick and extracting this logic to a shared utility to reduce duplication across parsers.
This comment was generated because it violated a code review rule: irule_tTqpIuNs8DV0QFGj.
| let inCodeBlock = false | ||
| let inInlineCode = false | ||
| let codeBlockDelimiterCount = 0 | ||
| let lastTwoChars = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'lastTwoChars' is computed but never used. Please remove it to keep the code clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like proofreading my own typos - I'll probably miss something obvious.
| let inCodeBlock = false | ||
| let inInlineCode = false | ||
| let codeBlockDelimiterCount = 0 | ||
| let lastTwoChars = "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lastTwoChars variable is declared on line 19 but never used in the implementation. This appears to be leftover from a different approach and should be removed to avoid confusion.
| } else { | ||
| // Check for inline code (single backtick) | ||
| if (codeBlockDelimiterCount === 1 && !inCodeBlock) { | ||
| inInlineCode = !inInlineCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider not toggling inInlineCode when already inside a code block. While the current logic works because inCodeBlock takes precedence, the inInlineCode state could become incorrect:
| inInlineCode = !inInlineCode | |
| // Check for inline code (single backtick) | |
| if (codeBlockDelimiterCount === 1 && !inCodeBlock) { | |
| inInlineCode = !inInlineCode | |
| } |
| } else { | ||
| // If we had one backtick and now a different char, toggle inline code | ||
| if (this.codeBlockDelimiterCount === 1 && !this.inCodeBlock) { | ||
| this.inInlineCode = !this.inInlineCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue here - consider not toggling inInlineCode when already in a code block:
| this.inInlineCode = !this.inInlineCode | |
| // If we had one backtick and now a different char, toggle inline code | |
| if (this.codeBlockDelimiterCount === 1 && !this.inCodeBlock) { | |
| this.inInlineCode = !this.inInlineCode | |
| } |
| } else { | ||
| // If we had one backtick and now a different char, toggle inline code | ||
| if (backtickCount === 1 && !inCodeBlock) { | ||
| inInlineCode = !inInlineCode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same pattern here - only toggle inline code when not in a code block:
| inInlineCode = !inInlineCode | |
| // If we had one backtick and now a different char, toggle inline code | |
| if (backtickCount === 1 && !inCodeBlock) { | |
| inInlineCode = !inInlineCode | |
| } |
Description
This PR fixes issue #8242 where the ask_followup_question tool (and other tools) were incorrectly triggered when displaying code snippets containing tool XML tags.
Problem
When an assistant in Ask mode was asked to describe tool usage and displayed example XML tags in code blocks, the parsing logic incorrectly interpreted these as actual tool invocations rather than code examples.
Solution
Added code block detection to all three message parsing implementations:
parseAssistantMessage.tsparseAssistantMessageV2.tsAssistantMessageParser.tsThe parsers now:
Testing
Changes
Fixes #8242
Important
Fixes issue #8242 by updating message parsers to prevent tool parsing within code blocks and inline code, with comprehensive tests added.
parseAssistantMessage.ts,parseAssistantMessageV2.ts, andAssistantMessageParser.ts.) and maintains state variables (inCodeBlock`, `inInlineCode`).parseAssistantMessage.spec.tsto cover various code block scenarios.This description was created by
for eb0add2. You can customize this summary. It will automatically update as commits are pushed.