fix(core):prevent openAIConversationMerger merge history message with MessageRole#737
fix(core):prevent openAIConversationMerger merge history message with MessageRole#737feelshana wants to merge 6 commits intoagentscope-ai:mainfrom
Conversation
Summary of ChangesHello @feelshana, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request successfully removes the usage of MessageRole when merging conversation history, using only the agent name as a prefix. This aligns with the goal of treating all non-LLM messages as user messages. The implementation is mostly correct, but I've found a couple of areas for improvement. First, a parameter has become unused due to the changes and should be removed for code cleanliness. Second, there's a minor bug in handling ToolResultBlock where a null agent name would result in the string "null" being printed. I've provided suggestions for both issues.
I am having trouble creating individual review comments. Click here to see my feedback.
agentscope-core/src/main/java/io/agentscope/core/formatter/openai/OpenAIConversationMerger.java (295)
If agentName is null, textBuffer.append(agentName) will append the literal string "null", leading to an output like null (tool_name): .... This is likely not the intended behavior. You should handle the case where agentName is null.
textBuffer.append(agentName != null ? agentName : "");
agentscope-core/src/main/java/io/agentscope/core/formatter/openai/OpenAIConversationMerger.java (135-138)
With the removal of roleLabel, the roleFormatter parameter in processMessage is no longer used. To improve code clarity and maintainability, it should be removed from the method signature. This change should be propagated up to mergeToUserMessage and its callers, including updating the tests.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
fix(#731)
prevent openAIConversationMerger merge history message with MessageRole
main
Description
This prevents confusion: ASSISTANT should only mean "my LLM replied to me" .
Messages from other agents should be treated as USER messages
Checklist
Please check the following items before code is ready to be reviewed.
mvn spotless:applymvn test)