Refactor Assistant Message Parsing for Issue #4692 #4693
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed increased log level
…conflict with numerous other things that just use the default.
- Add TextDirective alias for TextContent - Add ToolDirective alias for ToolUse - Add Directive union type for TextDirective | ToolDirective - Maintain backward compatibility with AssistantMessageContent alias - Update internal usage to use new type names
- Extract parsing logic from monolithic parseAssistantMessage function - Create StreamingParser with specialized handler classes: - TextContentHandler: handles text content parsing - ToolUseHandler: manages tool directive detection and parsing - ParameterHandler: processes tool parameters - Maintain original streaming behavior and backward compatibility - All tests pass, no breaking changes - Improve code organization and maintainability
- Extract TextContentHandler into TextContentHandler.ts - Extract ParameterHandler into ParameterHandler.ts - Extract ToolUseHandler into ToolUseHandler.ts - Create types.ts for shared type definitions - Update StreamingParser to import from separate files - Update index.ts exports to reference correct files - Update README.md to reflect new file structure - Maintain backward compatibility and streaming behavior - All tests pass, multi-contributor friendly structure
…d names - Moved StreamingParser to DirectiveStreamingParser in assistant-message/ - Renamed directives/ to parsers/ - Created new parser classes with updated names: TextContentParser, ToolUseParser, ParameterParser - Simplified method names to parse() and finalize() - Updated documentation in README.md
- Changed imports in DirectiveStreamingParser.ts to use the index.ts file in parsers/ for a cleaner import structure
dosubot
bot
added
the
size:L
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| const toolContent = state.accumulator.slice(state.currentToolUseStartIndex) | ||
| const contentStartTag = `<${contentParamName}>` | ||
| const contentEndTag = `</${contentParamName}>` | ||
| const contentStartIndex = toolContent.indexOf(contentStartTag) + contentStartTag.length |
Contributor
There was a problem hiding this comment.
In the write_to_file special case, compute the index for the start tag separately before adding its length. As is, if indexOf returns -1, adding tag length yields a false positive. Consider storing the raw index and checking it before computing contentStartIndex.
Contributor
Author
There was a problem hiding this comment.
This is just a draft. I am reviewing more and I think this should be seriously refactored. PR coming after I am complete.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
removed
the
Issue/PR - Triage
Contributor
Author
|
I rewrote the whole thing will submit under a new PR. |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related GitHub Issue
Closes: #4692
Description
This PR refactors the assistant message parsing logic to improve maintainability and clarity, addressing issue #4692. The original monolithic structure in src/core/assistant-message/parseAssistantMessage.ts was difficult to maintain. The changes introduce a modular architecture with:
Test Procedure
pm test to confirm that all tests pass locally.
Type of Change
Pre-Submission Checklist
pm run lint).
pm test).
pm run changeset if this PR includes user-facing changes or dependency updates.
Screenshots / Videos
Not applicable for this change as it does not involve UI modifications.
Documentation Updates
Additional Notes
Please review the changes in src/core/assistant-message/ for the core refactoring of the parsing logic. The focus has been on ensuring backward compatibility and maintaining streaming behavior.
Get in Touch
Please provide your Discord username for reviewers or maintainers to reach you if they have questions about your PR.
Important
Refactor assistant message parsing by modularizing logic into specialized parsers, maintaining backward compatibility and streaming behavior.
DirectiveStreamingParserwith specialized parsers:TextContentParser,ToolUseParser,ParameterParser.src/core/assistant-message/parsers/.AssistantMessageContenttoDirectivewith alias for backward compatibility.parse()andfinalize().parseAssistantMessage.tsto useDirectiveStreamingParser.packages/evalsfiles to suppress prompts and update ports.This description was created by
for 2993a3a. You can customize this summary. It will automatically update as commits are pushed.