improvement(copilot): make copilot_messages the sole transcript store, remove JSONB dual-write#4826
improvement(copilot): make copilot_messages the sole transcript store, remove JSONB dual-write#4826waleedlatif1 wants to merge 1 commit into
Conversation
…, remove JSONB dual-write Stop writing/reading the legacy copilot_chats.messages JSONB column now that reads are cut over to copilot_messages. Make appendCopilotChatMessages the primary write (throws on failure instead of swallowing), repoint peripheral readers (workspace VFS, chat cleanup, data drains, fork, superuser import) to copilot_messages, and persist the assistant turn inside finalizeAssistantTurn's transaction so it commits atomically with the stream-marker clear. The column itself is dropped in a follow-up migration after this bakes.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Turn finalization is tighter: Chat create/fork/import inserts omit Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 8 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| if (updated) { | ||
| // `copilot_messages` is the sole store — propagate failures so a lost | ||
| // user message fails the request before the stream opens, rather than | ||
| // silently dropping the turn. |
There was a problem hiding this comment.
User persist lacks transaction
High Severity
persistUserMessage sets conversationId on copilot_chats in one statement, then calls appendCopilotChatMessages separately. If the append fails after the update commits, the chat shows an active stream with no user row in copilot_messages, which can block later turns and lose the message despite the HTTP error.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| }) | ||
| } | ||
| } catch (err) { | ||
| logger.warn('Failed to persist chat messages', { |
There was a problem hiding this comment.
Inbox swallows message failures
High Severity
Background inbox persistence still catches all errors after only bumping updatedAt, but it no longer writes the legacy JSONB transcript. When appendCopilotChatMessages throws, the handler logs and returns while the orchestration may already have completed, so the task transcript can be permanently lost with no surfaced failure.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| // Update chat metadata (plan artifact / config); the transcript itself is | ||
| // written to copilot_messages via replaceCopilotChatMessages below. | ||
| const updateData: Record<string, unknown> = { | ||
| messages: normalizedMessages, |
There was a problem hiding this comment.
Update metadata before messages
Medium Severity
The route commits planArtifact, config, and updatedAt on copilot_chats before replaceCopilotChatMessages. If replace throws, the handler returns 500 but metadata changes remain while the normalized transcript may be unchanged or partially updated, diverging client state from the database.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| [assistantMessage], | ||
| { streamId: userMessageId, chatModel }, | ||
| tx | ||
| ) |
There was a problem hiding this comment.
Finalize misses existing assistant
High Severity
finalizeAssistantTurn treats a turn as already answered only when an assistant row has stream_id equal to the user message id. Assistants saved earlier via replaceCopilotChatMessages (e.g. update-messages) keep a null stream_id, so finalize can append a second assistant for the same user turn.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| }) | ||
| } | ||
|
|
||
| return messagesAfter |
There was a problem hiding this comment.
Completion swallows finalize errors
High Severity
buildOnComplete catches all errors from finalizeAssistantTurn and only logs them. After moving the assistant write into the finalize transaction, a failed appendCopilotChatMessages rolls back the marker clear and assistant insert, leaving conversation_id set while the stream appears finished to the client.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| type: parent.type, | ||
| title, | ||
| model: parent.model, | ||
| messages: forkedMessages, |
There was a problem hiding this comment.
Fork leaves empty chat
Medium Severity
Fork inserts the new copilot_chats row first, then calls appendCopilotChatMessages for the sliced transcript. If the append throws, the handler returns 500 but the new chat row remains with no copilot_messages, producing an empty forked task.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| if (imported && Array.isArray(chat.messages) && chat.messages.length > 0) { | ||
| await appendCopilotChatMessages(imported.id, chat.messages as PersistedMessage[], { | ||
| if (imported && sourceMessages.length > 0) { | ||
| await appendCopilotChatMessages(imported.id, sourceMessages, { |
There was a problem hiding this comment.
Import chat missing transcript
Medium Severity
Each imported copilot chat is inserted before appendCopilotChatMessages runs. If append fails for one chat, the route errors out while earlier chats may already be committed, and the failing chat row can remain without any rows in copilot_messages.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
| }) | ||
| } | ||
|
|
||
| return messagesAfter |
There was a problem hiding this comment.
Error path swallows finalize
High Severity
When orchestration fails, buildOnError calls finalizeAssistantTurn to clear conversation_id, but its catch only logs failures. If finalize now throws (e.g. copilot_messages write fails inside the transaction), the marker stays set and later requests can hit a stuck in-progress stream even though the error path already ran.
Reviewed by Cursor Bugbot for commit f1a0d4c. Configure here.
Greptile SummaryThis PR completes the cutover from
Confidence Score: 4/5The core migration is sound: the atomic transaction in finalizeAssistantTurn closes a real data-loss window, error propagation is correct, and the dedup logic correctly relies on the row-level lock. The two remaining concerns are performance-shaped rather than correctness-shaped. The critical paths (stream finalization, user-message persistence, cleanup) are all correctly migrated with appropriate error handling and transactional guarantees. The workspace-vfs correlated subqueries and the import-workflow N+1 sequential reads are both non-blocking quality issues that won't cause incorrect behavior, just potential slowness under load. apps/sim/lib/copilot/vfs/workspace-vfs.ts (two correlated subqueries per row) and apps/sim/app/api/superuser/import-workflow/route.ts (sequential loadCopilotChatMessages per chat in loop) Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Post as post.ts
participant TS as terminal-state.ts
participant MS as messages-store.ts
participant DB as PostgreSQL
Client->>Post: POST /copilot/chat
Post->>DB: "UPDATE copilot_chats SET conversationId=userMsgId"
Post->>MS: "appendCopilotChatMessages(userMsg, streamId=userMsgId)"
MS->>DB: SELECT MAX(seq) FROM copilot_messages
MS->>DB: INSERT INTO copilot_messages ON CONFLICT DO UPDATE
Note over Client,DB: Stream completes
Post->>TS: finalizeAssistantTurn(chatId, userMessageId, assistantMsg)
TS->>DB: BEGIN TRANSACTION
DB-->>TS: SELECT copilot_chats FOR UPDATE
TS->>DB: "SELECT copilot_messages WHERE streamId=userMsgId AND role=assistant"
alt No existing assistant
TS->>DB: "UPDATE copilot_chats SET conversationId=NULL"
TS->>MS: "appendCopilotChatMessages(assistantMsg, executor=tx)"
MS->>DB: SELECT MAX(seq) inside tx
MS->>DB: INSERT INTO copilot_messages inside tx
DB-->>TS: COMMIT (marker clear + message insert atomic)
else Assistant already persisted
TS->>DB: "UPDATE copilot_chats SET conversationId=NULL"
DB-->>TS: COMMIT (marker clear only)
end
|
1 participant
Summary
copilot_chats.messagesJSONB column now that all transcript reads are cut over to the normalizedcopilot_messagestable.appendCopilotChatMessages/replaceCopilotChatMessagesthe primary store and throw on failure (previously best-effort/swallowed — a swallowed write now means permanent message loss).finalizeAssistantTurn's transaction so it commits atomically with the stream-marker clear (closes the "marker cleared but message lost" window).copilot_messages: workspace VFS task materialization, chat-cleanup file collection, data-drains export (batched per-page assembly, no N+1), chat fork, and superuser workflow import.messages-dual-write.ts→messages-store.ts(no longer "dual"); dropped the now-impossible user-existence dedup check infinalizeAssistantTurn.ALTER TABLE copilot_chats DROP COLUMN messages) + reconcile-script removal are intentionally deferred to a follow-up migration PR so this can bake with a safe rollback.Type of Change
Testing
lib/copilot/chat,lib/cleanup,app/api/copilot/chat,app/api/mothership.tsc --noEmitclean (0 errors), biome clean,check:api-validationpasses.copilot_messagesare byte-identical and correctly ordered; the JSONB column is no longer written.Checklist