-
Notifications
You must be signed in to change notification settings - Fork 197
History compression fix #394
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
History compression fix #394
Conversation
…tags to prevent unintended response patterns.
…d structured responses.
…rieveFactsFromHistory` logic.
…13880-improve-compression-history-2
Qodana for JVM178 new problems were found
@@ Code coverage @@
+ 56% total lines covered
6090 lines analyzed, 3447 lines covered
# Calculated according to the filters of your coverage tool ☁️ View the detailed Qodana report Detected 204 dependenciesThird-party software listThis page lists the third-party software dependencies used in koog-agents
Contact Qodana teamContact us at [email protected]
|
append("<${MemoryPrompts.historyWrapperTag}>\n") | ||
oldPrompt.messages.forEach { message -> | ||
when (message) { | ||
is Message.System -> append("<user>\n${message.content}\n</user>\n") |
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.
QQ: Is the "system" message converted to a "user" message intentionally? Do we ever need to preserve a system message in the history snippett?
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.
Yes, it was done intentionally to reduce its influence to LLM behavior. But we decided to keep it as user, because it may contain important context and task details to consider.
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.
@kpavlov without this change, LLM was following the system message instructions instead of summarization
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.
Thank you so much @denis-domanskii for working on this and for finding this problem!
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.
Looks super reasonable! Thank you for improvement!
The existing fact-retrieval strategy in history compression doesn't work because of LLM hallucination issues (tldr: after ~200 tool call messages and with old System message, LLM ignores "now summarize previous history" request and continues to do the original task).
To fix the issue, several changes were made:
Also I performed manual checks - looks fine.
Type of the change
Checklist for all pull requests
develop
as the base branchAdditional steps for pull requests adding a new feature