fix(core): persist full assistant parts and add e2e coverage#1131
Merged
Conversation
🦋 Changeset detectedLatest commit: e553579 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
📝 WalkthroughWalkthroughMerges assistant messages with the same id to preserve reasoning, tool-call, tool-result, and text across intermediate flushes when conversationPersistence.mode = "step"; adds unit/integration and end-to-end tests (LibSQL & Postgres) and CI jobs for E2E runs; modifies ensureConversationExists to return a boolean and enforce precondition checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Agent as Agent
participant Buffer as ConversationBuffer
participant Manager as MemoryManager
participant DB as Database
Agent->>Buffer: handleAssistantMessage(reasoning + tool-call)
Buffer->>Buffer: append assistant message (parts: reasoning, tool-call)
Agent->>Buffer: flush (checkpoint 1)
Buffer->>Manager: persist pending message
Manager->>DB: save message (parts: reasoning, tool-call)
DB-->>Manager: ok
Manager-->>Buffer: ack
Agent->>Buffer: handleAssistantMessage(tool-result)
Buffer->>Buffer: append tool-result part
Agent->>Buffer: handleAssistantMessage(text, same assistant id)
Buffer->>Buffer: detect same assistant id → merge parts into existing message
Agent->>Buffer: flush (checkpoint 2)
Buffer->>Manager: persist merged message
Manager->>DB: save message (parts: reasoning, tool-call, tool-result, text)
DB-->>Manager: ok
Manager-->>Buffer: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 15 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/e2e/src/message-persistence.libsql.e2e.spec.ts">
<violation number="1" location="packages/e2e/src/message-persistence.libsql.e2e.spec.ts:39">
P2: Ensure the libsql client is closed even when assertions fail (e.g., wrap the query/assertion block in `try/finally` or close in `afterEach`).</violation>
</file>
<file name=".github/workflows/prerelease.yml">
<violation number="1" location=".github/workflows/prerelease.yml:58">
P3: The E2E workflow spins up a Postgres service that the tests never use (they start their own docker-compose DB on port 5433). This adds unnecessary runtime and resource overhead. Remove the service or update the tests/env to use it.</violation>
</file>
<file name=".github/workflows/pull-request.yml">
<violation number="1" location=".github/workflows/pull-request.yml:204">
P2: Change detection hard-codes `origin/main`, which can skip E2E tests for PRs whose base branch isn’t main. Use the PR base ref so the diff matches the actual target branch.</violation>
</file>
<file name=".github/workflows/release.yml">
<violation number="1" location=".github/workflows/release.yml:54">
P3: The new postgres service is redundant because the E2E test script spins up its own Postgres on port 5433 and the tests default to that port. This extra service is unused in CI and adds unnecessary startup time/resources.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Deploying voltagent with
|
| Latest commit: |
e553579
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f79faa92.voltagent.pages.dev |
| Branch Preview URL: | https://fix-message-persistence-1130.voltagent.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/pull-request.yml (1)
176-213: E2E gate still pays PostgreSQL startup cost on every PRBecause service containers start before steps, Line 182-195 spins Postgres even when Line 201-212 decides to skip. Consider moving change detection to a lightweight precheck job and gating this job with
if:at job level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pull-request.yml around lines 176 - 213, The e2e-tests job currently starts the postgres service unconditionally while the change detection happens in the job steps (the step with id check_e2e), so the job still pays the container startup cost; create a lightweight separate job (e.g., "e2e-precheck") that runs the change detection logic (the same git diff check used in the step with id check_e2e) and emits an output like e2e_changed, then gate the existing e2e-tests job at the job level using an if: condition that references that precheck output (e.g., if: needs.e2e-precheck.outputs.e2e_changed == 'true'), so postgres and other services in e2e-tests are only started when changes actually require E2E runs.packages/e2e/package.json (1)
20-20: Separate local DB orchestration from CI test executionLine 20 always starts Docker Compose inside the package script. Since CI jobs may already provide Postgres, split this into
test:local(compose + vitest) andtest:ci(vitest only) to avoid duplicate infra and config drift.Possible script split
"scripts": { "build": "echo \"No build step for `@voltagent/e2e`\"", "lint": "biome check .", - "test": "sh -c 'docker compose -f ../postgres/docker-compose.test.yaml up -d && vitest run --config vitest.config.mts; status=$?; docker compose -f ../postgres/docker-compose.test.yaml down -v; exit $status'" + "test": "pnpm run test:local", + "test:local": "sh -c 'docker compose -f ../postgres/docker-compose.test.yaml up -d && vitest run --config vitest.config.mts; status=$?; docker compose -f ../postgres/docker-compose.test.yaml down -v; exit $status'", + "test:ci": "vitest run --config vitest.config.mts" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e/package.json` at line 20, Split the combined "test" npm script into two scripts: "test:local" which runs the Docker Compose orchestration and then invokes the vitest runner (preserving the existing sequence and exit-status propagation and the docker compose down -v cleanup using the same ../postgres/docker-compose.test.yaml and vitest run --config vitest.config.mts command), and "test:ci" which runs only vitest run --config vitest.config.mts (no Docker Compose). Update the package.json scripts section to replace the single "test" entry with these two named scripts so CI can call "test:ci" while local development uses "test:local".packages/e2e/src/message-persistence.libsql.e2e.spec.ts (1)
39-73: Ensureclient.close()always runs viafinally.If a query/assertion fails before the last line, the LibSQL client may remain open for the test process.
Suggested refactor
const client = createClient({ url: dbUrl }); - - const conversationRows = await client.execute({ - sql: `SELECT id, user_id, resource_id FROM ${tablePrefix}_conversations WHERE id = ?`, - args: [conversationId], - }); - expect(conversationRows.rows).toHaveLength(1); - expect(conversationRows.rows[0]?.id).toBe(conversationId); - expect(conversationRows.rows[0]?.user_id).toBe(userId); - expect(conversationRows.rows[0]?.resource_id).toBe("persistence-e2e-agent"); - - const messageRows = await client.execute({ - sql: `SELECT role, parts FROM ${tablePrefix}_messages WHERE conversation_id = ? AND user_id = ? ORDER BY created_at ASC`, - args: [conversationId, userId], - }); - const assistantRows = messageRows.rows.filter((row) => row.role === "assistant"); - expect(assistantRows).toHaveLength(5); - - for (const [index, row] of assistantRows.entries()) { - const rawParts = row.parts; - if (typeof rawParts !== "string") { - throw new Error("Expected libsql parts to be JSON string."); - } - const parts = JSON.parse(rawParts) as UIMessage["parts"]; - assertAssistantParts({ id: "libsql-row", role: "assistant", parts }, index + 1); - } - - const stepCountRows = await client.execute({ - sql: `SELECT COUNT(*) AS count FROM ${tablePrefix}_steps WHERE conversation_id = ? AND user_id = ?`, - args: [conversationId, userId], - }); - expect(Number(stepCountRows.rows[0]?.count ?? 0)).toBeGreaterThan(0); - - client.close(); + try { + const conversationRows = await client.execute({ + sql: `SELECT id, user_id, resource_id FROM ${tablePrefix}_conversations WHERE id = ?`, + args: [conversationId], + }); + expect(conversationRows.rows).toHaveLength(1); + expect(conversationRows.rows[0]?.id).toBe(conversationId); + expect(conversationRows.rows[0]?.user_id).toBe(userId); + expect(conversationRows.rows[0]?.resource_id).toBe("persistence-e2e-agent"); + + const messageRows = await client.execute({ + sql: `SELECT role, parts FROM ${tablePrefix}_messages WHERE conversation_id = ? AND user_id = ? ORDER BY created_at ASC`, + args: [conversationId, userId], + }); + const assistantRows = messageRows.rows.filter((row) => row.role === "assistant"); + expect(assistantRows).toHaveLength(5); + + for (const [index, row] of assistantRows.entries()) { + const rawParts = row.parts; + if (typeof rawParts !== "string") { + throw new Error("Expected libsql parts to be JSON string."); + } + const parts = JSON.parse(rawParts) as UIMessage["parts"]; + assertAssistantParts({ id: "libsql-row", role: "assistant", parts }, index + 1); + } + + const stepCountRows = await client.execute({ + sql: `SELECT COUNT(*) AS count FROM ${tablePrefix}_steps WHERE conversation_id = ? AND user_id = ?`, + args: [conversationId, userId], + }); + expect(Number(stepCountRows.rows[0]?.count ?? 0)).toBeGreaterThan(0); + } finally { + client.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/e2e/src/message-persistence.libsql.e2e.spec.ts` around lines 39 - 73, The test currently may leave the LibSQL client open if an assertion throws; wrap the usage of client (created by createClient) and all client.execute/assert logic in a try/finally block so that client.close() is always called in the finally clause; ensure client is declared before the try and that the finally calls client.close() even if earlier awaits or assertions fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/prerelease.yml:
- Around line 65-67: The workflow exposes Postgres on port 5432 but the e2e
tests expect port 5433, causing connectivity failures; update the workflow
service to align ports or export VOLTAGENT_E2E_POSTGRES_PORT so tests target the
same port — specifically change the service ports mapping to expose 5433:5432 or
set the environment variable VOLTAGENT_E2E_POSTGRES_PORT=5432/5433 (matching the
test default) in the job configuration so the e2e test runner and the workflow
Postgres service use the same port.
In `@packages/core/src/agent/memory-persistence.integration.spec.ts`:
- Line 292: The test assertion currently weakens types by using (part: any) when
mapping finalMessage.parts; change the callback to use the proper part type
(e.g., UIMessage["parts"][number] or let Type = typeof
finalMessage.parts[number] so the mapper reads (part:
UIMessage["parts"][number]) or infer the type via a typed const for finalMessage
before mapping) and update the mapping assertion to remove any while keeping the
same logic referencing finalMessage and its parts.
In `@packages/core/src/memory/manager/memory-manager.ts`:
- Around line 245-246: ensureConversationExists currently logs-and-swallows
failures so the subsequent call to
this.conversationMemory?.saveConversationSteps?.(steps) can run even when the
conversation wasn't created; change the flow to fail-fast by making
ensureConversationExists surface errors (throw or return a boolean) and then
guard the call to saveConversationSteps accordingly: either let
ensureConversationExists throw and remove its internal swallow, or check its
returned success value before calling conversationMemory.saveConversationSteps,
and if it failed, log and abort/throw to avoid orphaned steps. Reference
ensureConversationExists and saveConversationSteps/conversationMemory to locate
the fix.
---
Nitpick comments:
In @.github/workflows/pull-request.yml:
- Around line 176-213: The e2e-tests job currently starts the postgres service
unconditionally while the change detection happens in the job steps (the step
with id check_e2e), so the job still pays the container startup cost; create a
lightweight separate job (e.g., "e2e-precheck") that runs the change detection
logic (the same git diff check used in the step with id check_e2e) and emits an
output like e2e_changed, then gate the existing e2e-tests job at the job level
using an if: condition that references that precheck output (e.g., if:
needs.e2e-precheck.outputs.e2e_changed == 'true'), so postgres and other
services in e2e-tests are only started when changes actually require E2E runs.
In `@packages/e2e/package.json`:
- Line 20: Split the combined "test" npm script into two scripts: "test:local"
which runs the Docker Compose orchestration and then invokes the vitest runner
(preserving the existing sequence and exit-status propagation and the docker
compose down -v cleanup using the same ../postgres/docker-compose.test.yaml and
vitest run --config vitest.config.mts command), and "test:ci" which runs only
vitest run --config vitest.config.mts (no Docker Compose). Update the
package.json scripts section to replace the single "test" entry with these two
named scripts so CI can call "test:ci" while local development uses
"test:local".
In `@packages/e2e/src/message-persistence.libsql.e2e.spec.ts`:
- Around line 39-73: The test currently may leave the LibSQL client open if an
assertion throws; wrap the usage of client (created by createClient) and all
client.execute/assert logic in a try/finally block so that client.close() is
always called in the finally clause; ensure client is declared before the try
and that the finally calls client.close() even if earlier awaits or assertions
fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cd43885-7ce4-4c3d-8f54-24cfc6f4b0ce
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/five-candles-care.md.github/workflows/prerelease.yml.github/workflows/pull-request.yml.github/workflows/release.ymlpackages/core/src/agent/conversation-buffer.spec.tspackages/core/src/agent/conversation-buffer.tspackages/core/src/agent/memory-persistence.integration.spec.tspackages/core/src/memory/manager/memory-manager.spec.tspackages/core/src/memory/manager/memory-manager.tspackages/e2e/package.jsonpackages/e2e/src/message-persistence.libsql.e2e.spec.tspackages/e2e/src/message-persistence.postgres.e2e.spec.tspackages/e2e/src/message-persistence.shared.tspackages/e2e/vitest.config.mts
Comment on lines
+65
to
+67
| ports: | ||
| - 5432:5432 | ||
| options: >- |
Contributor
There was a problem hiding this comment.
Align Postgres port between workflow service and e2e test defaults.
The e2e Postgres spec uses 5433 by default (Line 16 in packages/e2e/src/message-persistence.postgres.e2e.spec.ts), but this job exposes 5432 and does not set VOLTAGENT_E2E_POSTGRES_PORT. That can make this CI job fail with an unreachable DB.
Suggested fix
- name: Run E2E Tests
run: pnpm --filter `@voltagent/e2e` test
+ env:
+ VOLTAGENT_E2E_POSTGRES_PORT: "5432"Also applies to: 85-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/prerelease.yml around lines 65 - 67, The workflow exposes
Postgres on port 5432 but the e2e tests expect port 5433, causing connectivity
failures; update the workflow service to align ports or export
VOLTAGENT_E2E_POSTGRES_PORT so tests target the same port — specifically change
the service ports mapping to expose 5433:5432 or set the environment variable
VOLTAGENT_E2E_POSTGRES_PORT=5432/5433 (matching the test default) in the job
configuration so the e2e test runner and the workflow Postgres service use the
same port.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/memory/manager/memory-manager.spec.ts`:
- Around line 259-261: The structural cast for the spy drops type safety by
using "as any" on manager.ensureConversationExists; change the cast to the
actual method signature so the input parameter union includes UIMessage (e.g.
cast manager to { ensureConversationExists: (arg: Conversation | UIMessage) =>
Promise<boolean> } or the precise signature used in your code) and update the
vi.spyOn call to use that typed cast; also import UIMessage from "ai" if not
already imported so the typed refactor compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 983de1c8-56bd-41a5-99a9-c4f48cbf806c
📒 Files selected for processing (3)
packages/core/src/agent/memory-persistence.integration.spec.tspackages/core/src/memory/manager/memory-manager.spec.tspackages/core/src/memory/manager/memory-manager.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/agent/memory-persistence.integration.spec.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/memory/manager/memory-manager.spec.ts (1)
297-299: Consider assertingensureConversationExistscall arguments as well.You already assert call count; adding argument verification will better protect against wiring regressions.
♻️ Suggested assertion hardening
expect(ensureConversationExistsSpy).toHaveBeenCalledTimes(1); + expect(ensureConversationExistsSpy).toHaveBeenCalledWith( + context, + "user-steps-fail", + "conv-steps-fail", + context.input, + ); expect(saveConversationStepsSpy).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/memory/manager/memory-manager.spec.ts` around lines 297 - 299, Add an assertion that the ensureConversationExists spy was called with the expected arguments to guard against wiring regressions: in the test near the ensureConversationExistsSpy and saveConversationStepsSpy assertions, assert ensureConversationExistsSpy.toHaveBeenCalledWith(...) using the exact conversation id/object and any context/options the test supplies (e.g., the conversationId, userId or request payload used earlier in the test). Keep the existing toHaveBeenCalledTimes(1) and ensure saveConversationStepsSpy remains asserted as not called.
🤖 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/memory/manager/memory-manager.spec.ts`:
- Around line 297-299: Add an assertion that the ensureConversationExists spy
was called with the expected arguments to guard against wiring regressions: in
the test near the ensureConversationExistsSpy and saveConversationStepsSpy
assertions, assert ensureConversationExistsSpy.toHaveBeenCalledWith(...) using
the exact conversation id/object and any context/options the test supplies
(e.g., the conversationId, userId or request payload used earlier in the test).
Keep the existing toHaveBeenCalledTimes(1) and ensure saveConversationStepsSpy
remains asserted as not called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04b089af-9cef-48eb-8c8e-64063d35ece6
📒 Files selected for processing (1)
packages/core/src/memory/manager/memory-manager.spec.ts
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
conversationPersistence.mode = "step"is used and the model invokes tools, stored assistant history can lose non-text parts. In affected flows, persisted messages can end up with only final text while reasoning and tool call/result context are missing, which breaks multi-turn continuity.What is the new behavior?
packages/e2eworkspace and true agent-level E2E coverage (mock LLM + real adapter persistence) for:stateandoutputfields turn-by-turn, not only text.E2E Testsjobs in PR, release, and prerelease workflows.fixes #1130
Notes for reviewers
Validated locally with:
pnpm --filter @voltagent/e2e testAlso added core changeset:
.changeset/five-candles-care.mdSummary by cubic
Preserves full assistant parts (reasoning, tool calls/results, text) in step-mode by merging same-id assistant checkpoints and persisting tool state across flushes. Hardens conversation persistence by ensuring conversations exist and failing fast if they don’t; adds LibSQL/PostgreSQL E2E coverage and CI gating; fixes #1130.
Bug Fixes
New Features
Written for commit e553579. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores