[Fix] Recover conversation migration state safely#817
Conversation
Skip admin-only conversation checks for personal routes and persist migration completion metadata before legacy tables are purged. This lets startup recovery handle an existing legacy sentinel without re-reading missing tables or discarding existing ChatLuna data. Tests: not run (not requested)
Probe legacy ChatHub tables before treating the sentinel as authoritative so restored databases still run the built-in migration instead of being marked complete. Keep sentinel-based recovery for genuinely empty legacy state while preserving the existing self-heal path. Tests: npx cross-env TS_NODE_PROJECT=packages/core/tests/tsconfig.json mocha -r tsconfig-paths/register -r esbuild-register -r yml-register --exit 'packages/core/tests/conversation-migration.spec.ts'
|
Caution Review failedPull request was closed or merged during review Walkthrough该PR修改了会话管理、数据迁移验证和权限检查逻辑。增强了conversation管理中间件的条件判断，改进了wipe操作中的元数据写入，优化了room-to-conversation迁移的流程控制，并添加了相应的测试覆盖。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 594cf247e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!resolved.constraint.allowNew) { | ||
| throw new Error('Conversation creation is disabled by constraint.') | ||
| } |
There was a problem hiding this comment.
Removing the assertManageAllowed call in ensureActiveConversation() opens an authorization bypass in the normal chat path: request_conversation always calls this method, so when no active conversation exists a non-admin user can now create one even if manageMode is admin. This contradicts the admin-only management behavior still enforced in conversation_new for non-personal routes and lets regular users bootstrap shared/custom managed conversations just by sending a message.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refines the migration logic from legacy tables to the new conversation system, introducing a sentinel-based recovery mechanism to handle interrupted migrations or fresh installs. It also updates the wipe process to re-initialize metadata and adjusts permission checks in the conversation management middleware. The review feedback highlights a security concern regarding the removal of permission assertions in the conversation service which could lead to unauthorized conversation creation. Additionally, there are suggestions to remove a redundant function call in the migration logic and to optimize sequential database queries using Promise.all for better performance.
| } | ||
| } | ||
|
|
||
| await assertManageAllowed(session, resolved.constraint) |
There was a problem hiding this comment.
Removing the assertManageAllowed check here introduces a permission bypass for automatic conversation creation. While the conversation_new middleware now handles this for explicit commands (with a personal route exception), the service layer's ensureActiveConversation is also called during the normal chat flow. If manageMode is set to admin for a non-personal route, regular users will now be able to trigger conversation creation simply by sending a message, which violates the intended administrative restriction.
if (resolved.constraint.routeMode !== 'personal') {
await assertManageAllowed(session, resolved.constraint)
}| ctx.logger.warn( | ||
| 'Legacy sentinel exists but legacy ChatHub data is still present; continuing migration from legacy tables.' | ||
| ) | ||
| defineLegacyMigrationTables(ctx, true) |
There was a problem hiding this comment.
This call to defineLegacyMigrationTables is redundant. The hasLegacyMigrationData function, which is called just a few lines above at line 122, already ensures that the legacy tables are defined by calling defineLegacyMigrationTables(ctx, true) internally.
| const conversations = (await ctx.database.get( | ||
| 'chatluna_conversation', | ||
| {}, | ||
| { | ||
| limit: 1 | ||
| } | ||
| )) as ConversationRecord[] | ||
| const messages = (await ctx.database.get( | ||
| 'chatluna_message', | ||
| {}, | ||
| { | ||
| limit: 1 | ||
| } | ||
| )) as MessageRecord[] | ||
| const bindings = (await ctx.database.get( | ||
| 'chatluna_binding', | ||
| {}, | ||
| { | ||
| limit: 1 | ||
| } | ||
| )) as BindingRecord[] | ||
| const acl = (await ctx.database.get( | ||
| 'chatluna_acl', | ||
| {}, | ||
| { | ||
| limit: 1 | ||
| } | ||
| )) as ACLRecord[] |
There was a problem hiding this comment.
These four database queries are executed sequentially, which can be optimized by using Promise.all to fetch the existence of data in parallel. This improves startup performance during the recovery flow.
| const conversations = (await ctx.database.get( | |
| 'chatluna_conversation', | |
| {}, | |
| { | |
| limit: 1 | |
| } | |
| )) as ConversationRecord[] | |
| const messages = (await ctx.database.get( | |
| 'chatluna_message', | |
| {}, | |
| { | |
| limit: 1 | |
| } | |
| )) as MessageRecord[] | |
| const bindings = (await ctx.database.get( | |
| 'chatluna_binding', | |
| {}, | |
| { | |
| limit: 1 | |
| } | |
| )) as BindingRecord[] | |
| const acl = (await ctx.database.get( | |
| 'chatluna_acl', | |
| {}, | |
| { | |
| limit: 1 | |
| } | |
| )) as ACLRecord[] | |
| const [conversations, messages, bindings, acl] = (await Promise.all([ | |
| ctx.database.get('chatluna_conversation', {}, { limit: 1 }), | |
| ctx.database.get('chatluna_message', {}, { limit: 1 }), | |
| ctx.database.get('chatluna_binding', {}, { limit: 1 }), | |
| ctx.database.get('chatluna_acl', {}, { limit: 1 }) | |
| ])) as [ConversationRecord[], MessageRecord[], BindingRecord[], ACLRecord[]] | |
1 participant
This pr hardens the built-in conversation migration flow so startup recovery can safely handle an existing legacy sentinel, avoid re-reading already-purged legacy tables, and preserve current ChatLuna data.
New Features
Bug fixes
cannot resolve tableas a missing-table error during legacy cleanup.Other Changes