Add the summary message as a user message#1693
Conversation
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Review Complete - No Issues Found
I've analyzed the change and confirmed this is a correct bug fix.
What This Fixes
After session compaction, when all messages before the summary are removed, there's a scenario where no user messages remain in the active session history. Since the summary was previously a MessageRoleSystem, the message array sent to the LLM API could contain:
- System messages (instructions)
- Summary as system message
- Only assistant/tool messages
This violates LLM API requirements (OpenAI, Claude, etc.) that require at least one user message in the conversation.
The Solution
Changing the summary message role from MessageRoleSystem to MessageRoleUser ensures there's always at least one user message present, preventing API errors that would break the session entirely.
Trade-offs Considered
The summary now counts against the maxItems limit in trimMessages(), but this is acceptable because:
- The summary represents previous conversation context and should count as conversation history
- The content is clearly prefixed with "Session Summary:" so the LLM understands its nature
- Without this fix, the API call would fail completely
Verdict: This change correctly fixes a real bug. Looks good! 🚀
This fixes the case where the session was compacted and no user messages were present in the session any more. There was no real reason why this was a system prompt in the first place... Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
rumpl
force-pushed
the
session-compaction-user-message
branch
from
b90a257 to
ecc4dfb
Compare
2 participants
This fixes the case where the session was compacted and no user messages were present in the session any more.
There was no real reason why this was a system prompt in the first place...