CAMEL-23955: Fix conversation memory user turns and history reset#24880
Conversation
Persist the outgoing user message in conversation history and clear history via exchange property when systemMessage is configured. Co-authored-by: Cursor <cursoragent@cursor.com>
gnodet
left a comment
There was a problem hiding this comment.
Review — CAMEL-23955: Fix conversation memory user turns and history reset
Well-researched and focused fix for two real bugs. Both root causes are correctly identified and the approach is sound.
What works well
- Precise diagnosis:
removeHeader()→removeProperty()is the exact fix for the history-reset bug, and the pending-user-message pattern correctly preserves user turns. - Test strategy: both tests capture the actual HTTP request and assert on the serialized payload — testing real user-visible behavior rather than internal state. The two-turn conversation test (
"My name is Alice"→"What is my name?") is an effective reproducer. - Consistency:
appendPendingUserMessageToHistory()is called in all threeupdateConversationHistoryoverloads, so user messages are preserved across simple, agentic-with-tools, and agentic-without-tools paths.
Suggestions
-
Upgrade guide entry (
docs/user-manual/modules/ROOT/pages/camel-4x-upgrade-guide-4_22.adoc): This changes observable behavior — conversation history now contains user messages where it previously only contained assistant responses. Code readingCamelOpenAIConversationHistorydirectly may need adjustment. Per project convention, a short entry documenting both fixes would be helpful. -
Minor edge case — property cleanup on API failure: If the OpenAI API call throws (between
buildMessages()settingPENDING_USER_MESSAGEandupdateConversationHistory()consuming it), the property remains on the exchange. This is low risk in practice (exceptions propagate, retry logic typically creates new exchanges), but a defensiveexchange.removeProperty(PENDING_USER_MESSAGE)at the start ofbuildMessages()would prevent stale state on exchange reuse. -
Nit — test assertion style: The project prefers AssertJ assertions. The current JUnit assertions (
assertTrue,assertFalse,assertNull) are fine but AssertJ would give more readable failure messages.
Checklist
| Item | Status |
|---|---|
| Tests | ✅ Two focused tests covering both bugs |
| Docs / upgrade guide | |
| Commit convention | ✅ CAMEL-23955: ... |
| Public API / backward compat | ✅ Bugfix aligning with documented contract |
| Security | ✅ No concerns — per-exchange, not cross-exchange |
| CI | ⏳ Not yet run — needs verification |
| Thread safety | ✅ Exchange processing is synchronous |
LGTM with the minor suggestions above. Nice work identifying and fixing these two interconnected bugs. 👍
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Add 4.22 upgrade guide entry, clear pending user message at buildMessages start, and switch tests to AssertJ. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Review feedback is applied, tests pass, and the branch is pushed. Changes Made
Tests Passed
Build: Pushed
|
gnodet
left a comment
There was a problem hiding this comment.
All three suggestions from the previous review have been addressed — upgrade guide entry added, defensive property cleanup at the start of buildMessages(), and tests migrated to AssertJ. Clean follow-up. 👍
Reviewed with Claude Code on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
There was a problem hiding this comment.
Both fixes are correct, well-tested, and documented in the upgrade guide.
Bug 1 — History reset was a no-op: in.removeHeader() was used to clear conversation history that's stored as an exchange property (exchange.getProperty()). This mismatch has been present since the original component commit (ac0b0e87f), so the documented reset behavior (systemMessage + conversationMemory=true clears history) never actually worked. The fix to exchange.removeProperty() is straightforward and correct.
Bug 2 — User messages missing from history: Only assistant responses were appended to the conversation history, so on subsequent turns the model saw answers without the questions that produced them. The PENDING_USER_MESSAGE exchange property approach is clean — set during buildMessages(), consumed and removed in appendPendingUserMessageToHistory(), and cleared at the start of each buildMessages() call to prevent leakage. All three updateConversationHistory() overloads are updated consistently.
Tests are well-structured reproducers for both scenarios. Upgrade guide entry properly documents the behavioral change.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of davsclaus
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 10 tested, 28 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
CAMEL-23955 — Summary
Branch:
CAMEL-23955-conversation-memory-fixCommit:
95e5ee478a7Module:
camel-openaiBugs Fixed
systemMessagereset had no effectremoveHeader()used instead ofremoveProperty()Code Changes
File modified:
OpenAIProducer.java(+20 lines)in.removeHeader(...)exchange.removeProperty(...)PENDING_USER_MESSAGEinbuildMessages()appendPendingUserMessageToHistory()runs in all 3 update pathsFiles added (from Jira reproducers):
OpenAIConversationMemoryUserMessageTest.javaOpenAIConversationMemoryResetTest.javaTests Passed
OpenAIConversationMemoryUserMessageTest"My name is Alice")OpenAIConversationMemoryResetTestsystemMessage+conversationMemory=trueclears stale historyOpenAIProducerMcpMockTest#conversationMemoryIncludesToolCallChainBuild:
BUILD SUCCESS— 3 tests, 0 failuresFlow After Fix