fix(core): prevent duplicate step persistence messages in memory#1123
Merged
fix(core): prevent duplicate step persistence messages in memory#1123
Conversation
🦋 Changeset detectedLatest commit: 01bae0f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment has been minimized.
This comment has been minimized.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds per-operation fingerprinting and stable assistant response message IDs to deduplicate assistant/tool messages across step checkpoints, preventing duplicate persistence during intermediate flushes and ensuring checkpoint flushes update the same memory message. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Step as Step Handler
participant Agent as Agent
participant Buffer as ConversationBuffer / MemoryPersistQueue
participant Memory as MemoryManager / DB
Note over Step,Agent: Step produces response messages (streaming or final)
Step->>Agent: emit responseMessages
Agent->>Agent: normalizeStepResponseMessages (fingerprint dedupe, ensure stable messageId)
Agent->>Buffer: buffer.save(normalizedMessages) (uses stable messageId)
Buffer->>Memory: saveMessage/upsert (persists single message per stable id)
Memory-->>Buffer: ack
Buffer-->>Agent: persistence complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying voltagent with
|
| Latest commit: |
01bae0f
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://457b874d.voltagent.pages.dev |
| Branch Preview URL: | https://fix-1121-duplicate-memory-me.voltagent.pages.dev |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/src/agent/step-persistence.spec.ts">
<violation number="1" location="packages/core/src/agent/step-persistence.spec.ts:267">
P2: The test can pass even when assistant message IDs are missing (`undefined`), so it does not fully verify stable ID reuse.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/agent/step-persistence.spec.ts (1)
145-148: Extract the new test harness setup to reuse the existingcreateHarness()helper or define type-safe mock interfaces.The new test case (lines 133–161) duplicates harness initialization with
as anycasts on lines 145, 147, and 159. Either reuse the existingcreateHarness()helper or define narrow local test interfaces (e.g.,Pick<MemoryManager, 'saveMessage'>) instead of casting toanyto maintain type safety per coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/step-persistence.spec.ts` around lines 145 - 148, The test duplicates a harness setup and uses unsafe as any casts for memoryManager and oc.logger when instantiating MemoryPersistQueue; replace this by reusing the existing createHarness() helper to produce a properly typed memoryManager and logger, or create narrow mock types (e.g., const memoryManager: Pick<MemoryManager, 'saveMessage' | 'loadMessages'> = { saveMessage: jest.fn(), loadMessages: jest.fn() } and a typed logger) and pass those into new MemoryPersistQueue so you can remove all as any casts and keep the test type-safe; update references to MemoryPersistQueue, createHarness, memoryManager, and logger accordingly.packages/core/src/agent/agent.ts (1)
3667-3671: Use stable ID for assistant message deduplication to handle provider ID variance.At line 3669, assistant message fingerprints include provider-emitted IDs. The Vercel AI SDK does not guarantee assistant message IDs remain stable across intermediate steps—they can change per step during streaming. If the same assistant content arrives with different provider IDs across steps, it won't dedupe.
Apply the suggested adjustment to use
fallbackAssistantMessageIdfor assistant message fingerprints, ensuring deduplication works correctly regardless of upstream ID changes:Suggested adjustment
const fingerprint = safeStringify({ role: normalizedMessage.role, - id: (normalizedMessage as { id?: unknown }).id ?? null, + id: + normalizedMessage.role === "assistant" + ? fallbackAssistantMessageId + : ((normalizedMessage as { id?: unknown }).id ?? null), content: normalizedMessage.content, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/agent.ts` around lines 3667 - 3671, The fingerprint currently includes provider-emitted IDs from normalizedMessage.id which can change during streaming; update the fingerprint logic in the agent message dedupe to use fallbackAssistantMessageId when normalizedMessage.role === 'assistant' (i.e., set id = (normalizedMessage.role === 'assistant' ? (normalizedMessage as any).fallbackAssistantMessageId : (normalizedMessage as { id?: unknown }).id) ?? null) so assistant messages use the stable fallbackAssistantMessageId for deduplication while other roles keep their id or null; update references in the fingerprint creation (function/variable names: fingerprint, normalizedMessage, fallbackAssistantMessageId) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/agent/agent.ts`:
- Around line 3667-3671: The fingerprint currently includes provider-emitted IDs
from normalizedMessage.id which can change during streaming; update the
fingerprint logic in the agent message dedupe to use fallbackAssistantMessageId
when normalizedMessage.role === 'assistant' (i.e., set id =
(normalizedMessage.role === 'assistant' ? (normalizedMessage as
any).fallbackAssistantMessageId : (normalizedMessage as { id?: unknown }).id) ??
null) so assistant messages use the stable fallbackAssistantMessageId for
deduplication while other roles keep their id or null; update references in the
fingerprint creation (function/variable names: fingerprint, normalizedMessage,
fallbackAssistantMessageId) accordingly.
In `@packages/core/src/agent/step-persistence.spec.ts`:
- Around line 145-148: The test duplicates a harness setup and uses unsafe as
any casts for memoryManager and oc.logger when instantiating MemoryPersistQueue;
replace this by reusing the existing createHarness() helper to produce a
properly typed memoryManager and logger, or create narrow mock types (e.g.,
const memoryManager: Pick<MemoryManager, 'saveMessage' | 'loadMessages'> = {
saveMessage: jest.fn(), loadMessages: jest.fn() } and a typed logger) and pass
those into new MemoryPersistQueue so you can remove all as any casts and keep
the test type-safe; update references to MemoryPersistQueue, createHarness,
memoryManager, and logger accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/khaki-rivers-fly.mdpackages/core/src/agent/agent.tspackages/core/src/agent/step-persistence.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core/src/agent/step-persistence.spec.ts (1)
173-175: Avoidanyin the new step-handler test signature.
event: anydrops compile-time guarantees in the new regression path; prefer a typed fixture/interface (orStepResult<ToolSet>-compatible shape).As per coding guidelines, "**/*.ts: Maintain type safety in TypeScript-first codebase".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/agent/step-persistence.spec.ts` around lines 173 - 175, The test uses an untyped handler signature: replace the `any` types in the handler declaration created from (agent as any).createStepHandler(oc, undefined) so the test asserts compile-time safety; change the handler variable signature to the concrete event/result type expected by createStepHandler (for example a StepResult<ToolSet>-compatible interface or the specific StepEvent/StepResult type used across the agent), update the cast from (agent as any) to a narrower type if needed, and ensure the parameter signature for handler is strongly typed instead of `event: any` so the test keeps TypeScript guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/agent/agent.ts`:
- Around line 3661-3665: The code only normalizes assistant IDs when
rawMessageId is missing, which allows provider-assigned assistant IDs to vary;
change the logic in the normalizedMessage assignment so that whenever
responseMessage.role === "assistant" you always return ({ ...responseMessage,
id: fallbackAssistantMessageId } as ModelMessage) regardless of rawMessageId,
otherwise keep responseMessage as-is; update the expression that constructs
normalizedMessage (referencing normalizedMessage, responseMessage, rawMessageId,
and fallbackAssistantMessageId) to enforce this unconditional override for
assistant messages.
In `@packages/core/src/agent/step-persistence.spec.ts`:
- Around line 146-287: Update the test "reuses assistant message id across step
checkpoints after intermediate flushes" to simulate the regression by supplying
explicit, different assistant message IDs across the three handler invocations
(e.g., "m1", "m2", "m3") in the response payloads so the code exercises the path
where the same payload is persisted with changing message_id values;
specifically, modify the three handler calls' response.messages[0] objects to
include an id field with unique values, keep the saveMessage mock and the
persistedAssistantIds extraction/assertions unchanged, and ensure the final
assertions still verify that persisted assistant IDs are all strings and
deduplicate to a single canonical id as before.
---
Nitpick comments:
In `@packages/core/src/agent/step-persistence.spec.ts`:
- Around line 173-175: The test uses an untyped handler signature: replace the
`any` types in the handler declaration created from (agent as
any).createStepHandler(oc, undefined) so the test asserts compile-time safety;
change the handler variable signature to the concrete event/result type expected
by createStepHandler (for example a StepResult<ToolSet>-compatible interface or
the specific StepEvent/StepResult type used across the agent), update the cast
from (agent as any) to a narrower type if needed, and ensure the parameter
signature for handler is strongly typed instead of `event: any` so the test
keeps TypeScript guarantees.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/agent/agent.tspackages/core/src/agent/memory-persist-queue.tspackages/core/src/agent/step-persistence.spec.ts
This was referenced Mar 3, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
Bugs / Features
What is the current behavior?
When step-level conversation persistence (
conversationPersistence.mode = "step") flushes around tool results, the same assistant response can be persisted multiple times with differentmessage_idvalues.This leads to duplicate assistant messages in memory and can trigger downstream provider errors such as duplicate OpenAI reasoning item IDs.
Related issue: #1121
What is the new behavior?
fixes #1121
Notes for reviewers
Validation run locally:
cd packages/core && pnpm exec vitest src/agent/step-persistence.spec.ts src/agent/memory-persistence.integration.spec.ts src/agent/conversation-buffer.spec.tsChangeset added:
.changeset/khaki-rivers-fly.mdSummary by cubic
Prevents duplicate assistant messages during step-level persistence by canonicalizing the assistant message ID and deduping repeated responses across flushes. Fixes #1121 and avoids downstream provider errors from duplicate reasoning item IDs.
Written for commit 01bae0f. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor