GH-4170 Fixed the issue where SystemMessage was not being saved to the chat memory

[sunyuhan1998]

As discussed in the issue, the current implementation of MessageChatMemoryAdvisor does not save the SystemMessage into ChatMemory, which is clearly a BUG.

This PR fixes the issue by checking, when adding chat memory, whether the current conversation is the first turn or whether the current SystemMessage differs from the previous one. If either condition is met, the SystemMessage will be added to the chat memory.

Fixes #4170

[sjohnr]

@sunyuhan1998 I admit I'm new to this project, but I don't think the solution here is the correct one.

The system message is maintained in the ChatClient and is part of the system's configuration, not part of the chat memory. It is automatically added to the prompt of the ChatClientRequest and is available before the MessageChatMemoryAdvisor runs. The logic in MessageChatMemoryAdvisor#before is indeed the culprit, but I don't think adding the system message to chat memory makes sense or will help.

[sunyuhan1998]

Hi @sjohnr Thanks for your review!

I think this issue might stem from the inconsistency in design between org.springframework.ai.chat.memory.MessageWindowChatMemory and org.springframework.ai.chat.client.advisor.MessageChatMemoryAdvisor. If we consider SystemMessage as not part of the conversation memory, then certain aspects of MessageWindowChatMemory's design appear redundant. Conversely, if SystemMessage should be included in the memory, then the way MessageChatMemoryAdvisor adds messages to memory may be flawed. @markpollack , @ThomasVitale , what are your thoughts or suggestions on this matter?

[markpollack]

I'll look into it shortly and get back to folks. I appreciate all the hard work on the thread and the PR.

[ThomasVitale]

@sunyuhan1998 thanks for looking into this issue. The memory advisors are not currently designed to persist the SystemMessages. Changing that would be a breaking change from a behaviour perspective and I would recommend not doing it.

The reason why MessageWindowChatMemory includes logic to handle SystemMessages is because it's a core API that should support more general memory handling. It's up to the "caller" to decide what to persist. That logic is not used by the built-in advisors (which were there before MessageWindowChatMemory was introduced), but it can potentially be used by other implementations. So it's worth fixing the issue you mentioned in #4170 (comment) about the message ordering. Thank you for reporting that!

There is still a pending-design idea to introduce memory as a "first-class citizen" in ChatClient. As part of that work, it's good to review the requirements and have all messages sent to the memory. That's what I had in this initial draft: #2803 It might be a good time to pick this up again, together with the short-term memory/long-term memory/history discussion. @markpollack what do you think?

[sunyuhan1998]

@ThomasVitale Thank you for the detailed explanation. I now have a clear understanding of the original design intent, as well as the current state of MessageWindowChatMemory and MessageChatMemoryAdvisor. I fully agree that conversation memory should be treated as a "first-class citizen" and designed more systematically and thoroughly. Indeed, discussions around long-term and short-term memory mechanisms have come up repeatedly across multiple issues, and the current conversation memory design has indeed caused confusion for many users. As one of the core capabilities of a framework like Spring AI, effective conversation memory management is crucial. I believe it's now the right time to revisit and refine this component. My sincere appreciation goes to the team for all the hard work so far, and I will continue to follow the progress of this capability closely.