[Fix] 修复 1.4 alpha conversation 回归#886
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Walkthrough中间件在有显式 target 时调用解析器、无 target 时复用预解析会话；调整会话排序与消息内容空值保护；测试 helpers 新增 DB.set 与 plugin 注册；增加 rollback/stop 回归用例验证解析优先级。 ChangesCore conversation resolution and service improvements
Test infrastructure and assertion updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 introduces optimizations and bug fixes to conversation resolution, sorting, and message formatting. Specifically, it bypasses redundant conversation resolution in rollback_chat and stop_chat middlewares when a valid active conversation is already present in the context options. It also refactors the sorting logic in ConversationService to prioritize sequence numbers, handles potential null values when formatting messages, and updates corresponding tests and helpers.
However, two critical issues were identified in the rollback_chat and stop_chat middlewares: when a user explicitly specifies a target conversation, the code incorrectly bypasses resolution and reuses the current active conversation. This can be resolved by ensuring the existing conversation is only reused when no target conversation is specified.
liuwanwan1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/tests/rollback-chat.spec.ts (2)
465-470: 💤 Low value建议使用更精确的断言验证调用行为。
当前使用
some()只验证至少有一次调用包含正确的targetConversation，但根据测试场景（提供了显式 target 和完整 pre-resolved conversation），理论上应该只调用一次resolveConversation。更精确的断言能更早发现意外的重复调用。♻️ 建议的改进
- assert.equal( - resolveOpts.some( - (opts) => (opts as any).targetConversation === target.id - ), - true - ) + assert.equal(resolveOpts.length, 1) + assert.equal((resolveOpts[0] as any).targetConversation, target.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/rollback-chat.spec.ts` around lines 465 - 470, The test currently asserts that at least one call to resolveConversation included the correct targetConversation by checking resolveOpts.some(...), but given the scenario (explicit target + pre-resolved conversation) you should assert that resolveConversation was called exactly once and that its single call had targetConversation === target.id; change the assertion to check resolveOpts.length === 1 and that resolveOpts[0].targetConversation (or (resolveOpts[0] as any).targetConversation) equals target.id to catch unexpected duplicate calls to resolveConversation.
837-842: 💤 Low value建议使用更精确的断言验证调用行为。
与 rollback_chat 测试类似，使用精确断言能更好地验证
resolveConversation只被调用一次且参数正确。♻️ 建议的改进
- assert.equal( - resolveOpts.some( - (opts) => (opts as any).targetConversation === target.id - ), - true - ) + assert.equal(resolveOpts.length, 1) + assert.equal((resolveOpts[0] as any).targetConversation, target.id)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/tests/rollback-chat.spec.ts` around lines 837 - 842, Replace the loose truthy check on resolveOpts with a precise call assertion: stub/spy resolveConversation and assert it was called exactly once with the expected argument where the targetConversation equals target.id (i.e., use resolveConversation.calledOnce or sinon.assert.calledOnce and calledWithExactly / strict equality against target.id), referencing resolveOpts, resolveConversation and target.id to locate and verify the correct call rather than using assert.equal(...some(...), true).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/tests/rollback-chat.spec.ts`:
- Around line 465-470: The test currently asserts that at least one call to
resolveConversation included the correct targetConversation by checking
resolveOpts.some(...), but given the scenario (explicit target + pre-resolved
conversation) you should assert that resolveConversation was called exactly once
and that its single call had targetConversation === target.id; change the
assertion to check resolveOpts.length === 1 and that
resolveOpts[0].targetConversation (or (resolveOpts[0] as
any).targetConversation) equals target.id to catch unexpected duplicate calls to
resolveConversation.
- Around line 837-842: Replace the loose truthy check on resolveOpts with a
precise call assertion: stub/spy resolveConversation and assert it was called
exactly once with the expected argument where the targetConversation equals
target.id (i.e., use resolveConversation.calledOnce or sinon.assert.calledOnce
and calledWithExactly / strict equality against target.id), referencing
resolveOpts, resolveConversation and target.id to locate and verify the correct
call rather than using assert.equal(...some(...), true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 98c0d0c7-5785-408a-b2fd-fe7dbca651e7
📒 Files selected for processing (3)
packages/core/src/middlewares/chat/rollback_chat.tspackages/core/src/middlewares/chat/stop_chat.tspackages/core/tests/rollback-chat.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/middlewares/chat/stop_chat.ts
- packages/core/src/middlewares/chat/rollback_chat.ts
|
一样的问题，A conversation must be resolved before this pull request can be merged. |
…lewares When targetConversation is null, resolve_conversation middleware has already populated context.options.conversation — reuse it directly instead of re-resolving with redundant null checks.
dingyi222666
force-pushed
the
fix/alpha-conversation-regressions
branch
from
cc29b32 to
459cf01
Compare
2 participants
关联
背景
继续测试
1.4 alpha时，packages/core的 conversation 相关用例暴露了几处回归和测试夹具失配。该 PR 针对这些问题做集中修复。问题
content可能为null，导出 Markdown 时会继续调用gzipDecode(null)并失败。preset lane列出会话时先按bindingKey排序，导致seq展示和数字切换顺序不稳定。rollback_chat/stop_chat已有完整conversation resolution时仍会重复解析。targetConversation时，不能复用当前预解析会话，否则会忽略用户指定的目标会话。database.set、默认pluginchat chain，并仍引用已移除的extension-agentcli service 文件。修复
exportMarkdown格式化消息时允许message.content为空，并回退使用message.text。listConversations跨 route / preset lane 排序时优先使用conversation.seq，仅在seq相同且合并多 binding 时再比较bindingKey。rollback_chat/stop_chat在没有显式targetConversation且现有解析结果完整时复用context.options.conversation。rollback_chat和stop_chat回归测试。chatluna/after-binding-update。验证
本地已验证：
GitHub Actions 已通过：
preparelintbuildCodeFactorCodeRabbit